unfork DDC's copy of most SDK libraries

unfork DDC's copy of most SDK libraries

There's some changes in strong mode errors we may want to follow up on.

Unforks the following: async, collection, convert, core, developer, internal, io, isolate, math, mirrors.

Does not unfork: html, indexed_db, js, js_util, svg, typed_data, web_audio, web_gl, web_sql.

R=vsm@google.com

Committed: https://github.com/dart-lang/sdk/commit/fe294f37538df7094df8b09b6bef1a53de4fc4a4

[Jennifer Messerly, 2017-02-17 01:27:07]

Description was changed from

==========
remove DDC's copy of most SDK libraries, use the standard ones

there's some changes in strong mode errors we may want to follow up on.
==========

to

==========
unfork DDC's copy of most SDK libraries

There's some changes in strong mode errors we may want to follow up on.

Unforks the following: async, collection, convert, core, developer, internal,
io, isolate, math, mirrors.

Does not unfork: html, indexed_db, js, js_util, svg, typed_data, web_audio,
web_gl, web_sql.
==========

[Jennifer Messerly, 2017-02-17 02:29:55]

Patchset #1 (id:1) has been deleted

[Jennifer Messerly, 2017-02-17 02:30:19]

Patchset #1 (id:1) has been deleted

[Jennifer Messerly, 2017-02-17 02:30:51]

jmesserly@google.com changed reviewers:
+ vsm@google.com

[Jennifer Messerly, 2017-02-17 02:31:52]

Hi Vijay, here's the change to unfork non-html SDK libraries. I think it's ready
for review. Let me know!

[vsm, 2017-02-17 16:37:52]

Nice!

Mostly comments on the new/different errors.

If you can send me a CL or branch based off of:

https://github.com/dart-lang/sdk/commits/ddc-1.22.0-dev.10.6

I can try internally.  If that looks good and our own tests are passing, I think
you should land this, and we should file issues on the sdk folks.

https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/test/b...
File pkg/dev_compiler/test/browser/language_tests.js (right):

https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/test/b...
pkg/dev_compiler/test/browser/language_tests.js:353: 'list_test': fail,
Any idea why this one started breaking?

https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/i...
File pkg/dev_compiler/tool/input_sdk/patch/internal_patch.dart (right):

https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/patch/internal_patch.dart:50: //
TODO(jmesserly): is this equivalent to _nativeSymbol toString?
I think you're right - though somewhat accidental.  Looks like ES6 prints them
the same already.

https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/s...
File pkg/dev_compiler/tool/sdk_expected_errors.txt (right):

https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/s...
pkg/dev_compiler/tool/sdk_expected_errors.txt:4: [error] The argument type
'ListMixin<E>' can't be assigned to the parameter type 'List<Comparable>'.
(dart:collection/list.dart, line 312, col 17)
These last two on list.dart look new.

