Skip to content

Analyzer won't promote type of "this" #32120

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Hixie opened this issue Feb 11, 2018 · 14 comments
Closed

Analyzer won't promote type of "this" #32120

Hixie opened this issue Feb 11, 2018 · 14 comments
Labels
area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language).

Comments

@Hixie
Copy link
Contributor

Hixie commented Feb 11, 2018

Consider this perfectly reasonable code:

class A<T> {
  void test() {
    if (this is A<double>) {
      final A<double> self = this;
    }
  }  
}

The analyzer complains:

A value of type 'A<T>' can't be assigned to a variable of type 'A<double>'

...despite the fact that we know for a fact that this is an A<double> where it is assigned to the variable of type A<double>.

@Hixie
Copy link
Contributor Author

Hixie commented Feb 11, 2018

It won't even work if you first assign it to another variable, so I guess it's not specific to "this":

class A<T> {
  void test() { 
    final A<T> a = this;
    if (a is A<double>) {
      final A<double> self = a; // "A value of type 'A<T>' can't be assigned to a variable of type 'A<double>'"
    }
  }  
}

@bwilkerson
Copy link
Member

@leafpetersen
Copy link
Member

We only do promotion when you test against a subtype of the static type of the thing being tested. This guarantees you don't "lose" capabilities via promotion. We don't know that A<double> <: A<T>: this could be dynamically an A<Foozle>.

If you assign to a variable of type Object, the promotion will work.

There does seem to be something slightly different about testing against a different instance of the same class. Something we could consider for future improvements.

Another possibility (which has some appeal) is to have a separate promoting instance check that always promotes (or perhaps a way to suppress promotion, since you almost always want it).

Related: #25260

@Hixie
Copy link
Contributor Author

Hixie commented Feb 11, 2018

I don't really see what I could be losing here. What can I do with an A<T> that I can't do with an A<double>?

FWIW, and I think there's another open bug about this, what I really want from type promotion is unions. If I have a variable x of static type X, and I say assert(x is Y), where Y and X are unrelated, then IMHO x should now be of type X | Y.

@leafpetersen
Copy link
Member

I don't really see what I could be losing here. What can I do with an A that I can't do with an A?

See example below. As I said though, something does feel different here when you're just converting between instances of the same class - you don't lose methods, you just lose assignment.

what I really want from type promotion is unions.

Intersections actually, but yes. You want the type system to know that x is of type X & Y: that is, it has all of the capabilities of both. There's definitely interest in exploring this.

class A<T> {
  void bar(T y) { ... }
  void test(T x) {
    if (this is A<double>) {
      bar(x); // Error, T is not assignable to double
   }
 }
}

@Hixie
Copy link
Contributor Author

Hixie commented Feb 11, 2018

if this is A<double> then T is double, so why would bar(x) not be valid? I guess because the system doesn't think that deeply and still views T as a distinct type.

Intersections actually

Ah, right, yes.

@bwilkerson bwilkerson added the area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). label Feb 11, 2018
@eernstg
Copy link
Member

eernstg commented Feb 12, 2018

I think the core language design issue here is the following. We do not recommend the following kind of testing on this, because it conflicts with the basic structure of OO design:

class A {
  void foo() {
    print("We're doing this every time.");
    if (this is B) this.bar();
  }
}

class B extends A {
  void bar() { print("This is a special thing we only do in B"); }
}

The preferred way to do the same thing is to use OO dispatch to obtain the desired control flow rather than an explicit type test on this. There are several ways to do that---here's one example:

class A {
  foo() { print("We're doing this every time."); }
}

class B extends A {
  foo() { super(); bar(); }
  bar() { print("This is a special thing we only do in B"); }
}

We could also use the template method design pattern which will handle situations where we want to inject special behavior in the middle of a method body, or more than once, etc., but the point is that we don't make control flow decisions based on the type of this, because it's better (and more idiomatic OO) to make those decisions using late binding of methods (in this case: foo).

