Fix noSuchMethod handling of methods that are also extension methods. Fix noSuchMethod handling of …

Fix noSuchMethod handling of methods that are also extension methods. Fix noSuchMethod handling of methods with names that are rewritten in DDC e.g. "_get" instead of "[]" Fix some mirror system handling of symbols with names that are rewritten in DDC

BUG=
R=jmesserly@google.com

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

[Jacob, 2017-01-11 03:10:39]

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

[Jacob, 2017-01-11 03:10:39]

https://codereview.chromium.org/2623053004/diff/1/pkg/dev_compiler/tool/input...
File pkg/dev_compiler/tool/input_sdk/private/js_mirrors.dart (right):

https://codereview.chromium.org/2623053004/diff/1/pkg/dev_compiler/tool/input...
pkg/dev_compiler/tool/input_sdk/private/js_mirrors.dart:105: return name;
Even without the code_generator.dart change, this code was broken due to symbols
generated by the regular dcalls.

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

one question/suggestion but LGTM

https://codereview.chromium.org/2623053004/diff/1/pkg/dev_compiler/lib/src/co...
File pkg/dev_compiler/lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/2623053004/diff/1/pkg/dev_compiler/lib/src/co...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:1508:
_declareMemberName(method, useDisplayName: true),
it might be good to just change InovcationImpl rather than code generation? I
only ask because poor _declareMemberName/_emitMemberName are constantly on the
verge of too much complexity to use correctly. That said this one here looks
quite safe.

do the other code paths where we create InvocationImpl also know to invert the
name?

[Jacob, 2017-01-21 00:44:15]

On 2017/01/11 03:25:44, Jennifer Messerly wrote:
> one question/suggestion but LGTM
> 
>
https://codereview.chromium.org/2623053004/diff/1/pkg/dev_compiler/lib/src/co...
> File pkg/dev_compiler/lib/src/compiler/code_generator.dart (right):
> 
>
https://codereview.chromium.org/2623053004/diff/1/pkg/dev_compiler/lib/src/co...
> pkg/dev_compiler/lib/src/compiler/code_generator.dart:1508:
> _declareMemberName(method, useDisplayName: true),
> it might be good to just change InovcationImpl rather than code generation? I
> only ask because poor _declareMemberName/_emitMemberName are constantly on the
> verge of too much complexity to use correctly. That said this one here looks
> quite safe.
> 
> do the other code paths where we create InvocationImpl also know to invert the
> name?

Long story short that is indeed and issue and fixing that issue exposed one
issue after another.
This CL now includes full support for NoSuchMethod with correct names, correct
implementation of dget, dput and dsend that perform the required type checks
before allowing DDC runtime objects to be corrupted.
As part of this had to fix a bunch of holes in how class metadata was stored and
propagated.

JS interop behavior is now more consistent with Dart2JS for the dynamic case
although we may want to put back the old behavior as a DDC optional flag for
people who don't care about Dart2JS.

[Jacob, 2017-01-21 01:11:30]

https://codereview.chromium.org/2623053004/diff/20001/pkg/dev_compiler/tool/i...
File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/classes.dart (left):

