Add a `@JSExportName` annotation for internal use in the runtime (use it to export dart.assert inst…

Add a `@JSExportName` annotation for internal use in the runtime (use it to export dart.assert instead of assert_)

BUG=
R=jmesserly@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/dbc8291f0156698d6ad75cecd031d7b750c49e02

[ochafik, 2016-01-13 11:07:36]

ochafik@google.com changed reviewers:
+ jmesserly@google.com, leafp@google.com, vsm@google.com

[ochafik, 2016-01-13 11:07:36]

Hey guys, I've split the JS annotation out of the "runtime in dart" CL and have
reduced its scope: please have a look :-)

[ochafik, 2016-01-13 12:31:59]

Description was changed from

==========
Add a `@JSExportName` annotation for internal use in the runtime (use it to
export dart.assert instead of assert_)

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

to

==========
Add a `@JSExportName` annotation for internal use in the runtime (use it to
export dart.assert instead of assert_)

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

[ochafik, 2016-01-13 12:31:59]

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

[Jennifer Messerly, 2016-01-13 17:44:00]

A few comments but otherwise this LGTM!

https://codereview.chromium.org/1580413002/diff/100001/lib/src/codegen/js_cod...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1580413002/diff/100001/lib/src/codegen/js_cod...
lib/src/codegen/js_codegen.dart:1599: return _emitTopLevelName(element);
nice! it's funny how a name change makes such a difference to how the code reads
:)

https://codereview.chromium.org/1580413002/diff/100001/lib/src/codegen/js_cod...
lib/src/codegen/js_codegen.dart:1781: var nameExpr = _propertyName(name ??
_getJSExportName(e) ?? e.name);
oh wow, the [name] optional parameter--which was probably my fault--is really
hacky. I think it breaks your JSExportName logic on generic classes.

It seems like it should maybe just be a suffix. That would allow you to rename a
generic class def, and it would generate as the exported name followed by '$'.
So this line would have:

    _propertyName((_getJSExportName(e) ?? e.name) + suffix)

and the call 10 lines above would be like:

    _emitTopLevelName(element, suffix: r'$')

https://codereview.chromium.org/1580413002/diff/100001/lib/src/codegen/js_cod...
lib/src/codegen/js_codegen.dart:2244: var fieldName = _getJSExportName(element)
?? field.name.name;
So for top-level fields, I think this works.

I worry it's in a slightly strange place for static fields, because we'll
generate uses of them using the Element.name, but we'll generate definitions
using the @JSExportName name. So if the dart code  is:

class Foo {
  @JSExportName('foo')
  static int bar = 42;
}
main() => print(Foo.bar);

I think it will generate the definition as 'Foo.foo' but the use as 'Foo.bar'

Huh, and looking at _emitMemberName, it's not easy to fix -- we don't always
have an Element when we need to emit a member name.

Probably the safest thing to do for now is only have this logic kick in for
top-level fields, something like:

var fieldName = field.name.name;
if (element is TopLevelVariableElement) {
  String exportName = _getJSExportName(element);
  if (exportName != null) fieldName = exportName;
}
...

(aside: we might be in a good place now with the Analyzer, where we can rely on
Elements being correctly populated for things like
MethodInvocation.methodName.staticElement. But that wasn't the case in the
past.)

[ochafik, 2016-01-13 18:51:29]

Thanks John!

https://codereview.chromium.org/1580413002/diff/100001/lib/src/codegen/js_cod...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1580413002/diff/100001/lib/src/codegen/js_cod...
lib/src/codegen/js_codegen.dart:1599: return _emitTopLevelName(element);
On 2016/01/13 17:43:59, John Messerly wrote:
> nice! it's funny how a name change makes such a difference to how the code
reads
> :)

Yeah, *sounds* so much more deterministic now ;-)

https://codereview.chromium.org/1580413002/diff/100001/lib/src/codegen/js_cod...
lib/src/codegen/js_codegen.dart:1781: var nameExpr = _propertyName(name ??
_getJSExportName(e) ?? e.name);
On 2016/01/13 17:44:00, John Messerly wrote:
> oh wow, the [name] optional parameter--which was probably my fault--is really
> hacky. I think it breaks your JSExportName logic on generic classes.
> 
> It seems like it should maybe just be a suffix. That would allow you to rename
a
> generic class def, and it would generate as the exported name followed by '$'.
> So this line would have:
> 
>     _propertyName((_getJSExportName(e) ?? e.name) + suffix)
> 
> and the call 10 lines above would be like:
> 
>     _emitTopLevelName(element, suffix: r'$')

Cool! Done.

https://codereview.chromium.org/1580413002/diff/100001/lib/src/codegen/js_cod...
lib/src/codegen/js_codegen.dart:2244: var fieldName = _getJSExportName(element)
?? field.name.name;
On 2016/01/13 17:43:59, John Messerly wrote:
> So for top-level fields, I think this works.
> 
> I worry it's in a slightly strange place for static fields, because we'll
> generate uses of them using the Element.name, but we'll generate definitions
> using the @JSExportName name. So if the dart code  is:
> 
> class Foo {
>   @JSExportName('foo')
>   static int bar = 42;
> }
> main() => print(Foo.bar);
> 
> I think it will generate the definition as 'Foo.foo' but the use as 'Foo.bar'
> 
> Huh, and looking at _emitMemberName, it's not easy to fix -- we don't always
> have an Element when we need to emit a member name.
> 
> Probably the safest thing to do for now is only have this logic kick in for
> top-level fields, something like:
> 
> var fieldName = field.name.name;
> if (element is TopLevelVariableElement) {
>   String exportName = _getJSExportName(element);
>   if (exportName != null) fieldName = exportName;
> }
> ...
> 
> (aside: we might be in a good place now with the Analyzer, where we can rely
on
> Elements being correctly populated for things like
> MethodInvocation.methodName.staticElement. But that wasn't the case in the
> past.)

Aaaah, absolutely, thanks!

[Jennifer Messerly, 2016-01-19 17:33:17]

LGTM :)

[ochafik, 2016-01-19 17:51:22]

Description was changed from

==========
Add a `@JSExportName` annotation for internal use in the runtime (use it to
export dart.assert instead of assert_)

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

to

==========
Add a `@JSExportName` annotation for internal use in the runtime (use it to
export dart.assert instead of assert_)

BUG=
R=jmesserly@google.com

Committed:
https://github.com/dart-lang/dev_compiler/commit/dbc8291f0156698d6ad75cecd031...
==========

[ochafik, 2016-01-19 17:51:24]

Committed patchset #8 (id:140001) manually as
dbc8291f0156698d6ad75cecd031d7b750c49e02 (presubmit successful).