[go: nahoru, domu]

Skip to content
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

Infer generics in is check for type promotion #2047

Open
rkj opened this issue Jul 28, 2018 · 19 comments
Open

Infer generics in is check for type promotion #2047

rkj opened this issue Jul 28, 2018 · 19 comments
Labels
inference small-feature A small feature which is relatively cheap to implement.

Comments

@rkj
Copy link
rkj commented Jul 28, 2018

Consider following code:

abstract class A<T> {}
abstract class B<T> implements A<T> {
  b();
}

void foo<T>(A<T> a) {
  if (a is B) {
    return a.b();
  }
}

It gives error message:

// The method 'b' isn't defined for the class 'A'.

so it seems the type promotion on a didn't happen inside the if block. To fix this I can change the condition to be: if (a is B<T>).

This somewhat makes sense, but it seems to me that in this case compiler could infer that B must be B<T>, if it's casted from A<T>. I guess it could be some subclass of T, but that would still be substitutable for B. Or maybe there are some other edge cases in general. If this is not possible then maybe compiler could give some hint why type promotion didn't happen?

@leafpetersen
Copy link
Member

Interesting idea, worth considering in for future type promotion improvements.

@srawlins
Copy link
Member

I don't think this belongs to core-l; it's a language request, correct?

@tp
Copy link
tp commented Jun 6, 2019

I just ran into a similar issue:

abstract class I {
    int get value;
}

class X extends Route<dynamic> implements I {
  final value = 3;
}

void foo(Route a) {
    if (a is I) {
        print('${a.value}'); // Error: undefined_getter
    }
}

It also seem to be related to the fact that Route (from Flutter) has a generic type argument.

Is this the same underlying issue or should I file a new one?

@srawlins
Copy link
Member
srawlins commented Jun 6, 2019

Sorry, @tp I think this is unrelated.

In your example, Route is unrelated to I, so you the programmer know that the type of a, inside your if body, is Route & I, but there is no concrete type that represents that intersection, so no type promotion is performed. I think dart-lang/sdk#35314 is related to your request.

@jamesderlin
Copy link

I'm not sure if it's related, but a similar case:

class Foo {
  final int value = 42;
}

class Base<T> {}

class DerivedInt extends Base<int> {
  Foo get foo => Foo();
}

void f<T>(Base<T> base) {
  if (base is DerivedInt) {
    print((base as DerivedInt).foo.value); // Works

    // The getter 'foo' isn't defined for the class 'Base<T>'.
    print(base.foo.value);
  }
}

void main() {
  var derivedInt = DerivedInt();
  f<int>(derivedInt);
}

In this case, the is check is fully qualified. Why isn't the local variable base being promoted to DerivedInt?

@eernstg
Copy link
Member
eernstg commented Feb 20, 2021

You can only promote to a subtype, and DerivedInt is not a subtype of Base<T>. For instance, T could be double or Widget. Given that you're never using T you could use void f(Base<Object?> base), and DerivedInt <: Base<Object?> does hold.

@jamesderlin
Copy link

@eernstg Thanks for the explanation. I guess I have the wrong mental model for how promotion works. I had been presuming that a localVariable is T check would be syntactic sugar for an automatic localVariable as T cast. That is:

if (localVariable is T) {
  localVariable.f();
  localVariable.g();
}

would be equivalent to:

if (localVariable is T) {
  final temporary = localVariable as T;
  temporary.f();
  temporary.g();
}

So that raises a question: why shouldn't it be that way? It seems to me that it would be easier to understand and would work the way more people expect and want. Forcing people to add the extra cast is confusing, makes code less readable, and is more error-prone.

The "only promote to a subtype" rule also seems weird and arbitrary since I could just upcast to Object first:

void f<T>(Base<T> base) {
  Object object = base;
  if (object is DerivedInt) {
    print(object.foo.value);
  }
}

@eernstg
Copy link
Member
eernstg commented Feb 22, 2021

Promotion has been around in Dart at least since the language specification was added to the SDK repository in 2013, and it is required already in that 2013 version that the promotion changes the type of a local variable from a type S to a type T such that T <: S. This means that it would be a reversal of a very old decision to start promoting variables to anything other than a subtype of their previous type.

Promotion was made a lot more powerful in the version of Dart that supports null safety, but it still includes the requirement that the resulting type in a promotion is subtype related to the previous type. There is a list of 'types of interest' associated with the possible control flows up to the current location. We can promote to a subtype of the most recent one, and we can also demote to one of the earlier ones. Details here.

However, even though it would be a serious breaking change to promote to an unrelated type, it could still be a good design choice. So let's consider that design choice, as if we could just do it without worrying about existing software.

