Qualified exports and inheritance for Closure

[ochafik]

Hey guys, I know this is a big chunk of changes, sorry about that! (introducing properly qualified exports without doing some refactoring otherwise ended up in very unmaintainable code :-S).

This fixes two blockers for issue #312 (redefining Object class and other similar collisions with JS classes, and extending a non-qualifier class expression such as dart.mixin(...)).

Biggest changes in js_codegen.dart:

• Qualify exports when --closure: declarations are now emitted by method _emitDecl, which knows when to export things, and exports is renamed to the lib's temp name (e.g. mylib.Foo = class Foo {} instead of class Foo {}; exports.Foo = Foo).
• Simplified class building with _ClassBuilder helper (accumulates body, statements & preludes, then gets composed to top-level statements / lazy wrappers in one go). This makes it easier to switch to fully-qualified names in all the statements that go with a class definition. Updated the lazy class logic accordingly.
• Refactored visitClassDeclaration to make it (hopefully) easier to read with the new builder mechanism

This gives minor changes in generated code: shuffled symbols & JS iterator method moved to the end of classes (+ some exports.References are now local (fixed)).

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please let us know the company's name.

[googlebot]

CLAs look good, thanks!

[jmesserly]

@ochafik were you familiar with JS.MaybeQualifiedId? it's an expression that can add qualification if needed. It's already used for classes, needed because laziness decision can happen after we've emitted references in method bodies. I think that would remove the need for extensive "visitClassDeclaration" refactoring.

Why mylib. instead of exports.? We got the exports pattern from other JS module patterns. It's also a temp that can be renamed. mylib can conflict with names inside the module. We actually saw that happen (with one of the SDK libraries, if I recall correctly.)

As an aside, I don't think there's any problem with let Foo = exports.Foo = class Foo {} vs class Foo {}; exports.Foo = Foo;, so maybe we could use that always. We got the latter from Babel, for what it's worth: https://babeljs.io/repl/#?experimental=false&evaluate=true&loose=false&spec=false&code=export%20class%20Foo%20%7B%7D%0A

One more high level comment: I may try and resurrect ES6 Module AST nodes. I had created some nodes for ES6 Module/ModuleItem/Import/Export. The idea is that js_codegen could emit these high level nodes, then we can lower them appropriately. That would give you an easy way to make tweaks to how the module layout looks in Closure.

[ochafik]

Hey John, thanks for the comments!

Unfortunately let Object = exports.Object = class Object {}; won't work (conflicts with JavaScript's Object), so I think the need for qualified identifier for namespace purposes does not align with the current use-case of JS.MaybeQualifiedId (lazy + ref order). But... if we always qualified names and defined them directly on exports, I guess things would be much simpler... Can have a try if it sounds good to you :-)

Re/ exports, I seems best to keep it as it is indeed (but using a JS.TemporaryId should make it safe from conflicts, shouldn't it?), I just felt it was easier to read (I'm infected by the systematic qualified namespaces of pre-modules Closure code :-D).

Re/ modules, I've played with goog.module in branch closure-modules: I think it's the most pragmatic / least costly option for us right now (but we either need to write our runtime in Dart, or to commit to using it as our single module system).

[jmesserly]

Unfortunately let Object = exports.Object = class Object {}; won't work

oh sorry... I didn't explain that well. I meant we could use that pattern for most things, regardless of closure or not-closure. Naturally we would avoid it for Object.

I just felt it was easier to read

yeah agree it's more readable. Maybe we should do it always? Yeah, good point, we can keep it a temp. As long as it's okay w/ Closure if it gets renamed sometimes (e.g. mylib$)

I think it's the most pragmatic / least costly option for us right now

not sure I follow this. I don't think we can switch to goog.module, it seems to put us at odds with the rest of the JS ecosystem we're trying to interop with. Maybe you mean only for closure output?

[jmesserly]

But... if we always qualified names and defined them directly on exports, I guess things would be much simpler... Can have a try if it sounds good to you :-)

yeah definitely. I think you would only need to force it to qualify Object/Error/Symbol? I'm wondering if we could tackle this with something really simple ... like tweak _maybeQualifiedName to check for those types and force them down the qualified name path. We already do that for mutable top-level fields (which are always qualified). That plus the fix to use ClassExpression instead of ClassDeclaration for those 3 types, and should be good to go?

[jmesserly]

I'm wondering if we could tackle this with something really simple ... like tweak _maybeQualifiedName to check for those types and force them down the qualified name path.

went ahead and did it: https://codereview.chromium.org/1341963003/

[jmesserly]

Aside: it might be worth refactoring out a high level class node. Like ClassInfo/Builder. So we can represent concepts of a generic class and method signatures, without containing the policy of exactly how those get emitted. Ideally that could be in its own CL, so we can review the refactoring without any changes in output or new features.

[jmesserly]

I'm about to look at super mixins, which might involve tweaking how classes are generated. Discussing with @leafpetersen ... it sounds like it would be good to merge this first, that way we won't cause merge conflicts. Plus the classbuilder refactor is really nice IMO and will likely help.

Thanks @ochafik! we should definitely keep discussing the backend design (e.g. your design doc) but I think whatever we choose this pull req is a step in the right direction.

[jmesserly]

doh, it looks like this may have broken Travis, possibly(?) due to the language tests we added in the meantime. I'm going to see if I can reproduce locally, hopefully it's easy to fix and doesn't need a rollback.

[jmesserly]

build link: https://travis-ci.org/dart-lang/dev_compiler/builds/82198983

[jmesserly]

the only failure seems to be language private2_test.dart:

  dart.defineLazyClass(exports, {
    get C() {
      class C extends private2_lib.B {
        C() {
          super.B();
        }
      }
      dart.setSignature(C, {
        constructors: () => ({C: [C, []]})
      });
      return C;
    }
  });
  function main() {
    let a = new A();
    core.print(a.g);
    core.print(a[_f]);
    let o = new C();
    core.print(o.g);
    core.print(o[_f]);
  }

it looks like we generate the new C as unqualified instead of using a MaybeQualifiedId

[jmesserly]

Doh, I think this is a bit tricky to fix, at least for me... It appears the changes to _maybeQualifiedName regressed our support for lazy class definitions somewhat broadly. I'm not sure how to fix it in the new code, as the logic for name qualification is now rather complex (and appears to take into account whether the class is public or private, and whether the expression is a LiteralString, and whether it's a local type, which were not previously considered).

Maybe we need to abstract name qualification totally ... that's what MaybeQualifiedId was supposed to be doing, but maybe it doesn't adequately address Closure's use case. Hopefully we can find a simpler way to force Closure to always qualify all public names (if that is the desired behavior).

I may try and borrow some of the code here around class building, though.

Apologies in advance for any merge headaches :|