dev_compiler emits errors compiling SDK

[jmesserly]

related to #58, but this is tracking the errors instead of the JS code issues. The errors can be seen with:

$ dart -c bin/devc.dart --sdk-check --dart-sdk test/generated_sdk dart:core

[vsmenon]

Here's the current count - many are analyzer warnings that we should treat as errors:

package AE IMO IRCE STE LOC
other 13 17 6 2 43685

---

package AE IMO IRCE STE LOC
total 13 17 6 2 43685
% 0.03 0.04 0.01 0.00 100

Where:
AE: AnalyzerError
IMO: InvalidMethodOverride
IRCE: InvalidRuntimeCheckError
STE: StaticTypeError
LOC: LinesOfCode

[vsmenon]

@leafpetersen

*** Compiling SDK to JavaScript
severe: line 211, column 38 of dart:async/broadcast_stream_controller.dart: [AnalyzerError] The getter '_next' is not defined for the class 'StreamSubscription'
assert(!identical(subscription._next, subscription));
^^^^^
severe: line 207, column 36 of dart:async/broadcast_stream_controller.dart: [AnalyzerError] The getter '_next' is not defined for the class 'StreamSubscription'
assert(!identical(subscription._next, subscription));
^^^^^
severe: line 209, column 20 of dart:async/broadcast_stream_controller.dart: [AnalyzerError] The method '_setRemoveAfterFiring' is not defined for the class 'StreamSubscription'
subscription._setRemoveAfterFiring();
^^^^^^^^^^^^^^^^^^^^^
severe: line 208, column 22 of dart:async/broadcast_stream_controller.dart: [AnalyzerError] The getter '_isFiring' is not defined for the class 'StreamSubscription'
if (subscription._isFiring) {
^^^^^^^^^
severe: line 206, column 32 of dart:async/broadcast_stream_controller.dart: [AnalyzerError] The getter '_next' is not defined for the class 'StreamSubscription'
if (identical(subscription._next, subscription)) return null;
^^^^^
severe: line 321, column 34 of dart:async/stream.dart: [AnalyzerError] The getter '_addError' is not defined for the class 'StreamController'
final addError = eventSink._addError;
^^^^^^^^^
severe: line 1227, column 17 of dart:async/stream.dart: [AnalyzerError] The method '_addError' is not defined for the class 'StreamController'
eventSink._addError(error, stackTrace); // Avoid Zone error replacement.
^^^^^^^^^
severe: line 396, column 30 of dart:async/stream.dart: [AnalyzerError] The getter '_addError' is not defined for the class 'StreamController'
onError: eventSink._addError, // Avoid Zone error replacement.
^^^^^^^^^
severe: line 329, column 3 of dart:_interceptors: [InvalidMethodOverride] Invalid override. The type of JSNull.== ((dynamic) → bool) is not a subtype of Interceptor.== ((Object) → bool).
bool operator ==(other) => identical(null, other);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
severe: line 110, column 17 of dart:_interceptors/js_number.dart: [StaticTypeError] Type check failed: this (JSNumber) is not of type double
toDouble() => this;
^^^^
severe: line 344, column 13 of dart:_interceptors/js_number.dart: [InvalidMethodOverride] Base class introduces an invalid override. The type of JSNumber.abs (() → num) is not a subtype of int.abs (() → int).
class JSInt extends JSNumber implements int, double {
^^^^^^^^^^^^^^^^
severe: line 344, column 13 of dart:_interceptors/js_number.dart: [InvalidMethodOverride] Base class introduces an invalid override. The type of JSNumber.unary- (() → num) is not a subtype of int.unary- (() → int).
class JSInt extends JSNumber implements int, double {
^^^^^^^^^^^^^^^^
severe: line 344, column 13 of dart:_interceptors/js_number.dart: [InvalidMethodOverride] Base class introduces an invalid override. The type of JSNumber.<< ((num) → num) is not a subtype of int.<< ((int) → int).
class JSInt extends JSNumber implements int, double {
^^^^^^^^^^^^^^^^
severe: line 344, column 13 of dart:_interceptors/js_number.dart: [InvalidMethodOverride] Base class introduces an invalid override. The type of JSNumber.>> ((num) → num) is not a subtype of int.>> ((int) → int).
class JSInt extends JSNumber implements int, double {
^^^^^^^^^^^^^^^^
severe: line 344, column 13 of dart:_interceptors/js_number.dart: [InvalidMethodOverride] Base class introduces an invalid override. The type of JSNumber.& ((num) → num) is not a subtype of int.& ((int) → int).
class JSInt extends JSNumber implements int, double {
^^^^^^^^^^^^^^^^
severe: line 344, column 13 of dart:_interceptors/js_number.dart: [InvalidMethodOverride] Base class introduces an invalid override. The type of JSNumber.| ((num) → num) is not a subtype of int.| ((int) → int).
class JSInt extends JSNumber implements int, double {
^^^^^^^^^^^^^^^^
severe: line 344, column 13 of dart:_interceptors/js_number.dart: [InvalidMethodOverride] Base class introduces an invalid override. The type of JSNumber.^ ((num) → num) is not a subtype of int.^ ((int) → int).
class JSInt extends JSNumber implements int, double {
^^^^^^^^^^^^^^^^
severe: line 344, column 13 of dart:_interceptors/js_number.dart: [InvalidMethodOverride] Base class introduces an invalid override. The type of JSNumber.sign (() → num) is not a subtype of int.sign (() → int).
class JSInt extends JSNumber implements int, double {
^^^^^^^^^^^^^^^^
severe: line 415, column 16 of dart:_interceptors/js_number.dart: [InvalidMethodOverride] Base class introduces an invalid override. The type of JSNumber.remainder ((num) → num) is not a subtype of double.remainder ((num) → double).
class JSDouble extends JSNumber implements double {
^^^^^^^^^^^^^^^^
severe: line 415, column 16 of dart:_interceptors/js_number.dart: [InvalidMethodOverride] Base class introduces an invalid override. The type of JSNumber.abs (() → num) is not a subtype of double.abs (() → double).
class JSDouble extends JSNumber implements double {
^^^^^^^^^^^^^^^^
severe: line 415, column 16 of dart:_interceptors/js_number.dart: [InvalidMethodOverride] Base class introduces an invalid override. The type of JSNumber.unary- (() → num) is not a subtype of double.unary- (() → double).
class JSDouble extends JSNumber implements double {
^^^^^^^^^^^^^^^^
severe: line 415, column 16 of dart:_interceptors/js_number.dart: [InvalidMethodOverride] Base class introduces an invalid override. The type of JSNumber.+ ((num) → num) is not a subtype of double.+ ((num) → double).
class JSDouble extends JSNumber implements double {
^^^^^^^^^^^^^^^^
severe: line 415, column 16 of dart:_interceptors/js_number.dart: [InvalidMethodOverride] Base class introduces an invalid override. The type of JSNumber.- ((num) → num) is not a subtype of double.- ((num) → double).
class JSDouble extends JSNumber implements double {
^^^^^^^^^^^^^^^^
severe: line 415, column 16 of dart:_interceptors/js_number.dart: [InvalidMethodOverride] Base class introduces an invalid override. The type of JSNumber.* ((num) → num) is not a subtype of double.* ((num) → double).
class JSDouble extends JSNumber implements double {
^^^^^^^^^^^^^^^^
severe: line 415, column 16 of dart:_interceptors/js_number.dart: [InvalidMethodOverride] Base class introduces an invalid override. The type of JSNumber.% ((num) → num) is not a subtype of double.% ((num) → double).
class JSDouble extends JSNumber implements double {
^^^^^^^^^^^^^^^^
severe: line 415, column 16 of dart:_interceptors/js_number.dart: [InvalidMethodOverride] Base class introduces an invalid override. The type of JSNumber.sign (() → num) is not a subtype of double.sign (() → double).
class JSDouble extends JSNumber implements double {
^^^^^^^^^^^^^^^^
severe: line 345, column 7 of dart:_interceptors/js_string.dart: [StaticTypeError] Type check failed: s += s (String) is not of type JSString
s += s;
^^^^^^
severe: line 452, column 28 of dart:_native_typed_data: [AnalyzerError] The getter 'length' is not defined for the class 'NativeTypedData'
if (length == list.length) {
^^^^^^
severe: line 372, column 61 of dart:collection: [InvalidRuntimeCheckError] Invalid runtime check on non-ground type K
: _validKey = (validKey != null) ? validKey : ((v) => v is K);
^^^^^^
severe: line 795, column 61 of dart:collection: [InvalidRuntimeCheckError] Invalid runtime check on non-ground type K
: _validKey = (validKey != null) ? validKey : ((v) => v is K);
^^^^^^
severe: line 1194, column 61 of dart:collection: [InvalidRuntimeCheckError] Invalid runtime check on non-ground type E
: _validKey = (validKey != null) ? validKey : ((x) => x is E);
^^^^^^
severe: line 1596, column 61 of dart:collection: [InvalidRuntimeCheckError] Invalid runtime check on non-ground type E
: _validKey = (validKey != null) ? validKey : ((x) => x is E);
^^^^^^
severe: line 610, column 60 of dart:collection/splay_tree.dart: [AnalyzerError] The getter '_validKey' is not defined for the class '_SplayTree'
new SplayTreeSet(setOrMap._comparator, setOrMap._validKey);
^^^^^^^^^
severe: line 610, column 38 of dart:collection/splay_tree.dart: [AnalyzerError] The getter '_comparator' is not defined for the class '_SplayTree'
new SplayTreeSet(setOrMap._comparator, setOrMap._validKey);
^^^^^^^^^^^
severe: line 266, column 65 of dart:collection/splay_tree.dart: [InvalidRuntimeCheckError] Invalid runtime check on non-ground type K
_validKey = (isValidKey != null) ? isValidKey : ((v) => v is K);
^^^^^^
severe: line 692, column 65 of dart:collection/splay_tree.dart: [InvalidRuntimeCheckError] Invalid runtime check on non-ground type E
_validKey = (isValidKey != null) ? isValidKey : ((v) => v is E);
^^^^^^
severe: line 612, column 5 of dart:core/list.dart: [AnalyzerError] The method 'checkNull' is not defined for the class 'List'
checkNull(start); // TODO(ahe): This is not specified but co19 tests it.
^^^^^^^^^
severe: line 523, column 22 of dart:core/list.dart: [AnalyzerError] Undefined name 'value'
if (this[i] == value) {

[jmesserly]

for JSDouble/JSInt/JSBool/JSString, ideally we could find a way to make those work with the checker. My current CL that attempts to fix the double/int/bool/string methods is using those dart2js types. They're a little strange ... for one thing, they illegally implement the primitive interfaces ... and int/double/num have some magic typing rules for operators (special cases in the spec). Hmmm, maybe I will take another look at just getting rid of those types.

[vsmenon]

Note, some of the errors are recently promoted analyzer warnings that were introduced in this CL:
https://codereview.chromium.org/1112403004/diff/20001/tool/input_sdk/lib/async/broadcast_stream_controller.dart?context=10&column_width=80&tab_spaces=8

[leafpetersen]

The analyzer errors are (as you mention) new since I last looked at this. The _interceptor/ errors I had ignored, since it wasn't clear what we were going to do with that code. Some of it is definitely oddly typed. The (v is K) code is interesting. This seems to be the only use of this idiom in the sdk, but it's a pretty hard one to work around. We might want to allow these "is" checks and throw at runtime if they turn out to be questions we can't accurately answer.

[jmesserly]

@leafpetersen not sure we can throw at runtime ... this is for map/set lookups. It would be unfortunate if Set<SomeType<int>> didn't work, because it will blow up on is K line upon seeing SomeType<int> is non-ground type for K.

If the key is "invalid", Set/Map want to bail early, rather than look up the item. Ignoring the test entirely (isValidKey: (k) => true) would be close to the right semantics, because if "obj is K" test fails, it should not compare == to anything in the set. However:

• bailing early is a performance optimization
• you could observe hashCode/== being called a different number of times
• a bad == method could lead to different result from lookup

If we did want to be more strict, IMO, we could just constrain lookup to require a K:

V operator [](K key) { ... }

That would disallow lookups from other key types.

Alternatively, we can change how our SDK is implemented, it doesn't need to use the is operator. Instead of obj is K ... it can do dart_runtime.compatInstanceOf(obj, K) ... and then we get full compat with the SDK. But we'd have to implement existing Dart subtype relation, so extra code size :\

Of these, I think my favorite is the (k) => true (it won't change results assuming correct == implementation), or just tighten the type [](K key) (means some code won't type check, but restores symmetry between [] and []=). Note that _LinkedHashSet and maybe some other impl types in dart2js already seem to treat isValidKey as true if not provided. So it's not consistent even in the core libs.

[leafpetersen]

Related: http://dartbug.com/23356 .

[jmesserly]

ah, yup. I forgot about compareTo. Yeah that makes more sense.
[edit: hit send too soon]

[jmesserly]

Based on that, maybe require K key is best for dev_compiler?

[jmesserly]

Somewhat related: https://github.com/chalin/DEP-non-null/blob/master/doc/dep-non-null-AUTOGENERATED-DO-NOT-EDIT.md#ii32-dart-sdk-library

the non-null DEP changes library methods like contains(Object value) to contains(E value), which fits naturally with DDC too, and makes it less likely to need these type annotations in the SDK