The rationale we've used to justify restrictions on promotions is that they make the code less readable: Anyone who is reading source code using local variables (that is, just about any function/method body which is more than a few lines of code) will need to take the type of each local variable into account, in order to understand why it was created, and how to use it correctly.

Consider a variable v with declared type T. As long as promotions will promote v from T to some type T1 <: T, then to T2 <: T1 and so on, the available members on v will be the ones on T plus a number of increments (some extras due to T1, some more due to T2, and so on). We may demote v by actions like v = T2(); along the way, but we are still only using v with a type which is navigating the subtype graph under T. In particular, if we do v.foo() where foo is a member of T then we can rely on the assumption that this method call "will do the kind of work that T.foo does".

If we allow the type of v to change to something completely unrelated then we could promote v to a type S such that S.foo exists, but it has no relationship to T.foo. So we can completely misinterpret the action taken at v.foo(), and we'll have to scan the control flow backwards (for instance, we may need to find all the if-statements encountered up to this point, and various other constructs) in order to detect that the promotion to S occurred.

In other words, this rationale is all about being able to read a small snippet of code in a function body and understand what it does, rather than having to scan the whole function body up to that point and memorize several completely different interfaces that the variable might have.

Of course, there have been many, many other discussions about the design of variable promotion in Dart, but this one seems particularly important to me.

@cedvdb
Copy link
cedvdb commented Dec 27, 2021

Might wanna move this to lang, I did not find this issue then posted to stackoverflow.

Edit: wrong link this one is correct,

In some cases it's worse for readability, but even more annoying to write

   if( state is ReadStarted<Location, List<Restaurant>>) {
      
   }

vs

   if( state is ReadStarted) {

   }

I was expecting the generic <Location, List<Restaurant>> to be inferred from what's on the left side of the is

That is:

fn(ReadState<Location, List<Restaurant>> readState) {
  if( state is ReadStarted) {
    // state is ReadState<Location, List<Restaurant>> 
  }

If you want dynamic you have to explicitely set it, but it would hardly ever make sens. I don't see why you wouldn't want that to be the case and that sounds more intuitive to me.

I cannot either forsee a scenario where you'd have to scan through a lot of code to find out the type. At that point the type is probably generic (in the last snippet it would have been fn(ReadState<I, O> readState) because it would be too far from a meaningful context. What I mean is that if the function is fn(ReadState<Location, List<Restaurant>> readState) then you are close to the context of a generic being Location, List<Restaurant>. I'm not sure how to articulate that last thought but it just seems logical. If it can be statically inferred at least then the context is probably not that far away is it ?

@eernstg eernstg transferred this issue from dart-lang/sdk Jan 4, 2022
@eernstg
Copy link
Member
eernstg commented Jan 4, 2022

Might wanna move this to lang

Agreed, and I just moved it.

posted to stackoverflow.

Said StackOverflow question is about the support for implicit downcasts in versions of Dart that came before null safety (with null safety, all downcasts except the ones from dynamic must be explicit). This issue is about having an inference step on type tests (that is, expressions of the form e is T), so that's a different topic.

Let's say that v is a promotable local variable and C is a raw type (that is, C is a name that denotes a generic class, and we're using it without passing any actual type arguments).

With the current rules, v is C simply tests whether the value of v has the type C, which could mean something like C<dynamic> or C<num, List<Object?>>—the precise rules are given as the algorithm instantiation to bound.

If we introduce support for the feature proposed here, we would transform v is C into v is C<T1, .. Tk>, in the case where there exist types T1 .. Tk such that C<T1, .. Tk> implements the current type of v.

In short, we'll infer missing type arguments in an is expression target type, when that enables a promotion.

The transformation is well defined, because the types T1 .. Tk are uniquely determined. This is basically a consequence of the fact that no class D<X1, .. Xn> can implement both E<U1, .. Um> and E<V1, .. Vm> where Uj and Vj are distinct types for any j, for any class E.

I think the feature looks convenient and useful. Just one counter argument comes to mind: We would need to settle the exact conditions for that transformation to trigger, and in particular we may or may not want to perform this kind of inference when the scrutinee (that is e in e is C) is not a promotable local variable. It could be a non-promotable local variable, and it could be a non-local variable, and it could be an arbitrary expression. It's possible that we would simply apply the transformation in all cases, and it's possible that we would only do it when the scrutinee is a promotable local variable (as stated above). In any case, it's yet another special rule that we must have in mind as developers, in order to be able to read Dart code correctly.

@jamesderlin
Copy link

posted to stackoverflow.

Said StackOverflow question is about the support for implicit downcasts in versions of Dart that came before null safety (with null safety, all downcasts except the ones from dynamic must be explicit). This issue is about having an inference step on type tests (that is, expressions of the form e is T), so that's a different topic.

