Make the VM's dart:core and dart:async library patches clean.

Make the VM's dart:core and dart:async library patches clean.

Clean up the VM's dart:core and dart:async library patches so that
they are clean according to the patching rules implemented in the
analyzer.  Specifically:

  - If a member is patched in a patch file, it must be declared external
    in the SDK.

  - If a member is introduced in a patch file, it must be private.

  - A non-private superclass member cannot be overridden in a patch
    file.

BUG=
R=lrn@google.com, sigmund@google.com

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

[Kevin Millikin (Google), 2016-12-08 11:26:14]

kmillikin@google.com changed reviewers:
+ floitsch@google.com, lrn@google.com

[Kevin Millikin (Google), 2016-12-08 11:26:15]

https://codereview.chromium.org/2563633002/diff/1/sdk/lib/internal/internal.dart
File sdk/lib/internal/internal.dart (right):

https://codereview.chromium.org/2563633002/diff/1/sdk/lib/internal/internal.d...
sdk/lib/internal/internal.dart:68: class ExternalName {
This is an annotation class used in Kernel binaries to associate a string with
an implementation of an external member.  Instead of:

factory _List(length) native "List_allocate";

this will look like it had been:

@ExternalName("List_allocate") _List(length);

[Lasse Reichstein Nielsen, 2016-12-08 12:25:32]

https://codereview.chromium.org/2563633002/diff/1/runtime/lib/core_patch.dart
File runtime/lib/core_patch.dart (right):

https://codereview.chromium.org/2563633002/diff/1/runtime/lib/core_patch.dart...
runtime/lib/core_patch.dart:32: final _SyncGeneratorCallback moveNextFn;
Not your CL, but ...
I don't think this field should be public. The class leaks and users can see the
field if they do `print(()sync*{}().moveNextFn)`. Same for iterator.

https://codereview.chromium.org/2563633002/diff/1/runtime/lib/core_patch.dart...
runtime/lib/core_patch.dart:45: _SyncGeneratorCallback moveNextFn;
All of these should be private, the user gets access to the object.
(Again, you don't have to fix it, but somebody will have to).

https://codereview.chromium.org/2563633002/diff/1/sdk/lib/_internal/js_runtim...
File sdk/lib/_internal/js_runtime/lib/core_patch.dart (right):

https://codereview.chromium.org/2563633002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:238: [List
existingArgumentNames = null])
This constructor should never have been public. Probably too late to fix now. :(

https://codereview.chromium.org/2563633002/diff/1/sdk/lib/core/bool.dart
File sdk/lib/core/bool.dart (right):

https://codereview.chromium.org/2563633002/diff/1/sdk/lib/core/bool.dart#newc...
sdk/lib/core/bool.dart:44: // TODO: Doc comment please.
Shouldn't be necessary. It satisfies the requirements stated in Object.hashCode,
and we don't want to commit to any further details.

https://codereview.chromium.org/2563633002/diff/1/sdk/lib/core/errors.dart
File sdk/lib/core/errors.dart (right):

https://codereview.chromium.org/2563633002/diff/1/sdk/lib/core/errors.dart#ne...
sdk/lib/core/errors.dart:419: external String toString();
Here I would actually prefer each implementation to have their own subclass
instead of patching the toString of the general FallThroughError.

If only the VM changes the toString, only it needs to subclass.

https://codereview.chromium.org/2563633002/diff/1/sdk/lib/core/errors.dart#ne...
sdk/lib/core/errors.dart:427: AbstractClassInstantiationError(String
this._className);
Ick. This exposes a private name in the DartDoc. Should at least be

AbstractClassInstantiationError(String className) : _className = className;

https://codereview.chromium.org/2563633002/diff/1/sdk/lib/core/errors.dart#ne...
sdk/lib/core/errors.dart:430: external String toString();
Again, I'd prefer an implementation specific subclass.

https://codereview.chromium.org/2563633002/diff/1/sdk/lib/core/null.dart
File sdk/lib/core/null.dart (right):

https://codereview.chromium.org/2563633002/diff/1/sdk/lib/core/null.dart#newc...
sdk/lib/core/null.dart:19: // TODO: Doc comment please.
No doc needed.

https://codereview.chromium.org/2563633002/diff/1/sdk/lib/internal/internal.dart
File sdk/lib/internal/internal.dart (right):

https://codereview.chromium.org/2563633002/diff/1/sdk/lib/internal/internal.d...
sdk/lib/internal/internal.dart:68: class ExternalName {
LGTM!

[Lasse Reichstein Nielsen, 2016-12-08 12:26:38]

https://codereview.chromium.org/2563633002/diff/1/sdk/lib/core/errors.dart
File sdk/lib/core/errors.dart (right):

https://codereview.chromium.org/2563633002/diff/1/sdk/lib/core/errors.dart#ne...
sdk/lib/core/errors.dart:419: external String toString();
I can see that that is a more complex change, because you would also need to
change the constructor used everywhere in the VM, so it's OK to keep this for
now, and we can see if we can fix it up later.

[floitsch, 2016-12-08 12:30:46]

https://codereview.chromium.org/2563633002/diff/1/sdk/lib/core/errors.dart
File sdk/lib/core/errors.dart (right):

https://codereview.chromium.org/2563633002/diff/1/sdk/lib/core/errors.dart#ne...
sdk/lib/core/errors.dart:427: AbstractClassInstantiationError(String
this._className);
On 2016/12/08 12:25:32, Lasse Reichstein Nielsen wrote:
> Ick. This exposes a private name in the DartDoc. Should at least be
> 
> AbstractClassInstantiationError(String className) : _className = className;
> 

Alternatively, dartdoc could try to be clever about this.
It's a very common pattern, and not being able to write the shorter version just
because of dartdoc is annoying.

[Kevin Millikin (Google), 2016-12-09 08:38:59]

Thanks for the review.  This also needs a change to the VM's implementation
before it can be committed.

https://codereview.chromium.org/2563633002/diff/1/runtime/lib/core_patch.dart
File runtime/lib/core_patch.dart (right):

https://codereview.chromium.org/2563633002/diff/1/runtime/lib/core_patch.dart...
runtime/lib/core_patch.dart:32: final _SyncGeneratorCallback moveNextFn;
On 2016/12/08 12:25:32, Lasse Reichstein Nielsen wrote:
> Not your CL, but ...
> I don't think this field should be public. The class leaks and users can see
the
> field if they do `print(()sync*{}().moveNextFn)`. Same for iterator.

That should be noncontroversial, so I'll just change it here and below.

https://codereview.chromium.org/2563633002/diff/1/sdk/lib/core/bool.dart
File sdk/lib/core/bool.dart (right):

https://codereview.chromium.org/2563633002/diff/1/sdk/lib/core/bool.dart#newc...
sdk/lib/core/bool.dart:44: // TODO: Doc comment please.
On 2016/12/08 12:25:32, Lasse Reichstein Nielsen wrote:
> Shouldn't be necessary. It satisfies the requirements stated in
Object.hashCode,
> and we don't want to commit to any further details.

Done.

https://codereview.chromium.org/2563633002/diff/1/sdk/lib/core/errors.dart
File sdk/lib/core/errors.dart (right):

https://codereview.chromium.org/2563633002/diff/1/sdk/lib/core/errors.dart#ne...
sdk/lib/core/errors.dart:419: external String toString();
On 2016/12/08 12:26:38, Lasse Reichstein Nielsen wrote:
> I can see that that is a more complex change, because you would also need to
> change the constructor used everywhere in the VM, so it's OK to keep this for
> now, and we can see if we can fix it up later.

OK.  I will make an issue to track this and other cleanups.

https://codereview.chromium.org/2563633002/diff/1/sdk/lib/core/errors.dart#ne...
sdk/lib/core/errors.dart:427: AbstractClassInstantiationError(String
this._className);
On 2016/12/08 12:30:46, floitsch wrote:
> On 2016/12/08 12:25:32, Lasse Reichstein Nielsen wrote:
> > Ick. This exposes a private name in the DartDoc. Should at least be
> > 
> > AbstractClassInstantiationError(String className) : _className = className;
> > 
> 
> Alternatively, dartdoc could try to be clever about this.
> It's a very common pattern, and not being able to write the shorter version
just
> because of dartdoc is annoying.

I'll adopt Lasse's suggestion.

https://codereview.chromium.org/2563633002/diff/1/sdk/lib/core/errors.dart#ne...
sdk/lib/core/errors.dart:430: external String toString();
On 2016/12/08 12:25:32, Lasse Reichstein Nielsen wrote:
> Again, I'd prefer an implementation specific subclass.

Acknowledged.

https://codereview.chromium.org/2563633002/diff/1/sdk/lib/core/null.dart
File sdk/lib/core/null.dart (right):

https://codereview.chromium.org/2563633002/diff/1/sdk/lib/core/null.dart#newc...
sdk/lib/core/null.dart:19: // TODO: Doc comment please.
On 2016/12/08 12:25:32, Lasse Reichstein Nielsen wrote:
> No doc needed.

Done.

[Kevin Millikin (Google), 2016-12-09 12:09:59]

Verified to work on the VM and dart2js.  Please take another look.

[Lasse Reichstein Nielsen, 2016-12-09 13:17:41]

lgtm

https://codereview.chromium.org/2563633002/diff/40001/sdk/lib/internal/intern...
File sdk/lib/internal/internal.dart (right):

https://codereview.chromium.org/2563633002/diff/40001/sdk/lib/internal/intern...
sdk/lib/internal/internal.dart:66: /// Provides a backend-specific String that
can be used to identify the
String -> `String` (or just lower-case string).
maybe: Provides -> Contains

[Kevin Millikin (Google), 2016-12-09 13:33:57]

kmillikin@google.com changed reviewers:
+ hausner@google.com, sra@google.com

[Kevin Millikin (Google), 2016-12-09 13:33:57]

+sra,hausner

Stephen, Matthias, will you please look at the dart2js (respectively VM) parts
of this change.

For context: the analyzer now has support for parsing and applying patch files. 
We are going to use it in the Kernel front end.  This parser is stricter than
the VM's patch parser, so some changes to the SDK and patch files are required.

For the VM: this is mostly making public members in patch files private.  For
dart2js: this is mostly making implementations that came from the SDK into ones
that come from the patch file.

[Kevin Millikin (Google), 2016-12-14 11:00:25]

kmillikin@google.com changed reviewers:
+ sigmund@google.com

[Kevin Millikin (Google), 2016-12-14 11:00:25]

+sigmund

[hausner, 2016-12-14 23:16:59]

The adjustment of the patch files look fine. However, I would encourage to
rethink whether the restrictions are really needed. The restrictions make the
simple concept of patching a lot more cumbersome.

If the new restrictions are needed, then I'd like to see them formally written
down, and enforced by the VM when it patches classes.

[Siggi Cherem (dart-lang), 2016-12-15 00:40:52]

lgtm

[Kevin Millikin (Google), 2016-12-15 09:10:15]

Description was changed from

==========
Make the VM's dart:core and dart:async library patches clean.

Clean up the VM's dart:core and dart:async library patches so that
they are clean according to the patching rules implemented in the
analyzer.  Specifically:

  - If a member is patched in a patch file, it must be declared external
    in the SDK.

  - If a member is introduced in a patch file, it must be private.

  - A non-private superclass member cannot be overridden in a patch
    file.

BUG=
==========

to

==========
Make the VM's dart:core and dart:async library patches clean.

Clean up the VM's dart:core and dart:async library patches so that
they are clean according to the patching rules implemented in the
analyzer.  Specifically:

  - If a member is patched in a patch file, it must be declared external
    in the SDK.

  - If a member is introduced in a patch file, it must be private.

  - A non-private superclass member cannot be overridden in a patch
    file.

BUG=
R=lrn@google.com, sigmund@google.com

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

[Kevin Millikin (Google), 2016-12-15 09:10:17]

Committed patchset #3 (id:40001) manually as
f7b6f0cf1c188bbc375de2a37f9a4129cb845428 (presubmit successful).