Analyzer performs incorrect type substitution when resolving call to generic method in a generic extension

[FMorschel]

Describe the issue
I have a function with a generic type T extends Object?. Somewhere in the middle of my function, I have a test to see if my variable T object is DateTime?. I'm not sure how to describe this better than showing you the code (it can be tested with dartpad.dev):

To Reproduce

import 'package:intl/intl.dart';

/// This is an extension method so I don't always have to test for nullable values*
extension Apply<T extends Object> on T {
  R apply<R>(R Function(T self) transform) => transform(this);
}

void func<T extends Object?>(T object) {
  if (object is DateTime?) {
    final formatter = DateFormat('yyyy/MM/dd');
    object?.day; // No lint
    if (object != null) {
      formatter.format(object); // No lint
    }
    object?.apply(formatter.format); // The argument type 'String Function(DateTime)' can't be assigned to the parameter type 'dynamic Function(T)'.
    (object as DateTime?)?.apply(formatter.format); // Unnecessary cast.
  }
}

*Here is where I found it

Expected behavior
Either not triggering unnecessary_cast or not triggering the error with the line above as all the other code before works just fine.

[srawlins]

I am more surprised about the error at object?.apply(formatter.format);. CC @scheglov

I'm not sure why inference isn't setting T to DateTime there. object is confirmed to be promoted to DateTime? as per the lines like object?.day, and the receiver of object?.apply should be promoted to DateTime, I think...

[FMorschel]

I am more surprised about the error at object?.apply(formatter.format);. CC @scheglov

I'm not sure why inference isn't setting T to DateTime there. object is confirmed to be promoted to DateTime? as per the lines like object?.day, and the receiver of object?.apply should be promoted to DateTime, I think...

I believe that too. I wasn't sure how to reference that error for me to name this issue, but at least I figured it was a problem with the lint.

[scheglov]

Yeah, type inference is weird in this invocation.
See below what we have currently, but don't consider it to be the right expectation.
Apparently we don't infer R correctly.

  solo_test_X() async {
    await assertErrorsInCode(r'''
void f<T extends Object?>(T object) {
  if (object is int?) {
    object?.apply(g);
  }
}

extension E<T2 extends Object> on T2 {
  void apply<R>(R Function(T2 self) transform) {}
}

double g(int a) => 0.0;
''', [
      error(CompileTimeErrorCode.ARGUMENT_TYPE_NOT_ASSIGNABLE, 80, 1),
    ]);

    final node = findNode.singleMethodInvocation;
    assertResolvedNodeText(node, r'''
MethodInvocation
  target: SimpleIdentifier
    token: object
    staticElement: self::@function::f::@parameter::object
    staticType: T & int?
  operator: ?.
  methodName: SimpleIdentifier
    token: apply
    staticElement: MethodMember
      base: self::@extension::E::@method::apply
      substitution: {T2: T, R: R}
    staticType: void Function<R>(R Function(T))
  argumentList: ArgumentList
    leftParenthesis: (
    arguments
      SimpleIdentifier
        token: g
        parameter: ParameterMember
          base: root::@parameter::transform
          substitution: {R: dynamic}
        staticElement: self::@function::g
        staticType: double Function(int)
    rightParenthesis: )
  staticInvokeType: void Function(dynamic Function(T))
  staticType: void
  typeArgumentTypes
    dynamic
''');
  }

[srawlins]

Looks like the same bug as: #35799

[FMorschel]

Please give a look at this comment on dart-lang/sdk#52077

[pq]

Moving this one to the SDK tracker since UNNECESSARY_CAST is an analyzer warning code.

[FMorschel]

Has this been fixed? Asking because of #53828 (comment)

[srawlins]

It's possible. I'm not sure. I haven't specifically reproduced this one.

[FMorschel]

To help a little, I've tested it myself in the latest master, and the unnecessary_cast is not triggering anymore.

[stereotype441]

Thanks for retesting in the latest master, @FMorschel! I'm glad that the issue isn't triggering for you anymore.

I'm sorry to say that sometimes this sort of thing happens; we don't get around to fully investigating an issue before something else changes and the issue stops occurring. When this happens, it can be difficult to dig through the project history to figure out whether the bug was fixed on purpose (say, because someone else ran into it in a different form, and their issue got addressed), or whether it just kind of "went into hiding" because something else changed. Sometimes I go ahead and dig anyhow because I'm curious, or because I'm worried that maybe there's a deeper problem that still hasn't been solved. But we're all busy, so more often than not what happens at this point is that we close the issue and move on to other things.

In this particular case, I think it would be worth doing a bit more investigation, in particular into @scheglov's comment saying that we don't infer R correctly.

If I'm understanding that comment correctly, @scheglov is referring to this line from his repro of the issue:

       substitution: {T2: T, R: R}

Which suggests that when applying the extension apply, the analyzer is substituting a type of R for R. But R isn't a well-formed type at the point where apply is used; it's not even in scope:

void f<T extends Object?>(T object) {
  if (object is int?) {
    object?.apply(g);
  }
}

That seems like a problem that could potentially underlie a lot of bugs.

And just because this unnecessary_cast false positive isn't happening anymore, doesn't necessarily mean that the problem with R isn't still occurring. In fact, I suspect that the problem with R and the unnecessary cast might be unrelated, because the type parameter R only affects type checking at g, whereas the unnecessary cast had to do with the target of the extension method, object.

@scheglov, do you think you could repeat your experiment and see if R is still being substituted with an ill-formed type?