I think #2047 (comment) used the wrong link and instead meant https://stackoverflow.com/q/70499209/.

@cedvdb
Copy link
cedvdb commented Jan 4, 2022

I have no idea how you found the correct one, but that's indeed the case, that stackoverflow post I posted was not from me.

in particular we may or may not want to perform this kind of inference when the scrutinee (that is e in e is C) is not a promotable local variable

I lost you there, I guess it means that you can't infer the generic type of e, in that case wouldn't it be dynamic ? I don't see that as counter intuitive, if the type cannot be inferred it's because it lacks context, then it's also dynamic in the eye of the developer too. I simply cannot think of a counter example.

I hope this is going to make it through though, I had to change a data model that fit reality in favor of another one where generics weren't as annoying yesterday.

@eernstg
Copy link
Member
eernstg commented Jan 4, 2022

OK, sorry about that, I actually considered briefly whether it was the right link! ;-)

@eernstg
Copy link
Member
eernstg commented Jan 4, 2022

I lost you there, I guess it means that you can't infer the generic type of e

Nono, it just means that e isn't a promotable local variable. For instance, it could be an instance variable, it could be an expression like a + b, or it could even be a local variable x which is assigned in a function literal:

void foo(Function f) {...}

void main() {
  num x;
  foo(() => x = 1.5); // Careful! "They" can now execute `x = 1.5` whenever they want!
  x = 1;
  x.isEven; // Error: `x` is not promotable, because it's written in the function literal.
}

The assignment x = 1.5 is assumed to occur, potentially, at every single step of the computation. We could insist that some operations are "statically known", but that breaks down very quickly. In particular, any instance member invocation would in general have an effect which is undecidable, so we just have to expect that essentially any computation can invoke that function literal. And that means that we can't trust x to have type int even though the control flow in main shows us that this is the most recent thing that happened to x — because x = 1.5 could also have occurred at any point.

@lrhn
Copy link
Member
lrhn commented Jan 5, 2022

One reason that Dart only allows promotion to a subtype is that it makes assignability understandable.

If we could "promote" a variable to an arbitrary type, unrelated to the declared type, then which types would then be assignable to that variable?

class C {}
abstract class I {}
class D extends C implements I {}
class J implements I {}
void main() {
  C c = D() as C;
  if (c is I) {
    // Assume we promoted `c` to type `I`.
    c = J();  // Can't be allowed, although `J()` implements `I` and `c` has type `I`. Confused?
    c = c; // Can .. probably be allowed, but the static type of `c` is `I` and `I` is not assignable to `C`, so ... ?
    c = D(); // Allowed. Should it demote `c` to `C`?
    c = C(); // Allowed. Should definitely demote to `C`.
  }
}

If c has the intersection type C&I (which it doesn't, because Dart doesn't have intersection types in its type system, but it would have the promotion "chain" of C>I), then we could allow c = c;.

When we promote a variable to a subtype of its declared type, then we also ensure that all values of the promoted type are assignable to the variable.

