Create local alias for super class in --closure mode (issue #312)

Create local alias for super class in --closure mode (issue #312)

Closure compiler chokes on super classes that aren't qualified paths.

(example: http://goo.gl/5mHC7S)

BUG=
R=jmesserly@google.com

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

[ochafik, 2016-01-26 14:17:42]

Description was changed from

==========
Create local alias for super class in --closure mode (issue #312)

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

to

==========
Create local alias for super class in --closure mode (issue #312)

Closure compiler chokes on super classes that aren't qualified paths.

(example: http://goo.gl/5mHC7S)

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

[ochafik, 2016-01-26 14:18:17]

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

[Jennifer Messerly, 2016-02-01 23:59:55]

(Somehow I missed this one in the craziness last week, sorry about that! feel
free to send a ping.)

This LGTM w/ comments addressed.

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

https://codereview.chromium.org/1638533004/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:381: return _finishClassDef(element.type,
_emitClassDeclaration(classExpr));
The same fix is likely needed for ClassTypeAlias

https://codereview.chromium.org/1638533004/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:678: JS.Statement
_emitClassDeclaration(JS.ClassExpression cls) {
from the naming convention, I would probably expect that this emits a Dart
ClassDeclaration.

Maybe _emitClassHeritageWorkaround

https://codereview.chromium.org/1638533004/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:679: var body = <JS.Statement>[];
this can go inside the if:

if (...) {
  return new JS.Block(<JS.Statement>[
    js.statement('const # = #;', [superVar, cls.heritage]),
    new JS.ClassExpression(cls.name, superVar, cls.methods)
  ]);
}
return new JS.ClassDeclaration(cls);

that way we aren't building up a List just to immediately throw it away for the
normal code path.

https://codereview.chromium.org/1638533004/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:682: // Workaround for Closure: super classes
must be qualified paths.
After the method rename, this would be good as a doc comment.

https://codereview.chromium.org/1638533004/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:683: var superVar = new
JS.Identifier(cls.name.name + r'$super');
This should be a temp. Use `new JS.TemporaryId`. That will let the compiler know
that it is safe to rename, in case it conflicts with any user-controlled names
in the Dart scope. JS.TemporaryId's use pointer identity to know whether it is a
use of the same variable.

[Jennifer Messerly, 2016-02-02 00:00:54]

On 2016/02/01 23:59:55, John Messerly wrote:
> (Somehow I missed this one in the craziness last week, sorry about that! feel
> free to send a ping.)

(And by "craziness" I mean, all of the Angular blocking bugs we were trying to
land fixes for.)

[ochafik, 2016-02-03 19:41:34]

Thanks John!

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

https://codereview.chromium.org/1638533004/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:381: return _finishClassDef(element.type,
_emitClassDeclaration(classExpr));
On 2016/02/01 23:59:55, John Messerly wrote:
> The same fix is likely needed for ClassTypeAlias

Done.

https://codereview.chromium.org/1638533004/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:678: JS.Statement
_emitClassDeclaration(JS.ClassExpression cls) {
On 2016/02/01 23:59:55, John Messerly wrote:
> from the naming convention, I would probably expect that this emits a Dart
> ClassDeclaration.
> 
> Maybe _emitClassHeritageWorkaround

Done.

https://codereview.chromium.org/1638533004/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:679: var body = <JS.Statement>[];
On 2016/02/01 23:59:55, John Messerly wrote:
> this can go inside the if:
> 
> if (...) {
>   return new JS.Block(<JS.Statement>[
>     js.statement('const # = #;', [superVar, cls.heritage]),
>     new JS.ClassExpression(cls.name, superVar, cls.methods)
>   ]);
> }
> return new JS.ClassDeclaration(cls);
> 
> that way we aren't building up a List just to immediately throw it away for
the
> normal code path.

Done.

https://codereview.chromium.org/1638533004/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:682: // Workaround for Closure: super classes
must be qualified paths.
On 2016/02/01 23:59:55, John Messerly wrote:
> After the method rename, this would be good as a doc comment.

Done.

https://codereview.chromium.org/1638533004/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:683: var superVar = new
JS.Identifier(cls.name.name + r'$super');
On 2016/02/01 23:59:55, John Messerly wrote:
> This should be a temp. Use `new JS.TemporaryId`. That will let the compiler
know
> that it is safe to rename, in case it conflicts with any user-controlled names
> in the Dart scope. JS.TemporaryId's use pointer identity to know whether it is
a
> use of the same variable.

Done.

[ochafik, 2016-02-03 20:21:17]

Description was changed from

==========
Create local alias for super class in --closure mode (issue #312)

Closure compiler chokes on super classes that aren't qualified paths.

(example: http://goo.gl/5mHC7S)

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

to

==========
Create local alias for super class in --closure mode (issue #312)

Closure compiler chokes on super classes that aren't qualified paths.

(example: http://goo.gl/5mHC7S)

BUG=
R=jmesserly@google.com

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

[ochafik, 2016-02-03 20:21:18]

Committed patchset #3 (id:40001) manually as
e693a004a7421a1d226123ecb246804d2b393ddf (presubmit successful).