dart-lang / sdk

The Dart SDK, including the VM, dart2js, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
9.99k stars 1.54k forks source link

Analyzer incorrectly applies an extension on a type-variable-typed expression, derived from an intersection type #53711

Open srawlins opened 9 months ago

srawlins commented 9 months ago

From @FMorschel at https://github.com/dart-lang/sdk/issues/52077:

void main() {}

class Filter<T extends Filterable?, E extends Object?> {
  const Filter({
    required this.column,
    required this.filter,
  });

  final int column;
  final List<E> filter;

  bool test(T object) {
    if (object == null) return false;
    return filter.map(_constEmptyList).contains(
          object
              .when(_notEmptyList, otherwise: (_) => const [] as T)!
              .column(column),
        );
  }

  bool _notEmptyList(T self) {
    return (self is! List) || ((self as List).isNotEmpty);
  }

  R _constEmptyList<R>(R e) {
    return ((e is List) && e.isEmpty) ? const [] as R : e;
  }
}

mixin Filterable {
  T column<T extends Object?>(int index);
}

extension _When<T extends Filterable> on T {
  T? when(bool Function(T self) predicate, {T Function(T self)? otherwise}) {
    if (predicate(this)) return this;
    return otherwise?.call(this);
  }
}

@lrhn comments:

As commented in https://github.com/dart-lang/sdk/issues/56028, this is an analyzer bug, as things are currently specified (at least based on the extension method feature specification, haven't checked the language spec integration of that).

The type inference applied to object.when should behave the same as other inferences where a type argument is found from an intersection type, which means falling back to the type variable. That type variable is not a valid type argument to the extension, which means the extension is not applicable, and therefore the front-end behavior of saying "no when method declared" is correct.

(The error message is confusing, it should say, Error: The method 'when' isn't defined for the type 'T & Filterable'. Bonus points for saying that the extension WhenWidget does not apply, because T is not a valid type argument to it.)

Originally posted by @lrhn in https://github.com/dart-lang/sdk/issues/52077#issuecomment-1516211666

leafpetersen commented 2 months ago

Noted in passing that @srujzs just ran into this bug.

srujzs commented 2 months ago

A few more details that may or may not be useful:

extension E<T extends Object> on T {
  ExtType<T> get toExtType =>
      ExtType<T>._(this);
}

extension type ExtType<T extends Object>._(T t) {}

class C<T>  {
  void consumeExtType(ExtType e) {}

  void consumeT(T t) {
    if (t != null) {
      consumeExtType(t.toExtType);
    }
  }
}

Maybe displaying the intersection type makes it easier to understand than the current CFE error, but maybe that's even more confusing.

eernstg commented 2 months ago

We do have https://github.com/dart-lang/sdk/issues/56028, which is intended to elicit an answer to the underlying question.

That is, "during extension method resolution on e.m() where e has type X & B, do we consider the receiver e to have type X or type B when checking whether or not the receiver type is a match for the on type of a given extension?"

In this comment, I'm suggesting that the answer should be "B".

The main motivation is that B has an interface which is at least as wide as the interface of the bound of X (because otherwise the type X & B wouldn't arise). This property carries over to extension members because having type X will be a match for at most the same extension on types that B matches (except perhaps some cases where the extension has one or more F-bounded type parameters).

The main argument against this choice is that the return type of the extension member invocation, when based on a receiver type of B, might be less useful than a return type which is allowed to use T:

extension<X> on X {
  X get show {
    print(this);
    return this;
  }
}

extension on int {
  void get bar => 0;
}

void f<Y extends num>(Y y) {
  if (y is int) {
    // `y` has type `Y & int`.
    Y y2 = y;
    int i = y;
    // If `y` has type `int` for extension matching
    // then we do get `int` based extension members.
    y.bar;
    // We have `int` instance members, too, of course.
    y.isEven;
    // But if `y` has type `int` then we can't do this,
    // because `y.show` has type `int`, too.
    y2 = y.show;
  }
}

It would be nice to have all the affordances based on each of the operands of an intersection type, but we do have to choose an specific reified type to pass to the extension as the actual type argument at run time, and it isn't sound to use anything other than the type argument value which is actually passed when computing the type of the returned object.

In any case, I think this issue is blocked on https://github.com/dart-lang/sdk/issues/56028.

FMorschel commented 2 months ago

A few more details that may or may not be useful:

  • I came across this error with code that looks like:
extension E<T extends Object> on T {
  ExtType<T> get toExtType =>
      ExtType<T>._(this);
}

extension type ExtType<T extends Object>._(T t) {}

class C<T>  {
  void consumeExtType(ExtType e) {}

  void consumeT(T t) {
    if (t != null) {
      consumeExtType(t.toExtType);
    }
  }
}

Hi! Some thoughts on this today, and through testing, just to help to narrow the problem. If you take the T definition in C and change it to T extends Object it all works just fine.

But interestingly, adding ? to T extends Object? makes it warn The argument type 'ExtType<T>?' can't be assigned to the parameter type 'ExtType<Object>'. even with the null test neither by using ?..

Edit

Am I sure to guess that all parameter types with definitions like <T> are considered dynamic? Is this documented somewhere? I'm unfamiliar.

srujzs commented 1 month ago

Right, if both Ts match in terms of bounds, then the choice of T in the intersection type T & Object works fine for extension resolution. You don't even need to do the null-check in that case, and therefore won't even come across the intersection type.

Making it extend Object? is the same thing as a T without a bound, so we come across the same issue. Object? being the default bound is documented here: https://dart.dev/language/generics#restricting-the-parameterized-type.

FMorschel commented 1 month ago

Thanks for the docs, but I'm not completely sure they're right. Or at least that doesn't seem to be the case If you look at @eernstg comment at https://github.com/dart-lang/sdk/issues/52117#issuecomment-2178741302 (this is a snippet of that comment):

[...]

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)'. 

[...]

It seems to me that it is resulting R in the apply function as dynamic and not Object?. I'm not sure this is simply to show the user an easy to understand message but I don't think that's the case.

lrhn commented 1 month ago

(Notice that this is not an extension, so the rules are not at all in question.)

The inference of Apply(object) should make it Apply<T>(object) from the static type of object being T&DateTime. That's how intersection type erasure works.

Then inference of Apply<T>(object).apply(formatter.format) tries to infer R from the argument, giving the argument expression a context type of R Function(T) with R not solved yet.

It tries to find constraints for that based on the initial constraints of String Function(DateTime) <: R Function(T). Since String Function(DateTime) is not a valid argument for R Function(T) for any R, because the parameter type doesn't match, that's as far as it comes. It doesn't have a type for R when it reports that, so maybe it instantiates R to bounds to get dynamic before showing the error. (Or maybe it takes the greatest closure of R Function(T) wrt. R before even trying to solve anything, to report the error, which has the same effect).

eernstg commented 1 month ago

That's how intersection type erasure works.

I think that's exactly what we're clarifying right here, and elsewhere. ;-)

