qualify core types: Object, Error, Symbol

qualify core types: Object, Error, Symbol
this avoids colliding with global names
the old output was valid but some tools didn't understand it, see #312

[Jennifer Messerly, 2015-09-14 23:54:13]

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

[Jennifer Messerly, 2015-09-14 23:55:59]

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

[Jennifer Messerly, 2015-09-15 16:31:17]

Oh, and this also removes the JsSymbol workaround :)

[ochafik, 2015-09-15 16:58:33]

Hey John, thanks for giving it a go! Some quick comments :-)

https://codereview.chromium.org/1341963003/diff/1/lib/runtime/dart/_internal.js
File lib/runtime/dart/_internal.js (right):

https://codereview.chromium.org/1341963003/diff/1/lib/runtime/dart/_internal....
lib/runtime/dart/_internal.js:2320: class Symbol extends core.Object {
Note that this pattern will likely break closure's inlining in advanced mode.
The alternative:

  goog.module('lib');
  exports.Symbol = class Symbol {
    foo(x) { console.log(x); }
  }
  new exports.Symbol().foo(10);

Would inline foo completely. Compare the outputs:
- Inlined: http://goo.gl/VqfUDE
- Not inlined: http://goo.gl/3Vlm1q

(but it would require a bit more exports.Symbol references in the
dart.defineNamedConstructor statements and alike)

https://codereview.chromium.org/1341963003/diff/1/lib/runtime/dart/core.js
File lib/runtime/dart/core.js (right):

https://codereview.chromium.org/1341963003/diff/1/lib/runtime/dart/core.js#ne...
lib/runtime/dart/core.js:1479: class Map extends exports.Object {
This one will be seen as a redefine in ES6 as well, I think.

https://codereview.chromium.org/1341963003/diff/1/lib/src/codegen/js_codegen....
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1341963003/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:1689: case 'Symbol':
Presumably also Map, Set, TypeError, RangeError, RegExp...

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects

[Jennifer Messerly, 2015-09-15 17:06:09]

https://codereview.chromium.org/1341963003/diff/1/lib/runtime/dart/_internal.js
File lib/runtime/dart/_internal.js (right):

https://codereview.chromium.org/1341963003/diff/1/lib/runtime/dart/_internal....
lib/runtime/dart/_internal.js:2320: class Symbol extends core.Object {
On 2015/09/15 16:58:33, ochafik wrote:
> Note that this pattern will likely break closure's inlining in advanced mode.
> The alternative:
> 
>   goog.module('lib');
>   exports.Symbol = class Symbol {
>     foo(x) { console.log(x); }
>   }
>   new exports.Symbol().foo(10);
> 
> Would inline foo completely. Compare the outputs:
> - Inlined: http://goo.gl/VqfUDE
> - Not inlined: http://goo.gl/3Vlm1q
> 
> (but it would require a bit more exports.Symbol references in the
> dart.defineNamedConstructor statements and alike)

Right. I don't think that we have a goal at the present time to support advanced
optimizations, hence that is not part of my thinking when it comes to DDC's
output.

(and fwiw, an immediately invoked lambda shouldn't be very hard for a compiler
to analyze. I suspect they just never had need to do it.)

https://codereview.chromium.org/1341963003/diff/1/lib/src/codegen/js_codegen....
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1341963003/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:1689: case 'Symbol':
On 2015/09/15 16:58:33, ochafik wrote:
> Presumably also Map, Set, TypeError, RangeError, RegExp...
> 
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects

Maybe I need to step back, I honestly don't have a good model of why we are
avoiding global names. What are the reasons?

In some sense, the point of ES6 modules are to bring sanity to the ES scoping
world.

Unless the code in question needs to use one of them, there should be no reason
to avoid that as a local variable name.

In general, we can't guarantee ES won't add more names in the future (e.g. Map
and Set are new).

[ochafik, 2015-09-15 18:05:17]

Thanks John, added some explanations on goals + issues (basically, the
_maybeQualifiedName part of this patchset fixes an error, but a warning
remains).

https://codereview.chromium.org/1341963003/diff/1/lib/runtime/dart/_internal.js
File lib/runtime/dart/_internal.js (right):

https://codereview.chromium.org/1341963003/diff/1/lib/runtime/dart/_internal....
lib/runtime/dart/_internal.js:2320: class Symbol extends core.Object {
On 2015/09/15 17:06:09, John Messerly wrote:
> On 2015/09/15 16:58:33, ochafik wrote:
> > Note that this pattern will likely break closure's inlining in advanced
mode.
> > The alternative:
> > 
> >   goog.module('lib');
> >   exports.Symbol = class Symbol {
> >     foo(x) { console.log(x); }
> >   }
> >   new exports.Symbol().foo(10);
> > 
> > Would inline foo completely. Compare the outputs:
> > - Inlined: http://goo.gl/VqfUDE
> > - Not inlined: http://goo.gl/3Vlm1q
> > 
> > (but it would require a bit more exports.Symbol references in the
> > dart.defineNamedConstructor statements and alike)
> 
> Right. I don't think that we have a goal at the present time to support
advanced
> optimizations, hence that is not part of my thinking when it comes to DDC's
> output.

Advanced optimizations are my main goal (to let Closure size-optimize DDC's
output), with "simple" optimizations (and ES5 lowering for free as a
side-effect) being a milestone on that path. Maybe if we split out a
ClassBuilder interface we can resurrect the idea of having two backends with
different goals, in which case the "normal" codegen wouldn't even have to deal
with Closure's simple mode.

> (and fwiw, an immediately invoked lambda shouldn't be very hard for a compiler
> to analyze. I suspect they just never had need to do it.)

I would also suspect so :-)

https://codereview.chromium.org/1341963003/diff/1/lib/src/codegen/js_codegen....
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1341963003/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:550: if (_isJsBuiltinName(name)) {
nit: store as a var above for reuse below?

https://codereview.chromium.org/1341963003/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:552: [_libraryName(type.element.library), name,
genericInst]);
nit: isn't this guaranteed to be currentLibrary?

https://codereview.chromium.org/1341963003/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:1673: _isJsBuiltinName(e.name)) {
I think that check alone (in the context of "extends exports.Object") fixes the
Closure compilation error that occurs in core.js.

https://codereview.chromium.org/1341963003/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:1689: case 'Symbol':
On 2015/09/15 17:06:09, John Messerly wrote:
> On 2015/09/15 16:58:33, ochafik wrote:
> > Presumably also Map, Set, TypeError, RangeError, RegExp...
> > 
> >
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects
> 
> Maybe I need to step back, I honestly don't have a good model of why we are
> avoiding global names. What are the reasons?

Right now it's just to avoid errors in Closure with the following code, which I
believe your CL already fixes:

`class Object {}; class Sub extends Object {}`

Just defining `class Object {}` yields a mere warning, but extending `Object`
gives an error (extending `core.Object` apparently does the trick, but the
warning remains: http://goo.gl/AqXiqr).

> In some sense, the point of ES6 modules are to bring sanity to the ES scoping
> world.
 
Agreed, can't wait for https://github.com/whatwg/loader to succeed :-)
Note that we could also just ask the Closure Compiler team to remove those
warnings/errors when classes are redefined in a local scope (but again, I am /
was biased towards qualification, which is why I didn't ask them yet).

> Unless the code in question needs to use one of them, there should be no
reason
> to avoid that as a local variable name.
> 
> In general, we can't guarantee ES won't add more names in the future (e.g. Map
> and Set are new). 

This is why I would advocate to generalize qualified names for all types, but
maybe then the overhead of these extra function blocks could start becoming an
issue.

https://codereview.chromium.org/1341963003/diff/1/lib/src/js/nodes.dart
File lib/src/js/nodes.dart (right):

https://codereview.chromium.org/1341963003/diff/1/lib/src/js/nodes.dart#newco...
lib/src/js/nodes.dart:999: visitFunctionExpression(FunctionExpression node) {
Does FunctionExpression include arrow functions? (if so, they shouldn't increase
the nesting?)

(also, did you mean to include this change in this patchset?)

[Jennifer Messerly, 2015-09-15 18:34:23]

thanks! this is a good discussion. helpful to understand what some of the
different goals and perspectives are.

https://codereview.chromium.org/1341963003/diff/1/lib/runtime/dart/_internal.js
File lib/runtime/dart/_internal.js (right):

https://codereview.chromium.org/1341963003/diff/1/lib/runtime/dart/_internal....
lib/runtime/dart/_internal.js:2320: class Symbol extends core.Object {
On 2015/09/15 18:05:17, ochafik wrote:
> On 2015/09/15 17:06:09, John Messerly wrote:
> > On 2015/09/15 16:58:33, ochafik wrote:
> > > Note that this pattern will likely break closure's inlining in advanced
> mode.
> > > The alternative:
> > > 
> > >   goog.module('lib');
> > >   exports.Symbol = class Symbol {
> > >     foo(x) { console.log(x); }
> > >   }
> > >   new exports.Symbol().foo(10);
> > > 
> > > Would inline foo completely. Compare the outputs:
> > > - Inlined: http://goo.gl/VqfUDE
> > > - Not inlined: http://goo.gl/3Vlm1q
> > > 
> > > (but it would require a bit more exports.Symbol references in the
> > > dart.defineNamedConstructor statements and alike)
> > 
> > Right. I don't think that we have a goal at the present time to support
> advanced
> > optimizations, hence that is not part of my thinking when it comes to DDC's
> > output.
> 
> Advanced optimizations are my main goal (to let Closure size-optimize DDC's
> output), with "simple" optimizations (and ES5 lowering for free as a
> side-effect) being a milestone on that path. Maybe if we split out a
> ClassBuilder interface we can resurrect the idea of having two backends with
> different goals, in which case the "normal" codegen wouldn't even have to deal
> with Closure's simple mode.

hmm, yeah, let's discuss tomorrow?

Generally, I feel like size of DDC's output is mainly going to be addressed in
the JS way: by structuring libraries so you as the developer have insight and
control over your size. That seems to be the favored approach in the JS
ecosystem, and mostly preferred over accepting the constraints that
advanced_opts imposes.

Tree-shaking is temping, but, we already have that: dart2js. Hard to see how
DDC+Closure_Advanced would be better than dart2js at this.

Regarding inlining: it's usually a throughput optimization. It typically makes
code size worse. (Compilers often try to limit this by having a max code size
increase they're willing to tolerate.)

But yeah, I think a high level "DartClass node" which can be lowered differently
would be good.

I'm definitely in favor of multiple back ends. Ideally the flow would be like:

    Dart AST --[js_codegen]--> "high level" JS AST --[specific JS flavor
backend]--> JS file on disk.

Where some JS flavors might be: ES6, TypeScript, ClosureSimple, ClosureAdvanced.
 Somewhat independently, since ES6 modules aren't there yet, we'll probably have
another step for lowering the modules to something like: AMD, commonjs.

What worries me a bit is ClosureAdvanced because it seems to impose a lot of
constraints that typical JS code does not follow, and that could lead us away
from our readable/idiomatic JS goals...

> > (and fwiw, an immediately invoked lambda shouldn't be very hard for a
compiler
> > to analyze. I suspect they just never had need to do it.)
> 
> I would also suspect so :-)

Yeah, maybe another perspective to share... I think of DDC as kind of a new
project, trying to get off the ground really fast with a small team. That's why
I tend to be biased in favor of tradeoffs that let us go fast and keep the
compiler very small and hackable. e.g. I don't think we should try and work
around Closure limitations or bugs. IMO, we just don't have that kind of
bandwidth, and I think we can afford to wait for Closure's ES6 support to reach
a good place. (Workarounds like that would end up being dead code anyway.)

https://codereview.chromium.org/1341963003/diff/1/lib/src/codegen/js_codegen....
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1341963003/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:1689: case 'Symbol':
On 2015/09/15 18:05:17, ochafik wrote:
> On 2015/09/15 17:06:09, John Messerly wrote:
> > On 2015/09/15 16:58:33, ochafik wrote:
> > > Presumably also Map, Set, TypeError, RangeError, RegExp...
> > > 
> > >
> >
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects
> > 
> > Maybe I need to step back, I honestly don't have a good model of why we are
> > avoiding global names. What are the reasons?
> 
> Right now it's just to avoid errors in Closure with the following code, which
I
> believe your CL already fixes:
> 
> `class Object {}; class Sub extends Object {}`
> 
> Just defining `class Object {}` yields a mere warning, but extending `Object`
> gives an error (extending `core.Object` apparently does the trick, but the
> warning remains: http://goo.gl/AqXiqr).
> 
> > In some sense, the point of ES6 modules are to bring sanity to the ES
scoping
> > world.
>  
> Agreed, can't wait for https://github.com/whatwg/loader to succeed :-)
> Note that we could also just ask the Closure Compiler team to remove those
> warnings/errors when classes are redefined in a local scope (but again, I am /
> was biased towards qualification, which is why I didn't ask them yet).
> 
> > Unless the code in question needs to use one of them, there should be no
> reason
> > to avoid that as a local variable name.
> > 
> > In general, we can't guarantee ES won't add more names in the future (e.g.
Map
> > and Set are new). 
> 
> This is why I would advocate to generalize qualified names for all types, but
> maybe then the overhead of these extra function blocks could start becoming an
> issue.

ah, we try and qualify the minimum possible. In fact more is qualified at
present than is desirable. One of the biggest readability wins will be able to
unqualify names from other libraries--just like in Dart code (unless it uses
`as` in which case we'd keep it qualified)