(But then, could we "promote" final variables to unrelated types? Probably yes, but then you would have no good way to demote them again, and get access to the original declared type's methods.)

The other reason is that promotion to unrelated types removes access to the original declared type's methods. That's highly confusing to users, so we don't do that. People really expect that promotion provides intersection types (C&I), but it doesn't and can't. We have to choose at most one of C and I if the two types are unrelated. We chose C instead of I.

That's also the reason you can just assign the value to an Object variable and then promote that: It's a different variable, you won't expect to be able to call C methods on it afterwards.

@cedvdb
Copy link
cedvdb commented Jan 5, 2022

The original request was about generics, the counter examples don't use generics

foo(ReadState<User> state) {
   if( state is ReadCompleted) {
     print(state.output);  // won't work today but should be inferred as ReadCompleted<User>
   }

@eernstg eernstg added small-feature A small feature which is relatively cheap to implement. inference labels Jan 5, 2022
@lrhn
Copy link
Member
lrhn commented Jan 6, 2022

ACK. The compiler is being a little obtuse about raw types in some (most) cases.

It also affects code like List l = <int>[]; where the compiler could infer List<int>. (Whether it should is a different questions, probably answered by "sometimes yes, sometimes no".)

The is check is more obvious because it allows promotion. We can even special-case the promotion only, so it works even if you write if (state is ReadCompleted<Object>). In that case we know that the code is ReadState<User> and ReadCompleted<Object>, which can only be true if it's actually ReadCompleted<User>, so we could choose to promote to that, whether using a raw type or not. (And not need to change how we treat raw types.)

The alternative is to just not promote, and I see no real risk in promoting at an is check. It's slightly curious that it promotes to a different type than what you write, but since it can't promote to what you wrote, it's the one option we have.

@eernstg
Copy link
Member
eernstg commented Jan 18, 2022

There have been some proposals about implicit provision of type arguments in a context which is not an expression (so it's a mechanism which is similar to type inference, but it is applied to type annotations and similar constructs).

This issue is concerned with one of them: Let e be an expression with static type S and let C be a raw type (a generic class that does not receive any type arguments). Then e is C would be transformed into e is C<T1, .. Tk> such that C<T1, .. Tk> <: S. We'd want to choose T1 .. Tk as general as possible (counterpoint: we could always let Tj == Never for all j, but then the type test would almost always fail at run time).

Another case is C x = e; where C is raw and e has static type S; in this case we could transform C into C<T1, .. Tk>, where T1 .. Tk would preferably be the most special types we can choose, such that we still have S <: C<T1, .. Tk>.

Note that the latter one gets complex when we also want to use C<T1, .. Tk> or C as the context type for e during type inference (which will transform e to e1 and hence give the initializing expression a new static type S1 which would then presumably be used to find a new value for T1 .. Tk, etc).

I think this illustrates that we need to keep the concept 'type inference' (or 'expression type inference') clearly separated from the concept 'type test inference' (e is C), which is again different from 'type annotation inference' (C x = e;), and we could surely come up with even more kinds of inference. For instance, <List>[[1], [2.0]] could be transformed into <List<num>>[<int>[1], <double>[2.0]], and List f<X>(X x) => [{x}]; could become List<Set<X>> f<X>(X x) ....

However, given that there are several different kinds of inference under consideration, and all of them (except expression type inference, which has been around for a long time) would be breaking changes as shown above, I'd prefer to have a syntactic marker which serves as a request to perform a specific kind of inference.

An obvious choice would be the empty type argument list, aka the "diamond operator".

Then we'd have e is C<>, C<> x = e;, <List<>>[[1], [2.0]], and List<> f<X>(X x) => [{x}];, and each of those different kinds of inference would only be performed when there is a <> to request it (except for expression type inference, which is also performed even when there is no <>, just like today).

@eernstg
Copy link
Member
eernstg commented Jun 20, 2024

tl;dr  Don't say x is B, say x case B()  rd;lt

We do have a workaround for this issue today. Here is the original example:

abstract class A<T> {}
abstract class B<T> implements A<T> {
  b();
}

void foo<T>(A<T> a) {
  if (a is B) {
    a.b(); // Error: 'b' isn't defined for the type 'A'.
  }
}

The problem is that B in the type test a is B is taken to be a fully specified type. It doesn't take any hints from the context. This means that the type arguments will be computed using the algorithm known as 'instantiation to bound', and that yields B<dynamic>. And then we don't get the promotion because B<dynamic> isn't a subtype of A<T>.

But we want to test for a is B<T>, we just don't want to write those actual type arguments (they might be verbose, and we might get them wrong).

Today we can obtain the requested behavior (that is, relevant type arguments of B are inferred implicitly):

abstract class A<T> {}
abstract class B<T> implements A<T> {
  b();
}

void foo<T>(A<T> a) {
  if (a case B()) {
    a.b(); // OK.
  }
}

The syntax B() in the if-case statement is an object pattern. Object patterns will have their type arguments inferred from the static type of the scrutinee (so B() means B<T>() in this case, because a has type A<T>, so if a is a B<S> for any S then it will also be a B<T>).

This won't work unless the syntactic context admits a pattern, that is: We can use an if-case statement if (a case B()) ... rather than if (a is B) ..., or we can use a switch (e.g., switch (a) { ... case B(): ... } rather than switch (a) {... case B _: ...}), but there are some cases where we can't replace the expression a is B by a construct that uses a pattern (for example, var aIsB = a is B; probably doesn't have a very direct equivalent form using a pattern).

It is probably a good idea to use this form (if (a case B()) ...) rather than the traditional type test (if (a is B) ...) as a habit: it will work both when B is a generic type and when it is a non-generic type, and if we always use the pattern form if possible then we don't have to double check whether the given type is generic.

Of course, the is form is still safe when type arguments are passed explicitly (if (a is B<T>) ...), or the target type is well-known as being non-generic (if (a is int) ...). You must specify the type arguments explicitly if they aren't guaranteed based on the static type of the scrutinee (e.g., if xs has type List<num> then if (xs is List<int>) ... can't be changed to if (xs case List()) ..., and there is no reason that I'm aware of to use if (xs case List<int>()) ... rather than if (xs is List<int>) ...). But using if (a case B()) ... rather than if (a is B) ... seems to be a good habit in most cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inference small-feature A small feature which is relatively cheap to implement.
Projects
None yet
Development

No branches or pull requests

8 participants