Make some VM libraries patch cleanly using the analyzer.

Make some VM libraries patch cleanly using the analyzer.

Modify the following VM dart: library patches: collection, convert,
developer, _internal, isolate, math, mirrors, profiler, vmservice_io,
_vmservice, _builtin, nativewrappers, and io.

The modifications are mostly because:

    - Patches should not be able to introduce new public members to a
      library's API.  In cases where the VM's patches introduce public
      members, the patches are rewritten to introduce private members
      instead.

    - Patches should not be able to replace arbitrary members, only
      ones declared external.  In cases where the VM's patches replace
      arbitrary members, those members are declared external in the
      SDK sources and dart2js's patch files are rewritten to use the
      original SDK implementation.

BUG=
R=sigmund@google.com

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

[Kevin Millikin (Google), 2016-12-12 11:03:43]

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

[Kevin Millikin (Google), 2016-12-12 11:03:44]

These are the rest of the VM's dart: libraries except for dart:typed_data, which
has an interesting implementation and I'll change it separately.

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

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

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

+sigmund

[hausner, 2016-12-14 23:35:41]

There are a few cases where an existing function in the to-be-patched class is
replaced with an external declaration and then the function is patched in the
patch file. Why is that?

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.

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

https://codereview.chromium.org/2564383002/diff/1/sdk/lib/internal/symbol.dar...
sdk/lib/internal/symbol.dart:117: external toString();
Why is it necessary to patch toString()? Isn't the implementation the same for
all platforms?

[Siggi Cherem (dart-lang), 2016-12-15 00:31:46]

dart2js library changes lgtm

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

https://codereview.chromium.org/2564383002/diff/1/sdk/lib/internal/symbol.dar...
sdk/lib/internal/symbol.dart:117: external toString();
On 2016/12/14 23:35:40, hausner wrote:
> Why is it necessary to patch toString()? Isn't the implementation the same for
> all platforms?

It seems like the vm was patching it with some extra conversion to change names
like "set:foo' to 'foo=': 
https://github.com/dart-lang/sdk/blob/master/runtime/lib/symbol_patch.dart#L9

Is the VM patch is no longer necessary?


I also noticed that we are getting some inconsistent results among tools: this
program prints 'foo=' in the VM, and produces compile-time errors in dart2js and
analyzer:

main() {
  print(const Symbol('set:foo'));
}

https://codereview.chromium.org/2564383002/diff/1/sdk/lib/mirrors/mirrors.dart
File sdk/lib/mirrors/mirrors.dart (left):

https://codereview.chromium.org/2564383002/diff/1/sdk/lib/mirrors/mirrors.dar...
sdk/lib/mirrors/mirrors.dart:90: LibraryMirror findLibrary(Symbol libraryName) {
I was thinking whether there will be much of an impact if we take the vm
implementation here.

I like that the vm patch has better error messages. The risk is that those error
messages call `getName` which can cause more bloat (although if you are already
using `findLibrary` we are likely already in a place with a lot of bloat
regardless).

My current inclination is to keep what you did here, but I just wanted to bring
it up a possible alternative.

[Lasse Reichstein Nielsen, 2016-12-15 07:55:42]

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

https://codereview.chromium.org/2564383002/diff/1/sdk/lib/internal/symbol.dar...
sdk/lib/internal/symbol.dart:117: external toString();
That's http://dartbug.com/13596 - the VM isn't validating arguments to the
Symbol constructor.

It's somewhat morally defensible since a const constructor can't really validate
anything, but it could be handled by special-casing.

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

Description was changed from

==========
Make some VM libraries patch cleanly using the analyzer.

Modify the following VM dart: library patches: collection, convert,
developer, _internal, isolate, math, mirrors, profiler, vmservice_io,
_vmservice, _builtin, nativewrappers, and io.

The modifications are mostly because:

    - Patches should not be able to introduce new public members to a
      library's API.  In cases where the VM's patches introduce public
      members, the patches are rewritten to introduce private members
      instead.

    - Patches should not be able to replace arbitrary members, only
      ones declared external.  In cases where the VM's patches replace
      arbitrary members, those members are declared external in the
      SDK sources and dart2js's patch files are rewritten to use the
      original SDK implementation.

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

to

==========
Make some VM libraries patch cleanly using the analyzer.

Modify the following VM dart: library patches: collection, convert,
developer, _internal, isolate, math, mirrors, profiler, vmservice_io,
_vmservice, _builtin, nativewrappers, and io.

The modifications are mostly because:

    - Patches should not be able to introduce new public members to a
      library's API.  In cases where the VM's patches introduce public
      members, the patches are rewritten to introduce private members
      instead.

    - Patches should not be able to replace arbitrary members, only
      ones declared external.  In cases where the VM's patches replace
      arbitrary members, those members are declared external in the
      SDK sources and dart2js's patch files are rewritten to use the
      original SDK implementation.

BUG=
R=sigmund@google.com

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

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

Committed patchset #1 (id:1) manually as
9ab1f22b414d9df100b8e964223498627d33f913 (presubmit successful).