https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/s...
pkg/dev_compiler/tool/sdk_expected_errors.txt:10: [error] Invalid override. The
type of 'AsciiDecoder.startChunkedConversion' ('(Sink<String>) →
ByteConversionSink') isn't a subtype of 'ChunkedConverter<List<int>, String,
List<int>, String>.startChunkedConversion' ('(dynamic) → dynamic').
(dart:convert/ascii.dart, line 237, col 3)
Ditto the last 5 on ascii.dart

https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/s...
pkg/dev_compiler/tool/sdk_expected_errors.txt:14: [error] Invalid override. The
type of 'Base64Decoder.startChunkedConversion' ('(Sink<List<int>>) →
StringConversionSink') isn't a subtype of 'ChunkedConverter<String, List<int>,
String, List<int>>.startChunkedConversion' ('(dynamic) → dynamic').
(dart:convert/base64.dart, line 359, col 3)
And these 4.

https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/s...
pkg/dev_compiler/tool/sdk_expected_errors.txt:32: [error] Invalid override. The
type of 'Utf8Decoder.bind' ('(Stream<List<int>>) → Stream<String>') isn't a
subtype of 'ChunkedConverter<List<int>, String, List<int>, String>.bind'
('(dynamic) → dynamic'). (dart:convert/utf.dart, line 371, col 3)
This whole chunk looks new.

https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/s...
pkg/dev_compiler/tool/sdk_expected_errors.txt:37: [error] Annotation must be
either a const variable reference or const constructor invocation.
(dart:core/stacktrace.dart, line 43, col 3)
This looks like a dart2js annotation (@NoInline) - can we drop/add our own
definition?

https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/s...
pkg/dev_compiler/tool/sdk_expected_errors.txt:40: [error] Annotation must be
either a const variable reference or const constructor invocation.
(dart:developer, line 39, col 1)
I only see @deprecated annotations in this library.  I think we should be
picking that up from core?

https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/s...
pkg/dev_compiler/tool/sdk_expected_errors.txt:51: [error] Invalid override. The
type of '_WindowsCodePageDecoder.startChunkedConversion' ('(Sink<String>) →
ByteConversionSink') isn't a subtype of 'ChunkedConverter<List<int>, String,
List<int>, String>.startChunkedConversion' ('(dynamic) → dynamic').
(dart:io/string_transformer.dart, line 112, col 3)
Not looking too hard at the io ones since it mostly throws....

https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/s...
pkg/dev_compiler/tool/sdk_expected_errors.txt:52: [error] The getter
'packagesBase' isn't defined for the class 'IsolateNatives'. (dart:isolate, line
721, col 61)
This is new.

https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/s...
pkg/dev_compiler/tool/sdk_expected_errors.txt:55: [error] The return type
'double' isn't a 'T', as defined by the method 'max'. (dart:math, line 125, col
16)
These might be something we fixed locally?

https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/s...
pkg/dev_compiler/tool/sdk_expected_errors.txt:58: [warning] Unsafe implicit cast
from 'StreamSubscription<dynamic>' to 'StreamSubscription<T>'. This usually
indicates that type information was lost and resulted in 'dynamic' and/or a
place that will have a failure at runtime. (dart:async/stream_impl.dart, line
1011, col 42)
New

[Jennifer Messerly, 2017-02-17 16:42:19]

On 2017/02/17 16:37:52, vsm wrote:
> Nice!
> 
> Mostly comments on the new/different errors.
> 
> If you can send me a CL or branch based off of:
> 
> https://github.com/dart-lang/sdk/commits/ddc-1.22.0-dev.10.6
> 
> I can try internally.  If that looks good and our own tests are passing, I
think
> you should land this, and we should file issues on the sdk folks.
> 
>
https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/test/b...
> File pkg/dev_compiler/test/browser/language_tests.js (right):
> 
>
https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/test/b...
> pkg/dev_compiler/test/browser/language_tests.js:353: 'list_test': fail,
> Any idea why this one started breaking?
> 
>
https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/i...
> File pkg/dev_compiler/tool/input_sdk/patch/internal_patch.dart (right):
> 
>
https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/i...
> pkg/dev_compiler/tool/input_sdk/patch/internal_patch.dart:50: //
> TODO(jmesserly): is this equivalent to _nativeSymbol toString?
> I think you're right - though somewhat accidental.  Looks like ES6 prints them
> the same already.
> 
>
https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/s...
> File pkg/dev_compiler/tool/sdk_expected_errors.txt (right):
> 
>
https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/s...
> pkg/dev_compiler/tool/sdk_expected_errors.txt:4: [error] The argument type
> 'ListMixin<E>' can't be assigned to the parameter type 'List<Comparable>'.
> (dart:collection/list.dart, line 312, col 17)
> These last two on list.dart look new.
> 
>
https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/s...
> pkg/dev_compiler/tool/sdk_expected_errors.txt:10: [error] Invalid override.
The
> type of 'AsciiDecoder.startChunkedConversion' ('(Sink<String>) →
> ByteConversionSink') isn't a subtype of 'ChunkedConverter<List<int>, String,
> List<int>, String>.startChunkedConversion' ('(dynamic) → dynamic').
> (dart:convert/ascii.dart, line 237, col 3)
> Ditto the last 5 on ascii.dart
> 
>
https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/s...
> pkg/dev_compiler/tool/sdk_expected_errors.txt:14: [error] Invalid override.
The
> type of 'Base64Decoder.startChunkedConversion' ('(Sink<List<int>>) →
> StringConversionSink') isn't a subtype of 'ChunkedConverter<String, List<int>,
> String, List<int>>.startChunkedConversion' ('(dynamic) → dynamic').
> (dart:convert/base64.dart, line 359, col 3)
> And these 4.
> 
>
https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/s...
> pkg/dev_compiler/tool/sdk_expected_errors.txt:32: [error] Invalid override.
The
> type of 'Utf8Decoder.bind' ('(Stream<List<int>>) → Stream<String>') isn't a
> subtype of 'ChunkedConverter<List<int>, String, List<int>, String>.bind'
> ('(dynamic) → dynamic'). (dart:convert/utf.dart, line 371, col 3)
> This whole chunk looks new.
> 
>
https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/s...
> pkg/dev_compiler/tool/sdk_expected_errors.txt:37: [error] Annotation must be
> either a const variable reference or const constructor invocation.
> (dart:core/stacktrace.dart, line 43, col 3)
> This looks like a dart2js annotation (@NoInline) - can we drop/add our own
> definition?
> 
>
https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/s...
> pkg/dev_compiler/tool/sdk_expected_errors.txt:40: [error] Annotation must be
> either a const variable reference or const constructor invocation.
> (dart:developer, line 39, col 1)
> I only see @deprecated annotations in this library.  I think we should be
> picking that up from core?
> 
>
https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/s...
> pkg/dev_compiler/tool/sdk_expected_errors.txt:51: [error] Invalid override.
The
> type of '_WindowsCodePageDecoder.startChunkedConversion' ('(Sink<String>) →
> ByteConversionSink') isn't a subtype of 'ChunkedConverter<List<int>, String,
> List<int>, String>.startChunkedConversion' ('(dynamic) → dynamic').
> (dart:io/string_transformer.dart, line 112, col 3)
> Not looking too hard at the io ones since it mostly throws....
> 
>
https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/s...
> pkg/dev_compiler/tool/sdk_expected_errors.txt:52: [error] The getter
> 'packagesBase' isn't defined for the class 'IsolateNatives'. (dart:isolate,
line
> 721, col 61)
> This is new.
> 
>
https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/s...
> pkg/dev_compiler/tool/sdk_expected_errors.txt:55: [error] The return type
> 'double' isn't a 'T', as defined by the method 'max'. (dart:math, line 125,
col
> 16)
> These might be something we fixed locally?
> 
>
https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/s...
> pkg/dev_compiler/tool/sdk_expected_errors.txt:58: [warning] Unsafe implicit
cast
> from 'StreamSubscription<dynamic>' to 'StreamSubscription<T>'. This usually
> indicates that type information was lost and resulted in 'dynamic' and/or a
> place that will have a failure at runtime. (dart:async/stream_impl.dart, line
> 1011, col 42)
> New

yeah ... I can see the new ones in the diff :) ... the more interesting question
is what to do about them?

[Jennifer Messerly, 2017-02-24 20:12:32]

Patchset #3 (id:30001) has been deleted

[Jennifer Messerly, 2017-02-24 20:12:40]

Patchset #3 (id:40001) has been deleted

[Jennifer Messerly, 2017-02-24 23:00:44]

Thanks! Here's my replies to those comments.

https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/test/b...
File pkg/dev_compiler/test/browser/language_tests.js (right):

https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/test/b...
pkg/dev_compiler/test/browser/language_tests.js:353: 'list_test': fail,
On 2017/02/17 16:37:51, vsm wrote:
> Any idea why this one started breaking?

yes, was an internal array created with "[]" instead of "<E>[]" I guess a local
fix we didn't push back. I've rolled it in here and I'll work on a separate CL
for that.

https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/s...
File pkg/dev_compiler/tool/sdk_expected_errors.txt (right):

https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/s...
pkg/dev_compiler/tool/sdk_expected_errors.txt:4: [error] The argument type
'ListMixin<E>' can't be assigned to the parameter type 'List<Comparable>'.
(dart:collection/list.dart, line 312, col 17)
On 2017/02/17 16:37:51, vsm wrote:
> These last two on list.dart look new.

right. so this is a legit error. we work around it in JSArray via:

      Sort.sort(this, (a, b) => Comparable.compare(a, b));

instead of:


      Sort.sort(this, Comparable.compare);

what is going on though: you'll get a cast error at runtime if your array
doesn't contain Comparable values and you fail to supply a compare function.

I can send out a CL to fix that in the SDK.

https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/s...
pkg/dev_compiler/tool/sdk_expected_errors.txt:10: [error] Invalid override. The
type of 'AsciiDecoder.startChunkedConversion' ('(Sink<String>) →
ByteConversionSink') isn't a subtype of 'ChunkedConverter<List<int>, String,
List<int>, String>.startChunkedConversion' ('(dynamic) → dynamic').
(dart:convert/ascii.dart, line 237, col 3)
On 2017/02/17 16:37:51, vsm wrote:
> Ditto the last 5 on ascii.dart

yes these are all startChunkedConversion and bind (discussed below)

https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/s...
pkg/dev_compiler/tool/sdk_expected_errors.txt:14: [error] Invalid override. The
type of 'Base64Decoder.startChunkedConversion' ('(Sink<List<int>>) →
StringConversionSink') isn't a subtype of 'ChunkedConverter<String, List<int>,
String, List<int>>.startChunkedConversion' ('(dynamic) → dynamic').
(dart:convert/base64.dart, line 359, col 3)
On 2017/02/17 16:37:51, vsm wrote:
> And these 4.

yes these are all startChunkedConversion and bind (discussed below)

https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/s...
pkg/dev_compiler/tool/sdk_expected_errors.txt:32: [error] Invalid override. The
type of 'Utf8Decoder.bind' ('(Stream<List<int>>) → Stream<String>') isn't a
subtype of 'ChunkedConverter<List<int>, String, List<int>, String>.bind'
('(dynamic) → dynamic'). (dart:convert/utf.dart, line 371, col 3)
On 2017/02/17 16:37:51, vsm wrote:
> This whole chunk looks new.

yeah, it looks like we never fixed the typing issues in dart:convert for real.

I can give it a shot, but note we were in a pretty bad place already w/ our out
of date SDK code.

https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/s...
pkg/dev_compiler/tool/sdk_expected_errors.txt:37: [error] Annotation must be
either a const variable reference or const constructor invocation.
(dart:core/stacktrace.dart, line 43, col 3)
On 2017/02/17 16:37:51, vsm wrote:
> This looks like a dart2js annotation (@NoInline) - can we drop/add our own
> definition?

we do declare NoInline, it just needs to be added to "show" in core_patch

https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/s...
pkg/dev_compiler/tool/sdk_expected_errors.txt:40: [error] Annotation must be
either a const variable reference or const constructor invocation.
(dart:developer, line 39, col 1)
On 2017/02/17 16:37:51, vsm wrote:
> I only see @deprecated annotations in this library.  I think we should be
> picking that up from core?

it was @ForceInline which was added to dart2js but not us.

https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/s...
pkg/dev_compiler/tool/sdk_expected_errors.txt:51: [error] Invalid override. The
type of '_WindowsCodePageDecoder.startChunkedConversion' ('(Sink<String>) →
ByteConversionSink') isn't a subtype of 'ChunkedConverter<List<int>, String,
List<int>, String>.startChunkedConversion' ('(dynamic) → dynamic').
(dart:io/string_transformer.dart, line 112, col 3)
On 2017/02/17 16:37:51, vsm wrote:
> Not looking too hard at the io ones since it mostly throws....

i mean, also it's the same startChunkedConversion issue that occurs everywhere.

https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/s...
pkg/dev_compiler/tool/sdk_expected_errors.txt:52: [error] The getter
'packagesBase' isn't defined for the class 'IsolateNatives'. (dart:isolate, line
721, col 61)
On 2017/02/17 16:37:51, vsm wrote:
> This is new.

Fixed. it was added to dart2js private lib

https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/s...
pkg/dev_compiler/tool/sdk_expected_errors.txt:55: [error] The return type
'double' isn't a 'T', as defined by the method 'max'. (dart:math, line 125, col
16)
On 2017/02/17 16:37:51, vsm wrote:
> These might be something we fixed locally?

Yes, we are missing Bob's fix:
https://github.com/dart-lang/sdk/commit/20e402c43dfbcad502f794eecab81f311c421fb7

I will take a look. It will be a non-trivial CL because it needs to make the
method external, so affects VM & dart2js patch files.

https://codereview.chromium.org/2698353003/diff/20001/pkg/dev_compiler/tool/s...
pkg/dev_compiler/tool/sdk_expected_errors.txt:58: [warning] Unsafe implicit cast
from 'StreamSubscription<dynamic>' to 'StreamSubscription<T>'. This usually
indicates that type information was lost and resulted in 'dynamic' and/or a
place that will have a failure at runtime. (dart:async/stream_impl.dart, line
1011, col 42)
On 2017/02/17 16:37:51, vsm wrote:
> New

fixed.

[vsm, 2017-02-24 23:23:15]

lgtm

[Jennifer Messerly, 2017-02-24 23:33:39]

On 2017/02/24 23:23:15, vsm wrote:
> lgtm

thanks!!! I'll land once I get land these two:
fix Future onError: https://codereview.chromium.org/2718643003/
fix ListMixin: https://codereview.chromium.org/2715833004/

[Lasse Reichstein Nielsen, 2017-02-28 14:55:02]

lrn@google.com changed reviewers:
+ lrn@google.com

[Lasse Reichstein Nielsen, 2017-02-28 14:55:05]

https://codereview.chromium.org/2698353003/diff/80001/sdk/lib/collection/list...
File sdk/lib/collection/list.dart (right):

https://codereview.chromium.org/2698353003/diff/80001/sdk/lib/collection/list...
sdk/lib/collection/list.dart:277: void _filter(List source, bool test(var
element), bool retainMatching) {
Now that this is no longer a static method, the first argument can be dropped -
it's always `this`.

[Jennifer Messerly, 2017-02-28 23:46:16]

Thanks! I've updated this if you want to take another look.

https://codereview.chromium.org/2698353003/diff/80001/sdk/lib/collection/list...
File sdk/lib/collection/list.dart (right):

https://codereview.chromium.org/2698353003/diff/80001/sdk/lib/collection/list...
sdk/lib/collection/list.dart:277: void _filter(List source, bool test(var
element), bool retainMatching) {
On 2017/02/28 14:55:05, Lasse Reichstein Nielsen wrote:
> Now that this is no longer a static method, the first argument can be dropped
-
> it's always `this`.

ah good catch! I made this fix over at:
https://codereview.chromium.org/2715833004/

[Jennifer Messerly, 2017-03-01 01:11:03]

Description was changed from

==========
unfork DDC's copy of most SDK libraries

There's some changes in strong mode errors we may want to follow up on.

Unforks the following: async, collection, convert, core, developer, internal,
io, isolate, math, mirrors.

Does not unfork: html, indexed_db, js, js_util, svg, typed_data, web_audio,
web_gl, web_sql.
==========

to

==========
unfork DDC's copy of most SDK libraries

There's some changes in strong mode errors we may want to follow up on.

Unforks the following: async, collection, convert, core, developer, internal,
io, isolate, math, mirrors.

Does not unfork: html, indexed_db, js, js_util, svg, typed_data, web_audio,
web_gl, web_sql.

R=vsm@google.com

Committed:
https://github.com/dart-lang/sdk/commit/fe294f37538df7094df8b09b6bef1a53de4fc4a4
==========

[Jennifer Messerly, 2017-03-01 01:11:05]

Committed patchset #8 (id:100001) manually as
fe294f37538df7094df8b09b6bef1a53de4fc4a4 (presubmit successful).