Check for duplicate library names

Check for duplicate library names

This bites us on Angular - dart:collection and angular's collection
(facade) lib smash on top of each other, silently causing issues.

We'll need a better naming convention - perhaps we should use the lib
name and require uniqueness.

Fixes #208

R=jmesserly@google.com

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

[vsm, 2015-06-04 22:36:34]

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

[vsm, 2015-06-04 22:36:53]

https://codereview.chromium.org/1145243013/diff/1/test/codegen/expect/sunflow...
File test/codegen/expect/sunflower/dom.js (left):

https://codereview.chromium.org/1145243013/diff/1/test/codegen/expect/sunflow...
test/codegen/expect/sunflower/dom.js:1: var dom = dart.defineLibrary(dom,
window);
Note, in the old pattern, this is silently broken if dom was already defined by
a lazyImport - we ignore window.

In this CL, it's still broken, but we throw a runtime error.

[Jennifer Messerly, 2015-06-05 16:22:27]

Meta question ... we're doing a lot of runtime checks for imports, which is more
code size. It'd be nice if we could keep our runtime specific logic light, and
have it map directly to ES 6 modules. Could we do this checking at compile time?
That has the benefit that it's totally free at runtime :)

Anyway not opposed to this approach, but I think we'd have to undo it for
modules. Maybe we should invest in a little more compiler ability to rename
things? That way, we can have the library as "dart/core.js" vs
"my/custom/core.js" and at compile time we disambiguate in a module that uses
both:

import * from 'dart/core';
import * as my_custom_core from 'my/custom/core';

class Foo extends Object {
}
...
class Bar extends my_custom_core.SomeType {
}
...

https://codereview.chromium.org/1145243013/diff/1/lib/runtime/dart/_foreign_h...
File lib/runtime/dart/_foreign_helper.js (right):

https://codereview.chromium.org/1145243013/diff/1/lib/runtime/dart/_foreign_h...
lib/runtime/dart/_foreign_helper.js:159: })(_foreign_helper, core);
fyi, some tools, like Closure, don't like a variable that appears to be
uninitialized. Similarly, I don't think it will work in node.js. That's the
benefit of declaring it. e.g. see this patch:
https://github.com/webcomponents/webcomponentsjs/pull/327
(those polyfills are browser only, so safe to use `window` ... I don't think
`window` would work for us)

https://codereview.chromium.org/1145243013/diff/1/lib/runtime/dart_runtime.js
File lib/runtime/dart_runtime.js (right):

https://codereview.chromium.org/1145243013/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:54: console.error(message);
what's the benefit of logging and throwing? Don't they both appear at the
console?

https://codereview.chromium.org/1145243013/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:55: throw new _js_helper.RuntimeError(message);
what happens if this isn't loaded? don't we have bootstrapping issues?
it seems better to just throw a JS Error. I suspect this will all be replaced
with a general module loader pattern/polyfill at some point, so it won't even be
Dart specific.

https://codereview.chromium.org/1145243013/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:1237: var loadedLibraries = {};
How will this work with JS interop? It seems we are assuming closed world here,
in other words, that all modules come from Dart. I was hoping we could integrate
more naturally at runtime with the JS code that's been loaded

https://codereview.chromium.org/1145243013/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:1274: throwRuntimeError('Library ' + name + ' cannot
be redefined to ' + defaultValue);
long line.
also could use interpolation:

`Library ${name} cannot be ...`

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

https://codereview.chromium.org/1145243013/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:164: var name = new
JS.Identifier(jsLibraryName(currentLibrary));
suggestion, maybe build the Identifier later, and have `name` store the String?

String name = jsLibraryName(currentLibrary);


That way the string can be used directly, instead of `name.name`

I think in most places, js_ast will implicitly convert the string to an
identifier (for better or worse), but if needed, could do:

var nameId = new JS.Identifier(name);

[vsm, 2015-06-05 17:00:51]

