dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.25k stars 1.58k forks source link

NNBD: Analyzer allows inherit two interfaces with incompatible type parameters: `Object?` vs `FutureOr`, `Object?` vs `FutureOr<FutureOr>` #40454

Closed iarkh closed 4 years ago

iarkh commented 4 years ago

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

This issue is similar with #40453, but dart and analyzer behave in the different ways here.

The following source code declares classes X1 and X2 which inherit classes with incompatible type arguments:

import "dart:async";

class A<T>{}
class B implements A<Object?> {}

class C implements A<FutureOr> {}
class D implements A<FutureOr<FutureOr<FutureOr>>> {}

class X1 extends B implements C {}
class X2 extends B implements D {}

main() {}

This code throws two compile errors with dart and passes with analyzer. Seems like error should be thrown in both cases.

Sample output is:

$ dart --enable-experiment=non-nullable test.dart test.dart:9:7: Error: 'X1' can't implement both 'A<Object?>' and 'A<FutureOr>'

  • 'A' is from 'test.dart'.
  • 'Object' is from 'dart:core'.
  • 'FutureOr' is from 'dart:async'. class X1 extends B implements C {} ^ test.dart:10:7: Error: 'X2' can't implement both 'A<Object?>' and 'A<FutureOr<FutureOr<FutureOr>>>'
  • 'A' is from 'test.dart'.
  • 'Object' is from 'dart:core'.
  • 'FutureOr' is from 'dart:async'. class X2 extends B implements D {} ^ $ dartanalyzer --enable-experiment=non-nullable test.dart Analyzing test.dart... No issues found!
lrhn commented 4 years ago

Reading the spec, I believe the analyzer is correct if the code is null-safe. (I'm not so sure if it's legacy code, though).

The check for having the same instantiation of an interface uses NORM and NNBD_TOP_MERGE:

If a class C in an opted-in library implements the same generic class I more than once as I0, .., In, and at least one of the Ii is not syntactically equal to the others, then it is an error if NNBD_TOP_MERGE(S0, ..., Sn) is not defined where Si is NORM(Ii). Otherwise, for the purposes of runtime subtyping checks, C is considered to implement the canonical interface given by NNBD_TOP_MERGE(S0, ..., Sn).

The type NORM(FutureOr) is dynamic, as is NORM(FutureOr<FutureOr<FutureOr>>). The NNBD_TOP_MERGE(Object?, dynamic) is Object?, which means it's defined and there is no error.

iarkh commented 4 years ago

The same problem is reproducible for the dynamic vs FutureOr case.

eernstg commented 4 years ago

Agreeing that there is no error here, note that dart-lang/language#824 aims to make an adjustment to the definition of NNBD_TOP_MERGE. This will not change the ability of an opted-it class to have superinterfaces that differ in the ways that NNBD_TOP_MERGE eliminates, but it will make them implement a superinterface that has type argument Object? or dynamic in some situations where the current rules would yield void.

chloestefantsova commented 4 years ago

Proposed fix: https://dart-review.googlesource.com/c/sdk/+/134842