However, in some cases there is no non-trivial superinterface relationship because the type test is concerned with a type argument; so we may test this is A<double> where it is only statically known that this is A<T> and T <: Object (the point is that we don't "traverse any extends, implements or with edges" in the subtype graph for the basic subtype relation, but derived subtype tests such as tests on type arguments can do it). In these cases there is no new class to put a different method implementation into, so we can't use the standard OO techniques to handle it using late binding.

Some languages (e.g., Cecil) support "conditional method implementations":

class A<T> {
  void test() when T extends double {
    // In this scope it is known that `T <: double` and `this: A<T>`.
    final A<double> self = this;
    ...
  }
  void test() otherwise {
    // Here we only know that `T <: Object` and `this: A<T>`.
    ...
  } 
}

A mechanism like this would allow us to include at least some of those situations where we test type arguments, and it's actually very cool when it is used to say things like "a list can be sorted iff its element type can be sorted" (so List<T>.sort exists for some values of T and not for others).

But I think that this whole language design area tends to invite designs that are less idiomatically OO, and hence the whole community may end up having higher quality code if we don't go down that path.

In summary, it might be cool to support promotion on this, especially wrt type arguments, but it's not obviously good for the design quality of Dart software at large.

@Hixie
Copy link
Contributor Author

Hixie commented Feb 12, 2018

@eernstg I totally agree with what you say above. The specific case I'm dealing with is that we have found that we desperately need a way to chain from an Animation to a Tween. Right now you create an Animation, then create a Tween, then say tween.animate(animation) to link them, and people find that super confusing. They want to create the Animation, then call something on that animation and pass it a new tween, as in animation.drive(tween). However, this operation only makes sense for Animation<double> instances.

I tried a variety of solutions like having an explicit AnimationDouble class, but it isn't really enough, because Animation<double> types naturally arise out of the type system.

For now I've added drive to Animation<T> and I assert that T is double in the implementation (and in the docs), but that's when I ran into the problem above. Instead I have to use dynamic or other such tricks to convince the type system that I know what I'm doing.

@eernstg
Copy link
Member

eernstg commented Feb 13, 2018

I don't have any clever ideas right now, but maybe the animate method should have had a different name, say, getAnimatedBy? The DartDoc comment on that method seems to support the notion that the name is misleading, and if that's the main source of confusion then it might be worthwhile to perform a rename, possibly with two methods (oldname+newname) and @deprecated.

@Hixie
Copy link
Contributor Author

Hixie commented Feb 13, 2018

@InMatrix can talk about this more, but my understanding is that the problem is which object the method is on, not what the method is called (we can't really rename the method anyway, that would be a breaking change).

Specifically, what people seem to do is figure out they need an AnimationController, and then they look for a method on that they can use to take the next step. Currently there isn't one. I tried adding drive() just to AnimationController, but the problem then is that while it works for the simplest case, it quickly fails because you can't chain it (since the next object you get is an Animation<double>, not an AnimationController or an AnimationDouble).

@InMatrix
Copy link
Collaborator

There's a bit more context in this bug: flutter/flutter#14482

We'd like to achieve two things by placing the method on the AnimationController object: 1) increasing the discoverability of the method, since the controller is likely to be defined first, and 2) better communicating the controller-controllee relationship between the AnimationController and the resulting Animation.

@eernstg
Copy link
Member

eernstg commented Feb 14, 2018

OK,

.. we can't really rename the method ..

must be the main thing. It does make a difference whether a.isGreaterThan(b) means a is greater than b or b is greater than a, that is, the naming and the order are not independent, and in the concrete context it seems that tween.animate(animation) should really be pronounced `let the animation animate the tween'. ;-)

Anyway, I can see from flutter/flutter#14482 that the story is so much longer that I should avoid jumping to conclusions, so the best solution at this point may then well be the drive method that you already introduced. Later on, we can always consider things like

class Animation<T> {
  void drive(Tween tween) when T extends double {...}
  ...
}

main() {
  Tween tween; 
  Animation<double> doubleAnimation;
  Animation<num> numAnimation;
  
  doubleAnimation.drive(tween); // OK.
  numAnimation.drive(tween); // Error: There is no `drive` method on an `Animation<num>`
}

@lrhn
Copy link
Member

lrhn commented Jun 26, 2019

I see two issues here:

  • Promotion of this. We don't do that (according to a quick test in DartPad):

      class A { foo() { if (this is B) print(this.bar); } }
      class B extends A { String bar = "bar"; }
      main() { B().foo(); }

    The this.bar is not allowed, so we don't promote this to B.
    It would be useful, though.

  • Promotion inside a generic type parameter:
    ...<T extends num> ... { List<T> x = ...; if (x is List<int>) {
    Should this promote x to List<T & int>? Currently it doesn't, but it seems safe since you can only
    implement a generic interface with one type.

As stated, the workaround for the original problem, on both counts, would be:

class A<T> {
  void test() {
    final A<Object> self = this;
    if (self is A<double>) {
       ... use self when you need double, this when you need T...
    }
  }  
}

@stereotype441
Copy link
Member

I'm closing this out in favor of language issue dart-lang/language#1397 (which I'm currently considering for possible inclusion into language feature inference-update-2, for Dart 3.2).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language).
Projects
None yet
Development

No branches or pull requests

7 participants