dev_compiler emits errors compiling SDK

[vsmenon]

From @jmesserly on March 19, 2015 17:42

related to dart-archive/dev_compiler#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

Copied from original issue: dart-archive/dev_compiler#103

[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) {

[vsmenon]

From @jmesserly on May 27, 2015 22:23

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

[vsmenon]

From @leafpetersen on May 27, 2015 23:12

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.

[vsmenon]

From @jmesserly on May 28, 2015 18:32

@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.

[vsmenon]

From @leafpetersen on May 28, 2015 19:46

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

[vsmenon]

From @jmesserly on May 28, 2015 20:9

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

[vsmenon]

From @jmesserly on May 28, 2015 20:9

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

[vsmenon]

From @jmesserly on June 24, 2015 19:15

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

[vsmenon]

From @chalin on June 25, 2015 1:7

On Wed, Jun 24, 2015 at 12:15 PM, John Messerly [email protected]
wrote:

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

After this discussion
https://groups.google.com/a/dartlang.org/d/msg/core-dev/uYaX_aKBi0Q/y_I_ZjXd8vcJ,
I'm actually going back to change all of those argument methods to ?Object.
The problem is that argument type E can result in a dynamic type error in
cases of legitimate use: e.g., suppose MySet is a Set but with
typedContains(E value). Then the following code:

bool isSubset(MySet s, MySet t) {
for (Object o in s) if (!t.typedContains(o)) return false;
return true;
}

will result in a dynamic type error when invoked with sets, say, {'a', 1}
and {1,2,3} even if it is properly statically typed.

Cheers

Patrice [image: View my LinkedIn Profile.]
http://www.linkedin.com/in/patricechalin [image: Google+ Profile.]
https://plus.google.com/+PatriceChalin

[vsmenon]

From @jmesserly on June 26, 2015 17:55

btw, we should look at warnings too. DownCastComposite casts probably fail. For example:

warning: line 83, column 14 of dart:async/stream_controller.dart: [DownCastComposite] sync ? new _NoCallbackSyncStreamController() : new _NoCallbackAsyncStreamController() (_StreamController<dynamic>) will need runtime check to cast to type StreamController<T>

from this code:

  factory StreamController({void onListen(),
                            void onPause(),
                            void onResume(),
                            onCancel(),
                            bool sync: false}) {
    if (onListen == null && onPause == null &&
        onResume == null && onCancel == null) {
      return sync
          ? new _NoCallbackSyncStreamController/*<T>*/()
          : new _NoCallbackAsyncStreamController/*<T>*/();
    }
    return sync
         ? new _SyncStreamController<T>(onListen, onPause, onResume, onCancel)
         : new _AsyncStreamController<T>(onListen, onPause, onResume, onCancel);
  }

[vsmenon]

From @jmesserly on September 18, 2015 15:18

fyi, I landed a change to allow us to implement the disallowed types (e.g. int/double/bool/String)
d7cd11f

so when we update Analyzer, those will go away at least.

[vsmenon]

From @jmesserly on September 18, 2015 21:28

more progress: https://codereview.chromium.org/1348453004/

[vsmenon]

From @jmesserly on September 19, 2015 0:12

dart-archive/dev_compiler@94d823b too
errors are fixed now (once we update analyzer), lots of serious warnings though.

[vsmenon]

We have errors here again. Marking as a soundness issue as we're force compiling.

[mit-mit]

@vsmenon this is starting to look a bit stale, is this still an issue?

[vsmenon]

@munificent - can you triage & reassign this to the actual libraries with errors? Some may be in DDC patch files, but I believe most are in shared SDK code:

https://github.com/dart-lang/sdk/blob/master/pkg/dev_compiler/tool/sdk_expected_errors.txt

[dgrove]

@munificent any updates here? Do we need this for 1.23?

[munificent]

I'll take a look. I don't think we need it for 1.23, but @vsmenon can correct me if I'm wrong.

[jmesserly]

Should we open an issue under the SDK label? this isn't really a DDC issue anymore (except inasmuch as we have patch file problems). See also #29296

[dgrove]

Any updates here?

[munificent]

Yes! Florian fixed most of the SDK errors, and I fixed a couple more. He didn't rebuild the DDC SDK, so you don't see him in the history, but you can see most of the errors going away.

We're down to 12 errors and a pile of warnings. All of the warnings are the same root cause in dart:html. The errors are here and there. I'll try to beat them into submission. I don't see anything too alarming, except for math.min and .max, which might be a little tricky.

[jmesserly]

FYI @munificent -- I previously split out min/max into issue #28909, it has a link to your old CL and some backstory.

[munificent]

Ah, cool. Thanks. I'll claim that and try to get it fixed.

[dgrove]

Is this on target for 1.24?

[vsmenon]

No, good progress, but html ones will not be done in time.

[vsmenon]

Errors gone in html now. Still have some bogus warnings.

[munificent]

🎉 🎉🎉🎉🎉🎉