Incorrect extension method resolution with generics

[leafpetersen]

Filing this as a language issue first to ensure that I'm not missing something. The analyzer and the CFE both consistently resolve extensions applied to things of generic type in a way that appears completely incorrect to me. Specifically, given the program below, both implementations instantiate T to be num rather than S.

As best I can see, this is completely inconsistent with both the specification and my expectation. This seems to be the released behavior however, on both platforms. Am I confused, or missing something?

cc @eernstg @lrhn @johnniwinther @stereotype441 @scheglov

extension Test<T> on T {
  T Function(T) get test => (a) => this;
}

class Foo<S extends num> {
  void test1(S x) {
    S Function(S) f = x.test;
  }
}

void main() {
}

[johnniwinther]

I agree. I recently filed dart-lang/sdk#43177 which is the same issue.

It is a breaking change to fix it, though. I fixed it in https://dart-review.googlesource.com/c/sdk/+/155324 but had to revert in https://dart-review.googlesource.com/c/sdk/+/156195 because of failures in code matching the patterns in dart-lang/sdk#43177.

[lrhn]

I agree that it's a bug. The static type of x is S, so that's what T should be bound to.

Changing it to

    var tmp = x.test;
    S Function(S) f = tmp;

makes no difference (reasonably, a receiver has no type context).

If you instead (or additionally) write

    S Function(S) g = Test(x).test;
    S Function(S) h = Test<S>(x).test;

there is no error in either of those, but that's exactly what x.test should do.

@johnniwinther dart-lang/sdk#43177 is slightly different because it's about promoted type variables. You can't represent T&Foo as a type argument, but you can use T-with-bound-Foo as a type argument.

[eernstg]

I think it's surprising that both the CFE and the analyzer treat the example in this way. It is specified (in the feature spec as well as in the language-specification-integrated text) that type inference for the extension application Test(x) should be performed with no context type and as if Test had been a class like this:

class Test<T> {
  final T _this;
  Test(this._this);
}

@johnniwinther, @scheglov it seems like that approach is incompatible with the steps actually taken by the implementation?

[lrhn]

Johnni explained the likely reason for the bug: You need to use the bound/intersection type in order to check whether .test is an instance method. Then, when it isn't, you need to go back to the original type variable/intersection and use that as the type argument to the extension. Forgetting to do that, and carrying on with the bound/intersection type gives the erroneous behavior we see here, and apparently that easy-to-make mistake happened twice.

[johnniwinther]

@lrhn #43177 is not only about promoted type variables - it's about bounded type variables - this can be from declaration (as here) or from promotion (as in examples 1 and 3 in #43177).

[leafpetersen]

Ok, it sounds like this is something we'll have to manage as a breaking bug fix. @johnniwinther @scheglov any thoughts on rolling this out as part of null safety, vs as a flat out breaking change? Sounds like @johnniwinther encountered actual breakage from an attempted fix?

[scheglov]

Yes, the analyzer has the same issue - we resolve the type to its bounds to search for an interface member, and then continue using the resolved type to infer type arguments extensions. I will try to fix it in the analyzer tomorrow, run shared tests and google3.

[scheglov]

CL for the analyzer: https://dart-review.googlesource.com/c/sdk/+/160883
It looks green, I will try in google3 next.

[scheglov]

It almost works in google3, mostly have to add // ignore:unnecessary_cast.
https://test.corp.google.com/ui#id=OCL:329025715:BASE:329038619:1598661188027:c37575a9

Just one target is broken, like this:

// @dart = 2.8

class A {}

class B extends A {}

extension E<T> on T {
  T get test => this;
}

void g(B _) {}

void f<U extends A>(U u) {
  g(u.test);
}

What used to work because of implicit downcast from A to B does not work anymore because there is no such cast possible from U to B. Which looks like exposing a potential issue in the code, e.g. what if we called f(new C()) where class C extends A {}?

@leafpetersen The change to resolve type parameters to bounds in the analyzer is fairly limited, and could be done depending on whether the library being analyzer in Null Safe or not.

[leafpetersen]

It almost works in google3, mostly have to add // ignore:unnecessary_cast.

Yeah, I suspected this is why we hadn't noticed this yet.

I'm inclined to roll this out under the null safety flag if it's easy to do, just to avoid the overhead of the breaking change. @johnniwinther is that easy enough to do on your end? If not it's fine, I'll just file a breaking change request, I don't anticipate any concerns given the limited breakage.

[johnniwinther]

It can easily be under null safety in the CFE.

[leafpetersen]

Implementation issues:

• Analyzer
• CFE

@lrhn or @eernstg could one of you add a language test for this?

[eernstg]

Sure, I'll take a look. Proposed test: https://dart-review.googlesource.com/c/sdk/+/165022.

[eernstg]

Tests landed in dart-lang/sdk@cd280a3.

[scheglov]

I see that tests expect that we erase / demote type parameters.

Could you point me at the place where the spec describes this?
Do we do such erase / demote operation for all inference operations?

Currently in the analyzer we don't.
For example this code has not errors in the analyzer, but is an error in CFE.

class A<T> {
  A(T t);
  T get foo => throw 0;
}

void f<S>(S s) {
  if (s is int) {
    s.isEven;
    var bar = A(s).foo;
    bar.isEven;
  }
}

[eernstg]

Edit: The applicable rule in inference.md is

- If `P` is a type variable `X` in `L`, then the match holds:
  - Under constraint `_ <: X <: Q`.

so S & int <# T yields { S & int <: T <: _ }. With that, there is a need to erase S & int: T cannot be an intersection type, because they do not have a run-time representation, and it isn't sound to perform the static analysis with S & int as the actual type argument, and then run the program with S as the actual type argument.

X f<X>(X x) => "Hello!" as X;

int g<S>(S s) {
  if (s is int) {
    return f(s);
  } else {
    return 1;
  }
}

void main() {
  int y = g<Object>(42);
  y.isEven;
}

[leafpetersen]

@scheglov I think the issue you are raising is that we don't specify when intersection types get erased. @eernstg and I spent a bit of time discussing this, but have a few more things to explore before I follow up with a spec update. In the meantime, I noted that the analyzer behaves differently with your example depending on whether NNBD is on or off. In the former case it infers S for bar, in the latter case it infers int. Can you clarify where the difference is coming from in the analyzer?

@stefantsov do you know where the CFE does the erasure of intersections?

[johnniwinther]

The CFE erasure intersection through the demoteTypeInLibrary function.

This is called on an inferred type before store it as local function return types, local function parameter types, local variables types, field types, and type argument. I.e. in (hopefully) all cases where an inferred type is reified.

[scheglov]

With null safety the type of A(s).foo is S & int, and as requested in dart-lang/sdk#40413 we do demotion, so the inferred type of foo is S, but it is immediately promoted to S & int.

With legacy the type of A(s).foo is S & int, and we still do demotion, so the inferred type of foo is S, and because in legacy we don't do promotion on assignment, the type of foo stays S.

In the analyzer do demotion only when infer the type of a variable. Should we do more?

[eernstg]

@scheglov wrote:

the type of A(s).foo is S & int

This implies that the intersection is not erased before it is passed as the actual type argument to the constructor A, and that would give rise to the soundness problem that I mentioned here when the type argument passed at run time is S.

[scheglov]

https://dart-review.googlesource.com/c/sdk/+/165683 will also demote inferred type arguments, for constructor or function invocations.

[leafpetersen]

This has been fixed.