On 2015/06/05 16:22:27, John Messerly wrote:
> Meta question ... we're doing a lot of runtime checks for imports, which is
more
> code size. It'd be nice if we could keep our runtime specific logic light, and
> have it map directly to ES 6 modules. Could we do this checking at compile
time?
> That has the benefit that it's totally free at runtime :)

The code size here seems minimal.  Not sure what you mean about compile time
given our separate compilation.

I'm assuming a different module system (ES 6, etc) would not use dart.import,
etc., in the first place.  But I don't think we need to solve that right now.

> 
> Anyway not opposed to this approach, but I think we'd have to undo it for
> modules. Maybe we should invest in a little more compiler ability to rename
> things? That way, we can have the library as "dart/core.js" vs
> "my/custom/core.js" and at compile time we disambiguate in a module that uses
> both:
> 
> import * from 'dart/core';
> import * as my_custom_core from 'my/custom/core';
> 
> class Foo extends Object {
> }
> ...
> class Bar extends my_custom_core.SomeType {
> }
> ...
> 
>
https://codereview.chromium.org/1145243013/diff/1/lib/runtime/dart/_foreign_h...
> File lib/runtime/dart/_foreign_helper.js (right):
> 
>
https://codereview.chromium.org/1145243013/diff/1/lib/runtime/dart/_foreign_h...
> lib/runtime/dart/_foreign_helper.js:159: })(_foreign_helper, core);
> fyi, some tools, like Closure, don't like a variable that appears to be
> uninitialized. Similarly, I don't think it will work in node.js. That's the
> benefit of declaring it. e.g. see this patch:
> https://github.com/webcomponents/webcomponentsjs/pull/327
> (those polyfills are browser only, so safe to use `window` ... I don't think
> `window` would work for us)
> 
> https://codereview.chromium.org/1145243013/diff/1/lib/runtime/dart_runtime.js
> File lib/runtime/dart_runtime.js (right):
> 
>
https://codereview.chromium.org/1145243013/diff/1/lib/runtime/dart_runtime.js...
> lib/runtime/dart_runtime.js:54: console.error(message);
> what's the benefit of logging and throwing? Don't they both appear at the
> console?
> 
>
https://codereview.chromium.org/1145243013/diff/1/lib/runtime/dart_runtime.js...
> lib/runtime/dart_runtime.js:55: throw new _js_helper.RuntimeError(message);
> what happens if this isn't loaded? don't we have bootstrapping issues?
> it seems better to just throw a JS Error. I suspect this will all be replaced
> with a general module loader pattern/polyfill at some point, so it won't even
be
> Dart specific.
> 
>
https://codereview.chromium.org/1145243013/diff/1/lib/runtime/dart_runtime.js...
> lib/runtime/dart_runtime.js:1237: var loadedLibraries = {};
> How will this work with JS interop? It seems we are assuming closed world
here,
> in other words, that all modules come from Dart. I was hoping we could
integrate
> more naturally at runtime with the JS code that's been loaded
> 
>
https://codereview.chromium.org/1145243013/diff/1/lib/runtime/dart_runtime.js...
> lib/runtime/dart_runtime.js:1274: throwRuntimeError('Library ' + name + '
cannot
> be redefined to ' + defaultValue);
> long line.
> also could use interpolation:
> 
> `Library ${name} cannot be ...`
> 
>
https://codereview.chromium.org/1145243013/diff/1/lib/src/codegen/js_codegen....
> File lib/src/codegen/js_codegen.dart (right):
> 
>
https://codereview.chromium.org/1145243013/diff/1/lib/src/codegen/js_codegen....
> lib/src/codegen/js_codegen.dart:164: var name = new
> JS.Identifier(jsLibraryName(currentLibrary));
> suggestion, maybe build the Identifier later, and have `name` store the
String?
> 
> String name = jsLibraryName(currentLibrary);
> 
> 
> That way the string can be used directly, instead of `name.name`
> 
> I think in most places, js_ast will implicitly convert the string to an
> identifier (for better or worse), but if needed, could do:
> 
> var nameId = new JS.Identifier(name);

[vsm, 2015-06-05 17:01:04]

https://codereview.chromium.org/1145243013/diff/1/lib/runtime/dart/_foreign_h...
File lib/runtime/dart/_foreign_helper.js (right):

https://codereview.chromium.org/1145243013/diff/1/lib/runtime/dart/_foreign_h...
lib/runtime/dart/_foreign_helper.js:159: })(_foreign_helper, core);
On 2015/06/05 16:22:27, John Messerly wrote:
> fyi, some tools, like Closure, don't like a variable that appears to be
> uninitialized. Similarly, I don't think it will work in node.js. That's the
> benefit of declaring it. e.g. see this patch:
> https://github.com/webcomponents/webcomponentsjs/pull/327
> (those polyfills are browser only, so safe to use `window` ... I don't think
> `window` would work for us)