https://codereview.chromium.org/2623053004/diff/20001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/classes.dart:185:
getMethodTypeFromType(type, name) => JS(
removed the getMethodType call that used an object so all versions now just take
a type.
We generally have a type lying around when we call these methods so it is
simpler and more efficient
to always pass the type instead of the object.

https://codereview.chromium.org/2623053004/diff/20001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/classes.dart:434: let
originalSigFn = $getOwnPropertyDescriptor($dartExtType, $_methodSig).get;
We were forgetting to call updateSig on anything but methods. Same issue bellow.

https://codereview.chromium.org/2623053004/diff/20001/pkg/dev_compiler/tool/i...
File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/classes.dart (right):

https://codereview.chromium.org/2623053004/diff/20001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/classes.dart:57: fields: ()
=> {
hole in propagating  metadata

https://codereview.chromium.org/2623053004/diff/20001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/classes.dart:181: final
_staticGetterSig = JS('', 'Symbol("sigStaticGetter")');
Ran the formatter. Sorry about the formatter related delta.

https://codereview.chromium.org/2623053004/diff/20001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/classes.dart:284: // We need
to set the root proto to null not undefined.
This bug resulted in our type metadata looking like it contained the fields on
the JS Object class.

https://codereview.chromium.org/2623053004/diff/20001/pkg/dev_compiler/tool/i...
File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/types.dart (right):

https://codereview.chromium.org/2623053004/diff/20001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/types.dart:185: abstract
class AbstractFunctionType extends TypeRep {
Switched these to be first class Dart types as DDC has improved to the point
where that is feasible
as long as constructors are kept as pure JS code.

https://codereview.chromium.org/2623053004/diff/20001/pkg/dev_compiler/tool/i...
File pkg/dev_compiler/tool/input_sdk/private/js_mirrors.dart (right):

https://codereview.chromium.org/2623053004/diff/20001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/js_mirrors.dart:89: var name =
getName(symbol);
only relevant change in this file. Rest is just that I unfortunately ran the
formatter.

https://codereview.chromium.org/2623053004/diff/20001/tests/corelib_strong/dy...
File tests/corelib_strong/dynamic_nosuchmethod_test.dart (right):

https://codereview.chromium.org/2623053004/diff/20001/tests/corelib_strong/dy...
tests/corelib_strong/dynamic_nosuchmethod_test.dart:32: main() {
existing tests weren't designed to poke holes in the way DDC implements
noSuchMethod. Lots of these tests previously failed.

https://codereview.chromium.org/2623053004/diff/80001/pkg/dev_compiler/tool/i...
File pkg/dev_compiler/tool/input_sdk/private/js_mirrors.dart (right):

https://codereview.chromium.org/2623053004/diff/80001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/js_mirrors.dart:95: case '[]=':
this is the only change. The rest is just due to accidentally running the
formatter.

[Jennifer Messerly, 2017-01-24 03:51:03]

lgtm

https://codereview.chromium.org/2623053004/diff/80001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/2623053004/diff/80001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:1170: // TODO(vsm): Remove
was this code copied from somewhere else?

https://codereview.chromium.org/2623053004/diff/80001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:1181: var tInstanceFields
= <JS.Property>[new JS.Property(_emitMemberName('index'),
_emitAnnotatedType(intClass.type, null))];
format?

https://codereview.chromium.org/2623053004/diff/80001/pkg/dev_compiler/tool/i...
File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart
(right):

https://codereview.chromium.org/2623053004/diff/80001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart:54: var
setter = getSetterType(type, f);
Possibly calling this setterType would be a bit more clear what's going on.

"type" could be "objType" if you wanted to disambiguate further

https://codereview.chromium.org/2623053004/diff/80001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart:63: //
TODO(jacobr): propagate metadata tracking which fields are private.
What does this mean? It shouldn't be possible to get ahold of a the symbol for a
private field, right?

https://codereview.chromium.org/2623053004/diff/80001/pkg/dev_compiler/tool/i...
File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/types.dart (right):

https://codereview.chromium.org/2623053004/diff/80001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/types.dart:185: abstract
class AbstractFunctionType extends TypeRep {
I presume these are mostly indentation changes & JS->Dart porting?

[Jacob, 2017-01-24 19:12:33]

https://codereview.chromium.org/2623053004/diff/80001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/2623053004/diff/80001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:1170: // TODO(vsm): Remove
On 2017/01/24 03:51:03, Jennifer Messerly wrote:
> was this code copied from somewhere else?

That is scary. I had moved that to a shared helper but the change to lost. Fixed
again.

https://codereview.chromium.org/2623053004/diff/80001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:1181: var tInstanceFields
= <JS.Property>[new JS.Property(_emitMemberName('index'),
_emitAnnotatedType(intClass.type, null))];
On 2017/01/24 03:51:03, Jennifer Messerly wrote:
> format?

fixed.

https://codereview.chromium.org/2623053004/diff/80001/pkg/dev_compiler/tool/i...
File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart
(right):

https://codereview.chromium.org/2623053004/diff/80001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart:54: var
setter = getSetterType(type, f);
On 2017/01/24 03:51:03, Jennifer Messerly wrote:
> Possibly calling this setterType would be a bit more clear what's going on.
> 
> "type" could be "objType" if you wanted to disambiguate further

agreed. cleaned up.

https://codereview.chromium.org/2623053004/diff/80001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart:63: //
TODO(jacobr): propagate metadata tracking which fields are private.
On 2017/01/24 03:51:03, Jennifer Messerly wrote:
> What does this mean? It shouldn't be possible to get ahold of a the symbol for
a
> private field, right?
Done.
It means I got distracted writing the comment and wrote private when I meant to
write final instead of private.

https://codereview.chromium.org/2623053004/diff/80001/pkg/dev_compiler/tool/i...
File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/types.dart (right):

https://codereview.chromium.org/2623053004/diff/80001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/types.dart:185: abstract
class AbstractFunctionType extends TypeRep {
On 2017/01/24 03:51:03, Jennifer Messerly wrote:
> I presume these are mostly indentation changes & JS->Dart porting?


Yep. All just indentation changes and adding enough calls to the JS regular
macro
syntax that slow paths are avoided.

The key change here is now dynamic calls can be made against the TypeRep classes
as they
have the correct metadata which keeps the dart:mirrors implementation working.

[Jacob, 2017-01-24 19:16:30]

Description was changed from

==========
Fix noSuchMethod handling of methods that are also extension methods. Fix
noSuchMethod handling of methods with names that are rewritten in DDC e.g.
"_get" instead of "[]" Fix some mirror system handling of symbols with names
that are rewritten in DDC

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

to

==========
Fix noSuchMethod handling of methods that are also extension methods. Fix
noSuchMethod handling of methods with names that are rewritten in DDC e.g.
"_get" instead of "[]" Fix some mirror system handling of symbols with names
that are rewritten in DDC

BUG=
R=jmesserly@google.com

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

[Jacob, 2017-01-24 19:16:31]

Committed patchset #7 (id:120001) manually as
d90ed8c295da98b98e81f8abd97c5e51b499ce23 (presubmit successful).