Skip to content

NNBD: Default type bound of the generic type parameter is incorrect in analyzer: it's dynamic instead of Object? #40368

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

Closed
iarkh opened this issue Jan 29, 2020 · 7 comments
Assignees
Labels
legacy-area-analyzer Use area-devexp instead. NNBD Issues related to NNBD Release

Comments

@iarkh
Copy link
Contributor

iarkh commented Jan 29, 2020

Dart VM version: 2.8.0-edge.40f23c735f04433e4fc334fbd674474bd3de0f8b (Tue Jan 28 01:14:48 2020 +0000) on "linux_x64"

This bug is similar with #40367, but this one against analyzer, and #40367 is against runtime.

NNBD Spec reads:

The default bound of generic type parameters is treated as Object?.
However, currently this is not so and analyzer treats it as dynamic.

Please run analyzer with the following code:

F<X>? toF<X>(X x) => null;

class A<T> {}

main() {
  A? source;
  var fsource = toF(source);
  F<A<Object?>> target = fsource;
}

It should pass, however actually it fails with compile time error:

iarkh@med$ dartanalyzer --enable-experiment=non-nullable test.dart
Analyzing test.dart...
error • A value of type 'void Function<Y extends A?>()?' can't be assigned to a variable of type 'void Function<Y extends A<Object?>>()'. • test.dart:8:26 • invalid_assignment
hint • The value of the local variable 'target' isn't used. • test.dart:8:17 • unused_local_variable
1 error and 1 hint found.

@a-siva a-siva added legacy-area-analyzer Use area-devexp instead. NNBD Issues related to NNBD Release labels Jan 29, 2020
@scheglov scheglov self-assigned this Jan 29, 2020
@scheglov scheglov added the needs-info We need additional information from the issue author (auto-closed after 14 days if no response) label Jan 29, 2020
@scheglov
Copy link
Contributor

@leafpetersen My reading of the spec(s) is that analyzer does it correctly.

Yes, the bound of a type parameter in NNBD is Object?, just like it is Object in legacy libraries. However just like in legacy libraries List x; is List<dynamic> x;, in NNBD libraries it is also List<dynamic> x;. It is inside the class with this type variable is where we see it as Object or Object?, outside it is dynamic if not specified.

image
I don't see any changes in instantiation to bound in the NNBD spec.

@leafpetersen
Copy link
Member

@scheglov is correct. Also to the point, the error you are seeing has nothing to do with the difference between dynamic and Object?. The error you are getting is because you are trying to assign a nullable value (fsource which has type F<A?>?) to a non-nullable value (target, which has type F<A<Object?>>. Note that extra ? on the outside of the type of fsource.

@eernstg
Copy link
Member

eernstg commented Jan 31, 2020

Here's another comment on the approach. The following used to work when we have some declaration that introduces a generic type named 'A' and we want to test that the result of i2b on the raw type A is a specific type:

// Example declaration of a type which is to be tested.
class A<X extends num> {}

// Standard convenience definitions for this kind of test.
typedef F<X> = void Function<Y extends X>();
F<X> toF<X>(X x) => null;

main() {
  A source; // Use the target type as a raw type.
  var fsource = toF(source); // Type of `fsource` contains `A` invariantly.
  F<A<num>> target = fsource;
}

With nnbd, this template needs to be updated. In order to avoid involving more complex types it is probably useful to change source such that it is a formal parameter rather than a local variable. This means that we don't have to consider making the type annotation of source nullable, which would prevent testing i2b on a type like A?, or making the variable late, which would cause a dynamic error at run-time (catching that error would just make the code more verbose and less readable).

But we don't have to execute the code involving source/fsource/target, because it's concerned with static types only, so moving that code to a separate function and just ensuring that it gets compiled would be fine:

// Example declaration of a type which is to be tested.
class A<X extends num?> {}

// Standard convenience definitions for this kind of test.
typedef F<X> = void Function<Y extends X>();
F<X> toF<X>(X x) => throw 0;

void testA(A source) {
  var fsource = toF(source);
  F<A<num?>> _ = fsource;
}

main() {
  // Ensure that `testA` will be compiled, but not executed.
  print(testA);
}

We do not require that any tool must analyze/compile dead code, so a robust test which does not expect a compile-time error would need to have something like print(testA) in main in order to ensure that testA gets compiled/analyzed at all. I'm using a statement with side-effects rather than, e.g., var _ = testA; because an optimizer could eliminate the local variable, and then testA might not be compiled.

We need to avoid executing testA because it calls toF which always throws, and it would just clutter the code to catch that. Also, it is convenient to avoid calling testA because then we don't have to provide an actual object whose type is the type under test (so we can test an abstract class without writing some dummy concrete subtype just because that's needed in order to get hold of such an instance, etc).