I'd been thinking about keeping this explicit at decl.  E.g.,:

var _foreign_helper = dart.defineLibrary('_foreign_helper');

WDYT?

https://codereview.chromium.org/1145243013/diff/1/lib/runtime/dart_runtime.js
File lib/runtime/dart_runtime.js (right):

https://codereview.chromium.org/1145243013/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:54: console.error(message);
On 2015/06/05 16:22:27, John Messerly wrote:
> what's the benefit of logging and throwing? Don't they both appear at the
> console?

removed

https://codereview.chromium.org/1145243013/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:55: throw new _js_helper.RuntimeError(message);
On 2015/06/05 16:22:27, John Messerly wrote:
> what happens if this isn't loaded? don't we have bootstrapping issues?
> it seems better to just throw a JS Error. I suspect this will all be replaced
> with a general module loader pattern/polyfill at some point, so it won't even
be
> Dart specific.

Done.

https://codereview.chromium.org/1145243013/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:1237: var loadedLibraries = {};
On 2015/06/05 16:22:27, John Messerly wrote:
> How will this work with JS interop? It seems we are assuming closed world
here,
> in other words, that all modules come from Dart. I was hoping we could
integrate
> more naturally at runtime with the JS code that's been loaded

Not really assuming a closed world.  For JS code, we can either:
(1) Always use lazyImport - we never require a lib to be in loadedLibraries to
run code
(2) Provide a different import / register API
(3) Make import a little more forgiving - though it *is* catching real errors
right now

https://codereview.chromium.org/1145243013/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:1274: throwRuntimeError('Library ' + name + ' cannot
be redefined to ' + defaultValue);
On 2015/06/05 16:22:27, John Messerly wrote:
> long line.
> also could use interpolation:
> 
> `Library ${name} cannot be ...`

Done.

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

https://codereview.chromium.org/1145243013/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:164: var name = new
JS.Identifier(jsLibraryName(currentLibrary));
On 2015/06/05 16:22:27, John Messerly wrote:
> suggestion, maybe build the Identifier later, and have `name` store the
String?
> 
> String name = jsLibraryName(currentLibrary);
> 
> 
> That way the string can be used directly, instead of `name.name`
> 
> I think in most places, js_ast will implicitly convert the string to an
> identifier (for better or worse), but if needed, could do:
> 
> var nameId = new JS.Identifier(name);

Done.

[Jennifer Messerly, 2015-06-05 20:33:32]

fyi, we chatted about this ... to summarize, what this is heading towards is an
AMD-like approach to modules, which has some really nice, immediate benefits:

* we can start importing modules via fully qualified name
* we won't pollute the global scope; in particular, the "short" module name will
only appear inside the module scope, (and the temp renamer can already handle
collisions there)
* we can debug our compile time linker, because we'll be able to load things in
correct order at runtime.

All of that is on the path towards ES6 modules, too. +1 from me.