[scheglov]

The test in #52117 (comment) still produces the same resolution, with substitution: {T2: T, R: R}.

[FMorschel]

just because this unnecessary_cast false positive isn't happening anymore, doesn't necessarily mean that the problem with R isn't still occurring

If you then think renaming this issue to make sure we are clear about solving this substitution problem would be better (since the unnecessary_cast false positive isn't happening anymore), then by all means, do so. I don't feel like I understand enough of the problem to make a good issue title.

[FMorschel]

Sorry for the ping, @stereotype441, but I just wanted to know if you ever saw my last comment about renaming this. Or we could even open a new issue to fix that substitution problem and differ from this. I've not tested it again so that might be a good thing to do just to make sure it is still happening.

[stereotype441]

Sorry for the ping, @stereotype441, but I just wanted to know if you ever saw my last comment about renaming this. Or we could even open a new issue to fix that substitution problem and differ from this. I've not tested it again so that might be a good thing to do just to make sure it is still happening.

Good point, thanks! I've re-titled the issue.

[eernstg]

I think the core of this issue is when and how an intersection type is erased to a reifiable type.

As noted in #56028, actual type arguments for an extension in an extension member invocation (that is, for the extension itself, not for the member) are computed in the same way as the actual type arguments of a constructor invocation with a corresponding class. So we should get the same outcome (in particular, the same error) in the original example here as we do with the following example:

import 'package:intl/intl.dart';

class Apply<T extends Object> {
  final T t;
  Apply(this.t);
  R apply<R>(R Function(T self) transform) => transform(t);
}

void func<T extends Object?>(T object) {
  if (object is DateTime?) {
    final formatter = DateFormat('yyyy/MM/dd');
    if (object != null) {
      // Type of `object` is `T & DateTime`, same as receiver type with `object?.apply(...)`.
      Apply(object).apply(formatter.format); // Error.
    }
  }
}

This example yields the following error message at // Error.:

The argument type 'String Function(DateTime)' can't be assigned to the parameter type 'dynamic Function(T)'. 

which is exactly the same error message that we get with the extension member invocation in the original example. However, I would in both cases have expected the error message to acknowledge that the type inference step does not produce a binding for the given type variable that satisfies the constraints. Just like this example:

void f<X extends num>(X x) {}
void main() => f('Hello'); // Error: "Couldn't infer type parameter 'X'".

Considering that the developer did not choose the given value for the type variable T (in the Apply examples), it seems unreasonable to complain that the chosen value doesn't work.

For the extension example, the situation where type inference fails implies that the given extension isn't applicable. So we'd get an error specifying that there is no such method. In any case, it looks like the type inference failure does not get properly detected:

It is surprising that the parameter type in the Apply example is described as dynamic Function(T), because this implies that .apply(...) is processed during static analysis under the assumption that the receiver has type Apply<T>, in spite of the fact that T does not satisfy the requirement on actual type arguments to Apply (because it isn't known to be a subtype of Object).

This seems to imply that the Apply example proceeds to accept a receiver type of Apply<T> even though that type is a compile-time error, possibly because it's really Apply<T & DateTime> (which is a malformed type because T & DateTime cannot be reified, but it would satisfy the subtyping constraints if it hadn't been considered malformed). Similarly for the example where Apply is an extension.

So, no matter what's the ultimate resolution of this issue, I think it should not be possible for any static analysis steps to consider the receiver type of Apply(object).apply... to be Apply<T>, and then proceed to reach any derived conclusions based on that typing. Instead, Apply(object) should be flagged as having a failed type inference, and subsequent steps should then treat Apply(object) as having an invalid type, or a type that arises from passing some actual type arguments that do satisfy the bounds, etc., whatever is usually done in the situation where type inference fails on an expression which is used as a receiver.

(Of course, the intersection type could also be erased to DateTime rather than T, and the type inference could then succeed, but that's ... presumably ... a much bigger change, which would be a separate language enhancement proposal.)

[eernstg]

tl;dr It should say "isn't defined for the type" rd;lt

Coming back to this issue, I can see that it may not be obvious how to characterize the observed behavior, or what to do next. So here's a shorter version of what I tried to say in the previous comment. ;-)

Here is the core of the example program again, simplified a bit and modified to invoke the extension method with . rather than ?. as well:

extension Apply<T extends Object> on T {
  R apply<R>(R Function(T self) transform) => transform(this);
}

Object? f(DateTime _) => null;

void func<T extends Object?>(T object) {
  if (object is DateTime?) {
    object?.apply(f); // ... type can't be assigned ...
    if (object != null) {
      object.apply(f); // ... type can't be assigned ...
    }
  }
}

The observed behavior of the analyzer is understandable in the sense that object?.apply(f) should give rise to a failure as stated in the actual error message: "The argument type 'String Function(DateTime)' can't be assigned to the parameter type 'dynamic Function(T)'", given that the type parameter T of the extension Apply has been bound to T extends Object? (which is declared by func).

However, we should never have reached that point because Apply is not an applicable extension for this invocation because that actual type argument fails to satisfy the declared bound.

I think the fix could be to erase the receiver type from T & Datetime to T during the step where extension applicability is checked. We would then determine that no extensions are applicable, and .apply is simply an unknown member name.

Fun fact: If we force that outcome by using a different member name then we get the following:

The method 'notApply' isn't defined for the type '<unknown>'.

Shouldn't that be a specific type rather than '<unknown>'?