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: Never type is not treated as Null when legacy field overrides opted in field. #40389

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 related with the issues #39917, #40387 somehow.

Please let me know if this is a bug in the NNBD Spec, in this case I will file a new issue against the Spec.

Dart NNBD Spec reads:

Legacy libraries Static checking for a library which has not opted into this feature (a legacy library) is done using the semantics as of the last version of the language before this feature ships (or the last version to which it has opted in, if that is different). All opted-in libraries upstream from the legacy library are viewed by the legacy library with nullability related features erased from their APIs. In particular:

All types of the form T? in the opted-in API are treated as T. All required named parameters are treated as optional named parameters. The type Never is treated as the type Null In a legacy library, none of the new syntax introduced by this proposal is available, and it is a static error if it is used.

Particularly, this means that type Never should be treated as type Null when legacy code overrides opted in code.

However, this is not so. For example, try the following:

// testlib.dart: opted in library
library testlib;

class A {
  Never n = throw "Should not reach here";
}
// test.dart: legacy code
// @dart=2.6
import "testlib.dart";

class B extends A {
  Null n;
}

main() {}

Both dart and analyzer throw compile errors here. Sample output is:

$ dart --enable-experiment=non-nullable test.dart test.dart:6:8: Error: The return type of the method 'B.n' is 'Null', which does not match the return type, 'Never', of the overridden method, 'A.n'. Change to a subtype of 'Never'. Null n; ^ testlib.dart:5:9: Context: This is the overridden method ('n'). Never n = throw "Should not reach here"; ^ $ dartanalyzer --enable-experiment=non-nullable test.dart Analyzing test.dart... error • 'B.n' ('Null Function()') isn't a valid override of 'A.n' ('Never Function()'). • test.dart:6:8 • invalid_override 1 error found.

alexmarkov commented 4 years ago

/cc @johnniwinther

eernstg commented 4 years ago

I suspect that the sentence 'The type Never is treated as the type Null' may have been written at a point in time where the legacy library was not expected to be able to refer to the type Never. I believe that there is no need to have this sentence, and it actually creates some problems as shown here, and I've suggested in https://github.com/dart-lang/language/issues/825 that we delete this sentence from the nnbd spec.

leafpetersen commented 4 years ago

The implementations behave correctly on this now - they report an error in strong mode and accept the code in weak mode.