Exactly syntax probably won't be quite AMD yet
(https://github.com/amdjs/amdjs-api/wiki/AMD) although it wouldn't be hard to
use it at some point. Dart's library cycles/lazy imports are a little bit odd,
so we probably want to track those dependencies in a separate list.

[vsm, 2015-06-05 22:04:39]

PTAL

I'm thinking of dartifying the names - e.g., 'dart:core' instead of
'dart/core.js'.  Thoughts?

https://codereview.chromium.org/1145243013/diff/40001/lib/runtime/dart/_forei...
File lib/runtime/dart/_foreign_helper.js (right):

https://codereview.chromium.org/1145243013/diff/40001/lib/runtime/dart/_forei...
lib/runtime/dart/_foreign_helper.js:1: dart.library('dart/_foreign_helper.js',
null, /* Imports */[
Suggestions on indentation?

[Jennifer Messerly, 2015-06-05 22:25:51]

On 2015/06/05 22:04:39, vsm wrote:
> PTAL
> 
> I'm thinking of dartifying the names - e.g., 'dart:core' instead of
> 'dart/core.js'.  Thoughts?

well, we don't need the ".js" extension, as I don't think any JS module patterns
have that either.

not a huge fan of 'dart:' though. It's a made up URI scheme, and would probably
confuse other tools expecting paths.

> 
>
https://codereview.chromium.org/1145243013/diff/40001/lib/runtime/dart/_forei...
> File lib/runtime/dart/_foreign_helper.js (right):
> 
>
https://codereview.chromium.org/1145243013/diff/40001/lib/runtime/dart/_forei...
> lib/runtime/dart/_foreign_helper.js:1: dart.library('dart/_foreign_helper.js',
> null, /* Imports */[
> Suggestions on indentation?

[vsm, 2015-06-05 22:44:28]

On 2015/06/05 22:25:51, John Messerly wrote:
> On 2015/06/05 22:04:39, vsm wrote:
> > PTAL
> > 
> > I'm thinking of dartifying the names - e.g., 'dart:core' instead of
> > 'dart/core.js'.  Thoughts?
> 
> well, we don't need the ".js" extension, as I don't think any JS module
patterns
> have that either.
> 
> not a huge fan of 'dart:' though. It's a made up URI scheme, and would
probably
> confuse other tools expecting paths.

Done.

> 
> > 
> >
>
https://codereview.chromium.org/1145243013/diff/40001/lib/runtime/dart/_forei...
> > File lib/runtime/dart/_foreign_helper.js (right):
> > 
> >
>
https://codereview.chromium.org/1145243013/diff/40001/lib/runtime/dart/_forei...
> > lib/runtime/dart/_foreign_helper.js:1:
dart.library('dart/_foreign_helper.js',
> > null, /* Imports */[
> > Suggestions on indentation?

[Jennifer Messerly, 2015-06-05 22:45:58]

very nice, lgtm. Just a few suggestions

https://codereview.chromium.org/1145243013/diff/40001/lib/runtime/dart/_forei...
File lib/runtime/dart/_foreign_helper.js (right):

https://codereview.chromium.org/1145243013/diff/40001/lib/runtime/dart/_forei...
lib/runtime/dart/_foreign_helper.js:1: dart.library('dart/_foreign_helper.js',
null, /* Imports */[
On 2015/06/05 22:04:39, vsm wrote:
> Suggestions on indentation?

looks alright to me. Maybe skip lazy imports if it's empty, as that seems
common?

https://codereview.chromium.org/1145243013/diff/60001/lib/runtime/dart_runtim...
File lib/runtime/dart_runtime.js (right):

https://codereview.chromium.org/1145243013/diff/60001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:1285: var libraries = {};
use a Map here?

https://codereview.chromium.org/1145243013/diff/60001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:1286: var loadedLibraries = new Set();
I think the reason this exists is to track libraries that are *completed*
whereas the `libraries` can contain incomplete lazy imports?

Anyway, looks good, but maybe worth a comment?

https://codereview.chromium.org/1145243013/diff/60001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:1301: dart.import = import_;
This is public for interop/REPL purposes? maybe worth a comment

https://codereview.chromium.org/1145243013/diff/60001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:1311: function lazyImport(name) {
maybe we don't need this? just rename the other one "initLibrary" ?
another way of doing it, if you really want two names:

    const lazyImport = initializeLibraryStub;

https://codereview.chromium.org/1145243013/diff/60001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:1324: 'Library ${name} cannot be redefined to
${defaultValue}');
backticks here?

https://codereview.chromium.org/1145243013/diff/60001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:1334: function library(name, defaultValue, imports,
lazyImports, module) {
for defaultValue, move last? since it's rarely supplied. (And I doubt we should
even really load dom.js ... making it a fairly rare feature. dart:html might use
but other than that.)

https://codereview.chromium.org/1145243013/diff/60001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:1346: module.apply(dart.global, args);
couple of thoughts here:

Maybe pass "null" instead of "global"? I don't think we generate module-level
`this`, so shouldn't affect anything?

It looks like we're using eager loaded of the library? Seems fine. It's
interesting though, that we could load lazily if we wanted, because we have
enough info to do so, and trigger it all from "start" or "import". Not sure if
that's worth a comment of some sort.

https://codereview.chromium.org/1145243013/diff/60001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:1356: // TODO(jmesserly): hack to bootstrap the SDK
is this still needed?

[vsm, 2015-06-05 23:02:13]

Address comments

[vsm, 2015-06-05 23:03:11]

Thanks - landing.

I'll have a separate CL to deal with the out-of-order loading issues (and maybe
go lazy here).

https://codereview.chromium.org/1145243013/diff/60001/lib/runtime/dart_runtim...
File lib/runtime/dart_runtime.js (right):

https://codereview.chromium.org/1145243013/diff/60001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:1285: var libraries = {};
On 2015/06/05 22:45:57, John Messerly wrote:
> use a Map here?

Done.

https://codereview.chromium.org/1145243013/diff/60001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:1286: var loadedLibraries = new Set();
On 2015/06/05 22:45:57, John Messerly wrote:
> I think the reason this exists is to track libraries that are *completed*
> whereas the `libraries` can contain incomplete lazy imports?
> 
> Anyway, looks good, but maybe worth a comment?

Done.

https://codereview.chromium.org/1145243013/diff/60001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:1301: dart.import = import_;
On 2015/06/05 22:45:57, John Messerly wrote:
> This is public for interop/REPL purposes? maybe worth a comment

Done.

https://codereview.chromium.org/1145243013/diff/60001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:1311: function lazyImport(name) {
On 2015/06/05 22:45:57, John Messerly wrote:
> maybe we don't need this? just rename the other one "initLibrary" ?
> another way of doing it, if you really want two names:
> 
>     const lazyImport = initializeLibraryStub;

Done.

https://codereview.chromium.org/1145243013/diff/60001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:1324: 'Library ${name} cannot be redefined to
${defaultValue}');
On 2015/06/05 22:45:57, John Messerly wrote:
> backticks here?

Done.

https://codereview.chromium.org/1145243013/diff/60001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:1346: module.apply(dart.global, args);
On 2015/06/05 22:45:57, John Messerly wrote:
> couple of thoughts here:
> 
> Maybe pass "null" instead of "global"? I don't think we generate module-level
> `this`, so shouldn't affect anything?
> 
> It looks like we're using eager loaded of the library? Seems fine. It's
> interesting though, that we could load lazily if we wanted, because we have
> enough info to do so, and trigger it all from "start" or "import". Not sure if
> that's worth a comment of some sort.

Done.

I'll tackle ordering (lazy or eager) in a separate CL.

https://codereview.chromium.org/1145243013/diff/60001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:1356: // TODO(jmesserly): hack to bootstrap the SDK
On 2015/06/05 22:45:57, John Messerly wrote:
> is this still needed?

Done.

[vsm, 2015-06-05 23:34:20]

Committed patchset #6 (id:100001) manually as
d6a456ad565dc40bbd0a17acea3c8607e460e962 (presubmit successful).