Use a symbol for static length/name (conflicts with Function properties in ES5: cannot redefine Fun…

Use a symbol for static length/name (and other Function properties) for Closure, to avoid ES5->ES6 lowering bug (https://github.com/google/closure-compiler/issues/1460).

This is part of the overall "simple closure" effort (issue #312)

BUG=
R=jmesserly@google.com

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

[ochafik, 2016-01-26 14:14:12]

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

[ochafik, 2016-01-26 14:14:12]

Hey guys, this comes up on code that was lowered to ES5 with Closure:

// ES6
class Foo {
  static name() { ... }
  static length() { ... }
}
typeof Foo.name // function
typeof Foo.length // function

// ES5
function Foo() {}
Foo.name = function() { ... }
Foo.length = function() { ... }
typeof Foo.name // string
typeof Foo.length // number

(this messes up the dart.setSignature call)

[Jennifer Messerly, 2016-01-27 17:58:03]

This sounds like a bug in Closure Compiler. Our output is valid ES6, right?

(we do have a few invalid names for ES6 statics, those are in js_names
invalidStaticFieldName, but I was able to find those listed in the ES6 spec.)

[Jennifer Messerly, 2016-01-27 18:05:19]

On 2016/01/27 17:58:03, John Messerly wrote:
> This sounds like a bug in Closure Compiler. Our output is valid ES6, right?
> 
> (we do have a few invalid names for ES6 statics, those are in js_names
> invalidStaticFieldName, but I was able to find those listed in the ES6 spec.)

Hit send too fast. Couple of other thoughts:

I don't think Symbol is the right answer here. We have two specific uses of
those, for private members, and extension members.

It's possible to define almost any property on an Object, sometimes it just
means we can't define it using a particular syntax.

I tried this and it worked:

    Object.defineProperty(Foo, 'name', {value: function() { }})

that's also how we deal with "arguments" "caller" "callee"

Highly suspect here that the bug is in Closure, since our output works in ES6.
If you want a workaround when Closure output is turned on that would be okay.
But the workaround should be more along the lines of existing
arguments/caller/callee workaround. See test/codegen/names.dart

[ochafik, 2016-01-28 15:19:19]

Thanks John!

On 2016/01/27 18:05:19, John Messerly wrote:
> On 2016/01/27 17:58:03, John Messerly wrote:
> > This sounds like a bug in Closure Compiler. Our output is valid ES6, right?
> (...)
> Highly suspect here that the bug is in Closure, since our output works in ES6.
> If you want a workaround when Closure output is turned on that would be okay.
> But the workaround should be more along the lines of existing
> arguments/caller/callee workaround. See test/codegen/names.dart

There's definitely a bug in Closure (filed
https://github.com/google/closure-compiler/issues/1460), but...

While our output runs, I think overwriting Function properties feels wrong,
especially as some JS tools might be using classes as functions (debugging,
Angular's injection, etc), WDYT?
Left TODOs + fenced behind --closure flag.

> > 
> > (we do have a few invalid names for ES6 statics, those are in js_names
> > invalidStaticFieldName, but I was able to find those listed in the ES6
spec.)

Piggy-backed on invalidStaticFieldName (extended with closureCompiler mode), but
also had to add _topLevelExtensionNames (+ refactoring with
_emitExtensionNamesDeclaration, _emitExtensionName).

Not sure invalidStaticFieldName affects static methods much, btw.
 
> I don't think Symbol is the right answer here. We have two specific uses of
> those, for private members, and extension members.
> 
> It's possible to define almost any property on an Object, sometimes it just
> means we can't define it using a particular syntax.
> 
> I tried this and it worked:
> 
>     Object.defineProperty(Foo, 'name', {value: function() { }})
> 
> that's also how we deal with "arguments" "caller" "callee"

Note: we don't deal with those names when used for static methods, is it
expected?

  class Foo {
    static final arguments = 1; // lazy
    static const callee = 1;    // lazy
    static caller() => 1;       // not lazy
  }

[ochafik, 2016-01-28 15:21:38]

Description was changed from

==========
Use a symbol for static length/name (conflicts with Function properties in ES5:
cannot redefine Function.name/length properties) (issue #312)

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

to

==========
Use a symbol for static length/name (and other Function properties) for Closure,
to avoid ES5->ES6 lowering bug
(https://github.com/google/closure-compiler/issues/1460).

This is part of the overall "simple closure" effort (issue #312)

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

[Jennifer Messerly, 2016-01-28 19:52:09]

On 2016/01/28 15:19:19, ochafik wrote:
> Thanks John!
> 
> On 2016/01/27 18:05:19, John Messerly wrote:
> > On 2016/01/27 17:58:03, John Messerly wrote:
> > > This sounds like a bug in Closure Compiler. Our output is valid ES6,
right?
> > (...)
> > Highly suspect here that the bug is in Closure, since our output works in
ES6.
> > If you want a workaround when Closure output is turned on that would be
okay.
> > But the workaround should be more along the lines of existing
> > arguments/caller/callee workaround. See test/codegen/names.dart
> 
> There's definitely a bug in Closure (filed
> https://github.com/google/closure-compiler/issues/1460), but...
> 
> While our output runs, I think overwriting Function properties feels wrong,
> especially as some JS tools might be using classes as functions (debugging,
> Angular's injection, etc), WDYT?
> Left TODOs + fenced behind --closure flag.
> 
> > > 
> > > (we do have a few invalid names for ES6 statics, those are in js_names
> > > invalidStaticFieldName, but I was able to find those listed in the ES6
> spec.)
> 
> Piggy-backed on invalidStaticFieldName (extended with closureCompiler mode),
but
> also had to add _topLevelExtensionNames (+ refactoring with
> _emitExtensionNamesDeclaration, _emitExtensionName).
> 
> Not sure invalidStaticFieldName affects static methods much, btw.
>  
> > I don't think Symbol is the right answer here. We have two specific uses of
> > those, for private members, and extension members.
> > 
> > It's possible to define almost any property on an Object, sometimes it just
> > means we can't define it using a particular syntax.
> > 
> > I tried this and it worked:
> > 
> >     Object.defineProperty(Foo, 'name', {value: function() { }})
> > 
> > that's also how we deal with "arguments" "caller" "callee"
> 
> Note: we don't deal with those names when used for static methods, is it
> expected?
> 
>   class Foo {
>     static final arguments = 1; // lazy
>     static const callee = 1;    // lazy
>     static caller() => 1;       // not lazy
>   }

yeah, they may not be working in all cases--"invalidStaticFieldName" made me
suspicious too, what about static methods?

Names are so funny. Since we want to preserve the name whenever possible, it
forces us to have a really precise model of how names work in JS. So we get bugs
in both directions, we can't rename too much, and we can't generate code that
doesn't work either.

Anyway, I'll have a look at the CL.

[Jennifer Messerly, 2016-01-28 19:58:02]

On 2016/01/28 15:19:19, ochafik wrote:
> Thanks John!
> 
> On 2016/01/27 18:05:19, John Messerly wrote:
> > On 2016/01/27 17:58:03, John Messerly wrote:
> > > This sounds like a bug in Closure Compiler. Our output is valid ES6,
right?
> > (...)
> > Highly suspect here that the bug is in Closure, since our output works in
ES6.
> > If you want a workaround when Closure output is turned on that would be
okay.
> > But the workaround should be more along the lines of existing
> > arguments/caller/callee workaround. See test/codegen/names.dart
> 
> There's definitely a bug in Closure (filed
> https://github.com/google/closure-compiler/issues/1460), but...
> 
> While our output runs, I think overwriting Function properties feels wrong,
> especially as some JS tools might be using classes as functions (debugging,
> Angular's injection, etc), WDYT?
> Left TODOs + fenced behind --closure flag.

Maybe, but we've got to be careful. This is something that is allowed in ES5 and
ES6--it has nothing to do with Dart.

Changing our Dart compiler's output, in particular the API contract of the
output, just because some tools are making bad assumptions, is risky. Doesn't
mean we should never do it, but it needs to be thought through carefully.

I know we have a compiler, it's easy to hack on, and so it's tempting to use it
as our superpower to fix the world's JS problems :). That may very well be the
right answer, just something to think through. What looks like a very helpful
fix to an Angular user might look totally broken to some other user.

[Jennifer Messerly, 2016-01-28 20:03:37]

https://codereview.chromium.org/1630963003/diff/80001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1630963003/diff/80001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:3411: if (invalidStaticFieldName(name,
closureCompiler: true)) {
I still don't understand why were are using extension method Symbols.

In Chrome, right now, I run this JavaScript code:

    class Foo {
      static name() { return 42; }
    }
    console.log(Foo.name()); // 42

If I try the same thing in Dart, it won't work. I'll have to know about
"Foo[dartx.name]()"

If it helps, I can write up a CL to show the alternate approach? Maybe we should
discuss next design meeting?

[Jennifer Messerly, 2016-02-02 00:22:56]

LGTM, some comments on how it's factored, below... I'm fine with landing this as
is, and I can attempt to refactor it once it lands, if that works for you?

https://codereview.chromium.org/1630963003/diff/80001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1630963003/diff/80001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:163: if (_topLevelExtensionNames.isNotEmpty) {
I don't think this should be needed at the top-level.

Static methods/fields should only appear in a class, right? If it's not in a
class, then we don't need to worry about shadowing "Function".

It would be good to merge this in with the "dartxNames" code, that defines
extension members for a class.

It might need some light refactoring (given that extension members are currently
all or nothing for a class), but there's probably a clean way to factor it.

For example, we could rename this field to _extensionNames, initialize it when
we start generating a class, and then use it in _finishClassMembers. Some
benefits--I don't think we'll need to collect the names like it currently does,
and maybe "allowExtensions" bool can go away.

https://codereview.chromium.org/1630963003/diff/80001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:3411: if (invalidStaticFieldName(name,
closureCompiler: true)) {
On 2016/01/28 20:03:36, John Messerly wrote:
> If it helps, I can write up a CL to show the alternate approach? Maybe we
should
> discuss next design meeting?

We chatted about this, decided extension method symbols are okay for now.  (if
we look at statics the same as instance properties. In the future with Dart
metaclasses, that might be the case.)

So what I'd like to see here -- I feel like the "closureCompiler" part of this
workaround applies to certain names only.  I think this may be the right
approach:

if (isStatic) {
  // Avoid colliding with names on Function that are disallowed in ES6,
  // or where we need to work around transpilers.
  if (invalidStaticName(name, options)) {
    return _emitExtensionName(name);
  }
}

then in invalidStaticName, check for "closure" mode for the particular fields
we're worried about.

[ochafik, 2016-02-03 19:58:49]

Thanks John!

https://codereview.chromium.org/1630963003/diff/80001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1630963003/diff/80001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:163: if (_topLevelExtensionNames.isNotEmpty) {
On 2016/02/02 00:22:56, John Messerly wrote:
> I don't think this should be needed at the top-level.
> 
> Static methods/fields should only appear in a class, right? If it's not in a
> class, then we don't need to worry about shadowing "Function".
> 
> It would be good to merge this in with the "dartxNames" code, that defines
> extension members for a class.
> 
> It might need some light refactoring (given that extension members are
currently
> all or nothing for a class), but there's probably a clean way to factor it.
> 
> For example, we could rename this field to _extensionNames, initialize it when
> we start generating a class, and then use it in _finishClassMembers. Some
> benefits--I don't think we'll need to collect the names like it currently
does,
> and maybe "allowExtensions" bool can go away. 
> 

Great point! Leaving todo for follow up refactoring as you suggested

https://codereview.chromium.org/1630963003/diff/80001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:3411: if (invalidStaticFieldName(name,
closureCompiler: true)) {
On 2016/02/02 00:22:56, John Messerly wrote:
> On 2016/01/28 20:03:36, John Messerly wrote:
> > If it helps, I can write up a CL to show the alternate approach? Maybe we
> should
> > discuss next design meeting?
> 
> We chatted about this, decided extension method symbols are okay for now.  (if
> we look at statics the same as instance properties. In the future with Dart
> metaclasses, that might be the case.)
> 
> So what I'd like to see here -- I feel like the "closureCompiler" part of this
> workaround applies to certain names only.  I think this may be the right
> approach:
> 
> if (isStatic) {
>   // Avoid colliding with names on Function that are disallowed in ES6,
>   // or where we need to work around transpilers.
>   if (invalidStaticName(name, options)) {
>     return _emitExtensionName(name);
>   }
> }
> 
> then in invalidStaticName, check for "closure" mode for the particular fields
> we're worried about.

Done.

https://codereview.chromium.org/1630963003/diff/80001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:3411: if (invalidStaticFieldName(name,
closureCompiler: true)) {
On 2016/01/28 20:03:36, John Messerly wrote:
> I still don't understand why were are using extension method Symbols.
> 
> In Chrome, right now, I run this JavaScript code:
> 
>     class Foo {
>       static name() { return 42; }
>     }
>     console.log(Foo.name()); // 42
> 
> If I try the same thing in Dart, it won't work. I'll have to know about
> "Foo[dartx.name]()"
> 
> If it helps, I can write up a CL to show the alternate approach? Maybe we
should
> discuss next design meeting?
> 

Acknowledged.

[ochafik, 2016-02-03 20:25:11]

Description was changed from

==========
Use a symbol for static length/name (and other Function properties) for Closure,
to avoid ES5->ES6 lowering bug
(https://github.com/google/closure-compiler/issues/1460).

This is part of the overall "simple closure" effort (issue #312)

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

to

==========
Use a symbol for static length/name (and other Function properties) for Closure,
to avoid ES5->ES6 lowering bug
(https://github.com/google/closure-compiler/issues/1460).

This is part of the overall "simple closure" effort (issue #312)

BUG=
R=jmesserly@google.com

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

[ochafik, 2016-02-03 20:25:12]

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

[ochafik, 2016-02-03 22:48:04]

On 2016/02/03 20:25:12, ochafik wrote:
> Committed patchset #7 (id:120001) manually as
> a637e1b048487629d0d44620eb41e703531ce18d (presubmit successful).

Sorry, had to revert (broke regress_11010_test), preparing new version