In particular, perhaps T & Datetime should to be erased to T before we start checking whether the extension Apply is applicable to this call site, in which case we'd get "no" because the type inference that turns Apply(object) into Apply<S>(object) for some type S fails. Hence, object.apply... can't be an invocation of Apply.apply.

It follows that we have no idea whether formatter.format is a type correct actual argument, or even whether it is correct to have an actual argument list of that shape (just like unknownName(formatter.format)—we can't even get started deciding whether or not that's a statically correct actual argument list, perhaps unknownName isn't even a function, or it is a function but it doesn't accept any positional arguments, etc).

lrhn commented 1 month ago

perhaps T & Datetime should to be erased to T before we start checking whether the extension Apply is applicable to this call site

No "perhaps" needed, that's how it's specified. Given:

extension Apply<X> on X {
  R apply<R>(R Function(X) function) => function(this);
}
void foo<T>(T value) {
  if (foo is DateTime) {
    String text = foo.apply((DateTime value) => value.toString());
  }
}

the applicability check (after seeing that it has an apply member at all) requires first doing inference equivalent to a constructor call of Apply(object) for a class

class Apply<T> {
  Apply(T value);
}

with no context type and object having the static type we already found.

If we can solving that for binding to the extension's type parameters, and then find that the instantiated on type is a supertype of the static type of the receiver object, then the extension is applicable.

That inference and instantiation of type parameters must erase T & DateTime to T because it is used as a type argument. That erasure must happen before doing the subtype check, because we do the subtype check using the instantiated type variables, which cannot possibly be an intersection type. We must instantiate the on type X with a type argument, inferred like we would for the Apply class above, which means X = T.

The instantiated on type is T, which is a supertype of T, so the extension applies, which means that the object.apply(...) is inferred to work like Apply<T>(object).apply(...).

Then that invocation is rejected because String Function(DateTime) is not an R Function(T) for any R.

eernstg commented 1 month ago

the applicability check (after seeing that it has an apply member at all) requires first doing inference equivalent to a constructor call of Apply(object) for a class

Sure, that is exactly the reason why my example mentioned here contains a class Apply rather than an extension Apply.

This, however, does not prevent us from using the intersection type during type inference, as long as the final result is a list of actual type arguments that satisfy all the constraints. Here is an example:

X bar<X extends int>(X x1, X x2) {
  print(x1.isEven);
  return x1;
}

void foo<Y>(Y y) {
  if (y is int) { // `y` has type `Y & int`.
    // This invocation is inferred as `bar<int>(y, 24)`.
    bar(y, 24);
  }
}

void main() => foo<Object>(42);

For the invocation of bar we get the constraints that the actual type argument must be a supertype of Y & int and a supertype of int, and the bound requires it to be a subtype of int as well. Type inference selects int as the solution, which works fine and is sound.

If we erase the type of y to Y already before we start performing type inference then there is no solution (no type is a supertype of Y and a supertype of int and a subtype of int).

So we can do that, but (1) it would be less powerful, and (2) the CFE doesn't do it currently, and it works correctly as far as I can see.

I think this implies that we should not erase the intersection type before we start inferring the actual type arguments to the extension. But we must check that the required erasure at the end doesn't introduce any soundness issues.