sdk
sdk copied to clipboard
dart-lang
Better subclass handling for `argument_type_not_assignable`
When we have a subclass (different type parameter) being used instead of the expected class on a parameter we have a bad error message compared to when we use the expected class.
Also, the current good error message is a bit redundant in the following example. I'm having the declaration path for Iterable twice on the error message (shortened it here to make the message more easy to see in the issue but in my case it is really big) and the actual problem is simply the type parameter for the elements.
Repro:
a.dart:
class A {
const A();
}
main.dart:
import 'a.dart' as a;
void main() {
final list = <a.A>{};
list.addAll(A.list);
list.addAll(A.iterable);
}
class A {
const A();
static List<A> get list => [
for (int i = 0; i < 10; i++) const A(),
];
static Iterable<A> get iterable => [
for (int i = 0; i < 10; i++) const A(),
];
}
On main.dart line 5 list.addAll(A.list), we get:
The argument type 'List<A>' can't be assigned to the parameter type 'Iterable<A>'. dart(argument_type_not_assignable)
On line 6 list.addAll(A.iterable) we get a way better error message:
The argument type 'Iterable<A> (where A is defined in .\bin\bug.dart)' can't be assigned to the parameter type 'Iterable<A> (where A is defined in .\bin\a.dart)'. dart(argument_type_not_assignable)
iterable.dart(92, 22): Iterable is defined in ...\dart-sdk\lib\core\iterable.dart
bug.dart(9, 7): A is defined in .\bin\bug.dart
iterable.dart(92, 22): Iterable is defined in ...\dart-sdk\lib\core\iterable.dart
a.dart(1, 7): A is defined in .\bin\a.dart
Summary: The error message for argument_type_not_assignable is inconsistent when a subclass is used instead of the expected class. The error message for the subclass case is more informative, but both messages are redundant, repeating the declaration path for Iterable twice.
@bwilkerson 's comment here I think applies to fixing this issue:
For anyone looking to fix this bug, there's already support for this kind of disambiguation when passing types in as the arguments to the
reportErrormethods.
@srawlins, should I open a new issue concerning typedefs, or is this one the right place for this ask?
The repro:
typedef MyBadlyNamedTypedef = int Function(int);
void foo(MyBadlyNamedTypedef f) {}
void bar() {
foo(baz);
}
String baz(int x) => x.toString();
The error message:
The argument type 'String Function(int)' can't be assigned to the parameter type 'MyBadlyNamedTypedef'.
To show the actual type somewhere, replace the typedef or show both with one inside () or something (preferred).
Take my example. If you have a badly named typedef or you are unfamiliar with what it means, this could be really weird.
Also, because of https://github.com/dart-lang/sdk/issues/57013, we still can't "Go to Type Definition" and see the typedef. So for Function types, to find out what it is you'd need to look at the definition for whatever you are trying to assign the Function, so you can see the typedef written down to find its definition (if it is a constructor you could also only see in the propriety definition some classes up the chain with lots of super and one this).
I think good to track with a separate issue. Especially because there might be discussion on the messaging. I think it'd be great to show both type aliases and un-aliased actual types. But because of the lengthy message that creates, I think we essentially never do that, and default to the un-aliased types (or maybe it's a mixed bag).
To show the actual type somewhere ...
That seems like a reasonable thing to do. I think we should show both.
But because of the lengthy message that creates ...
When we stick the URI of the type in messages to disambiguate them we end up with fairly long messages, but it's better than "Can't assign 'C' to 'C'." style messages.
The alternative that I could imaging is to put the extra information in a context message, something like "The typedef 'MyBadlyNamedTypedef' is equivalent to 'int Function(int)'."
Also, because of #57013, we still can't "Go to Type Definition" and see the typedef.
If the only reason to add this information to the message were because of the lack of navigation, I'd recommend that we fix the navigation and leave the message alone. But the message also gets displayed by the command-line analyzer, so I think that fixing that issue wouldn't impact this one.
Also, would this specific issue help with cases like the error message from https://github.com/dart-lang/sdk/issues/59663?
I got really confused with that message but I'm not sure this is the same ask. I think in that case it was trying to say that Bound from one of the types is null but not Bound from the other type?
typedef BoundedFunction<R, Bound> = R Function<T extends Bound>(T value);
class BoundedClass<R, Bound> {
const BoundedClass(this.run);
final BoundedFunction<R, Bound> run;
}
void main() {
final BoundedFunction<int, String> boundedFunction = <Y extends String>(Y value) => 1;
final BoundedClass<int, String> boundedClass = BoundedClass(boundedFunction);
}
Output when compiling (they think its a CFE error):
lib/fn.dart:13:63: Error: The argument type 'int Function<T extends String>(T)' can't be assigned to the parameter type 'int Function<T extends String>(T)' because 'T' is nullable and 'T' isn't.
final BoundedClass<int, String> boundedClass = BoundedClass(boundedFunction);
^
Also, would this specific issue help with cases like the error message from #59663?
The hypothesis expressed in that issue is that it's a CFE bug. The CFE and the analyzer have different logic related to the production of diagnostics, so it seems unlikely that the two issues are related in terms of implementation.
Note that invalid_assignment probably has the same issue. We should probably check all error messages which include multiple types because the problem can arise in any of them.
Thanks! I'll be sure to take a look at other diagnostics too.
I plan to work on this in the coming weeks (maybe this one if I find the time). If anyone else wants to work on this sooner, please let me know so we avoid doing the same work twice. Thanks!
The example from @mraleph pointed out a couple of other issues that should be fixed, possibly at the same time. The first example involved two types List<A> and List<A> where one couldn't be assigned to the other because the As referred to different classes. The second had Iterable<A> for the second type.
In oth cases the CFE very nicely prints dart:core as the location of the definition of List rather than the path to where the SDK lives on the user's machine. Given that users can't edit the file, showing the full path of the file probably isn't helpful. The same is probably true of some declarations from other packages, but probably not all declarations from other packages, so changing the display of the package-defined types should probably wait for a follow-on CL.
In the first case, the analyzer produces four context messages, one for each of the two classes named A and two for the single class named List. It would be nice to avoid the duplication of the pointers to where List is declared.
In both cases, it would be really nice if the message could be (using the types from the second case) something like "The type 'Iterable<A>' can't be assigned to the type 'List<A>' because the type arguments refer to two different types despite having the same name." (with context messages for the two types that differ). I think that would more clearly articulate the problem and would allow us to drop the information about the locations of List and Iterable.
The context messages for the types with the same name should do a better job of saying which of the two types uses that particular type. We can't just say "The type A from List<A> is defined ..." because the enclosing type isn't guaranteed to be unique. I don't have a good suggestion at the moment for how to cleanly differentiate the two.