@iarkh
Copy link
Contributor Author

iarkh commented Feb 24, 2020

The following source code produces compile error:

typedef F<X> = void Function<Y extends X>();
F<X>? toF<X>(X x) => null;

class A<T> {}

F<A<Object?>>? testA(A? source) {
  var fsource = toF(source);
  return fsource;
}

main() {}

Sample output is:

$ dartanalyzer --enable-experiment=non-nullable,nonfunction-type-aliases test.dart
Analyzing test.dart...
error • A value of type 'void Function<Y extends A?>()?' can't be returned from function 'testA' because it has a return type of 'void Function<Y extends A<Object?>>()?'. • test.dart:10:10 • return_of_invalid_type
1 error found.

Dart version is:

Dart VM version: 2.8.0-edge.b20c35c7d90f790048ae54520d88e7b5bb158eaf (Mon Feb 17 15:00:05 2020 +0000) on "linux_x64"

@no-response no-response bot removed the needs-info We need additional information from the issue author (auto-closed after 14 days if no response) label Feb 24, 2020
@scheglov
Copy link
Contributor

@iarkh Hm... I think the error is expected here. The type void Function<Y extends A<dynamic>?>() is not a subtype of void Function<Y extends A<Object?>>(), because A<dynamic>? (note the question mark) and A<Object?> are not mutual subtypes. Types A<dynamic> and A<Object?> would be mutual subtypes.

@eernstg
Copy link
Member

eernstg commented Feb 26, 2020

Maybe the test should be like this?:

typedef F<X> = void Function<Y extends X>();
F<X>? toF<X>(X x) => null;

class A<T> {}

F<A<Object?>?>? testA(A? source) { // Extra `?` in return type.
  var fsource = toF(source);
  return fsource;
}

main() {}

I'll close this issue because we do get the expected results from the analyzer.

However, to sum up the considerations about how to test instantiation to bound, here's a version that gets as close as we can now:

typedef F<X> = void Function<Y extends X>();
F<X> toF<X>(X x) => throw 0;

class A<T> {}

F<A<Object?>> testA(A source) {
  var fsource = toF(source);
  return fsource;
}

void main() {
  print(testA);
}

@iarkh, this template should be usable for testing instantiation to bound under nnbd. Note that we never introduce nullability into the types, which will enable us to test a type like A? (if we were to write A? in order to test the type A then we wouldn't be able to test A? because we should then need to write A??, which is not syntactically supported and which is the same as A? anyway).

Note that the nnbd subtype rules will use mutual subtyping among type variable bounds, so this means that we cannot distinguish types like A<Object?> and A<dynamic> any more. This just means that we'll have to rely on tests that are less precise, and then we'll leave it to specialized tests (looking at the generated kernel code) to make the more precise distinctions.

@eernstg eernstg closed this as completed Feb 26, 2020
@iarkh
Copy link
Contributor Author

iarkh commented Feb 27, 2020

@iarkh, this template should be usable for testing instantiation to bound under nnbd. Note that we never introduce nullability into the types, which will enable us to test a type like A? (if we were to write A? in order to test the type A then we wouldn't be able to test A? because we should then need to write A??, which is not syntactically supported and which is the same as A? anyway).

Note that the nnbd subtype rules will use mutual subtyping among type variable bounds, so this means that we cannot distinguish types like A<Object?> and A<dynamic> any more. This just means that we'll have to rely on tests that are less precise, and then we'll leave it to specialized tests (looking at the generated kernel code) to make the more precise distinctions.

@eernstg, seems like it works for me, thank you!

All the old static tests for instantiate-to-bound calls toF directly, so they should be updated accordingly too. I've created co19 issue 533 about this and I am going to change the tests correspondingly together with nnbd ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
legacy-area-analyzer Use area-devexp instead. NNBD Issues related to NNBD Release
Projects
None yet
Development

No branches or pull requests

5 participants