Added support for exports and re-exports.

Added support for exports and re-exports.


BUG=5429
TESTS=language/export_test, language/export_cyclic_test

Committed: https://code.google.com/p/dart/source/detail?r=13478

[Johnni Winther, 2012-09-26 17:03:58]

https://codereview.chromium.org/10990060/diff/1017/pkg/dartdoc/lib/src/mirror...
File pkg/dartdoc/lib/src/mirrors/dart2js_mirror.dart (left):

https://codereview.chromium.org/10990060/diff/1017/pkg/dartdoc/lib/src/mirror...
pkg/dartdoc/lib/src/mirrors/dart2js_mirror.dart:372:
_compiler.scanner.loadLibrary(uri, null);
Stale code.

https://codereview.chromium.org/10990060/diff/1017/tests/language/export_cycl...
File tests/language/export_cyclic_helper1.dart (right):

https://codereview.chromium.org/10990060/diff/1017/tests/language/export_cycl...
tests/language/export_cyclic_helper1.dart:4: 
This is a new file.

https://codereview.chromium.org/10990060/diff/1017/tests/language/export_cycl...
File tests/language/export_cyclic_helper2.dart (right):

https://codereview.chromium.org/10990060/diff/1017/tests/language/export_cycl...
tests/language/export_cyclic_helper2.dart:4: 
This is a new file.

https://codereview.chromium.org/10990060/diff/1017/tests/language/export_cycl...
File tests/language/export_cyclic_helper3.dart (right):

https://codereview.chromium.org/10990060/diff/1017/tests/language/export_cycl...
tests/language/export_cyclic_helper3.dart:4: 
This is a new file.

https://codereview.chromium.org/10990060/diff/1017/tests/language/export_help...
File tests/language/export_helper1.dart (right):

https://codereview.chromium.org/10990060/diff/1017/tests/language/export_help...
tests/language/export_helper1.dart:4: 
This is a new file.

https://codereview.chromium.org/10990060/diff/1017/tests/language/export_help...
File tests/language/export_helper2.dart (right):

https://codereview.chromium.org/10990060/diff/1017/tests/language/export_help...
tests/language/export_helper2.dart:4: 
This is a new file.

https://codereview.chromium.org/10990060/diff/1017/tests/language/export_test...
File tests/language/export_test.dart (right):

https://codereview.chromium.org/10990060/diff/1017/tests/language/export_test...
tests/language/export_test.dart:4: 
This is a new file.

[kasperl, 2012-09-27 05:38:00]

https://codereview.chromium.org/10990060/diff/1017/tests/language/export_help...
File tests/language/export_helper1.dart (right):

https://codereview.chromium.org/10990060/diff/1017/tests/language/export_help...
tests/language/export_helper1.dart:8: export 'export_helper2.dart';
Very cool to see the support for re-export start trickling in.

[Lasse Reichstein Nielsen, 2012-09-28 07:45:36]

Needs serious renaming, and some refactoring.
Please avoid "data classes". Find some way to describe their role, and it will
probably be easy to see meaningful methods on them too (and better
encapsulation).

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
File lib/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/elements/elements.dart:107: static const ElementKind
ERRONEOUS =
It should be called ERROR - a noun - and not ERRONEOUS - an adjective.
It represents an error condition. It does so correctly, not erroneously.

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/elements/elements.dart:564: Map<SourceString,
Element> get exportScope {
Please don't export the map. Someone will just do bad stuff to it. Can you wrap
it in something opaque that only exposes simple lookup and foreach operations?

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/elements/elements.dart:602: if (existing !== element
&& existing !== null) {
Add a comment that you are not following the current spec here by testing for
(existing !== element). Importing the same element twice is specified as an
error.

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
File lib/compiler/implementation/scanner/scanner_task.dart (right):

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:50: var importsExports =
new LinkBuilder<ImportExport>();
ImportExport is very descriptive, but not very explanatory.
What is the common concept that covers Import and Export and that makes you
combine them here? LibraryDependency?

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:59: if
(tag.uri.dartString.slowToString() == 'dart:core') {
Using something called slowToString smells bad.
How about:
 Uri tagUri = tag.uri;
 if (tagUri.scheme == "dart" && tagUri.path == "core") {
?
(Or add an operator== on Uri)

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:92: (library.uri.scheme !=
'dart' || library.uri.path != 'core')) {
I really can't read this. Please use
  if (!importsDartCore ||
      !(library.uri.scheme == 'dart' && library.uri.path == 'core')) {
instead. And then, shouldn't the first '||' be an '&&'?

You may want a utility function:
 bool isDartCore(Uri uri) => uri.scheme == "dart" && uri.path == "core";
It seems to crop up often enough.
Or maybe just do
 .. && library != compiler.coreLibrary
?

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:98: for (ImportExport tag
in importsExports.toLink()) {
We even have a class called ImportExport? Please change it to something less
"bag of x and y" like. LibraryDependency is ok but not great.

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:128: 
Why are we passing both tag and compilationUnit?
I *think* it's in order to give good error messages for the library loading by
referring to the tag in its source.
How about combining Tag and compilationUnit into a single object, if they have
no independent uses?

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:205: static const int
IMPORT = 2;
Includes export?

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:219: * Data class for
handling import tags in [ImportExportHandler].
Comment isn't helping me understand it. This is just a way to say "you don't
want to know". A "Data class" suggests that it has no meaning in itself.
"Handling" is so vague it's useless. So this is something to do with "import
tags" (the syntax) used inside some other class.

If it deserves a class, find something that the collection of data has in
commmon - even if it's just a common use.

I guess that this combines data related to a single library import. The data is
used by ImportExportHandler to schedule and perform the actual inclusion of
declarations from one library in the other library's import scope.

If I knew that, I would understand why the data is needed and why it's combined.

If it was called ImportTask, you wouldn't think of it as a "data class" (and you
should rarely have data classes!), and would be more inclind to put meaningful
methods on the class.
Even if it's just 'performTask()'.

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:223: final LibraryElement
library;
Add documentation for fields.
What is this library? The importer or the importee?
What is it used for?

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:224: final
CompilationUnitElement compilationUnit;
What is it a compilationUnit of? What is it used for?

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:231: *
[ImportExportHandler].
Also bad explanation. I'm not any wiser, and I question the use of a "data
class".

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:235: Link<ImportData>
imports = const EmptyLink<ImportData>();
Explain fields.
Is this a work-list of import tasks that needs to be performed?

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:236: Map<Export,
ImportExportData> dependencyMap =
Just "dependencies". I can see it's a map, and if it stopped being a map, but
still worked the same, I shouldn't need to care.
And please encapsulate access to the fields.

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:237: new Map<Export,
ImportExportData>();
This maps syntax to a data object. That seems odd. When do you need to look up
by export tag again?

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:238: Map<SourceString,
Element> exportScope = new Map<SourceString, Element>();
This, I guess, is the export scope that is being built for a library. For
[library], presumably.

Have you considered making letting the Library create the map, and just use a
bool to inhibit access to it until it's fully populated, instead of making the
map non-final?
And preferably make the library only accept access to the map through specific
functions like addToExportScope?

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:246: * a set of libraries.
Are you sure this should not be a CompilerTask? It even has a compiler reference
:)
It's the linking phase of the compiler, not just some obscure helper function.
It seems it does significant work in the computeExports method - is this work
measured on any Task?

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:252: class
ImportExportHandler {
Again: "ImportExport" is a bad name. (I'll stop now :)

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:255:
Map<LibraryElement,ImportExportData> dataMap =
Plase give this a better name that describes how it is used.
And a descriptive comment.

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:270: var pendingExportSet
= new Set<Element>.from(data.pendingExportSet);
Why a Set? I'd think a list would have lower overhead and faster iteration.

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:276: if (existingElement
!== null && existingElement !== element) {
If you export the same library twice, should it be an error?
If so, make a comment here that you are not following the specification.

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:298:
dataMap.forEach((LibraryElement library, ImportExportData data) {
Join loops?

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:314:
dataMap[importedLibrary].dependencyMap[tag] = dataMap[importingLibrary];
Because dataMap says nothing about what it does, this line means nothing.
Better names, all around.
I want to be able to read that this adds this library as an export dependency on
the imported library. Even if it takes named variables for intermediate values.

Also, don't access the map on the dataMap[importedLibrary], make a method:
  dataMap[importedLibrary].addDependency(tag, dataMap[importingLibrary]);

You are crippling your style by accepting that something is just a data class
that doesn't correspond to any concept.

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:326: dataMap[library] =
new ImportExportData(library);
So the ImportExportData is the data associated with library by this class. It
collects dependencies and it builds the exportMap. These are all data that could
have been on LibraryElement, but is only used in this class.

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:337: }));
Ick.
How about:
  ImportExportData libraryData = dataMap[library];
  library.localScope.forEachPublic(libraryData.addExport);
Add methods as necessary to avoid these long chains of
dots. Or aim even higher and never have more than one '.'
in a "chain".

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:362:
compiler.reportError(tag.prefix, 'duplicate defintion');
Mmissing 'i" in "definition"

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
File lib/compiler/implementation/tree/nodes.dart (right):

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/tree/nodes.dart:1645: abstract class ImportExport
extends LibraryTag {
Call it LibraryDependency (or something better if you have it).
Add doc-comment: This tag describes a dependency between one library and the
exported identifiers of another library. The other library is specified by the
[uri]. Combinators filter away some identifiers from the other library.

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/tree/nodes.dart:1652: class Import extends
ImportExport {
Comment to the effect that an Import is dependency on another library where the
exported identifiers are put into the import scope of the importing library. The
import scope is only visible inside the library.

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/tree/nodes.dart:1681: class Export extends
ImportExport {
And a comment too, describing what an export is relative to a LibraryDependency.

[Bob Nystrom, 2012-09-28 16:41:26]

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
File lib/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/elements/elements.dart:564: Map<SourceString,
Element> get exportScope {
On 2012/09/28 07:45:36, Lasse Reichstein Nielsen wrote:
> Please don't export the map. Someone will just do bad stuff to it. Can you
wrap
> it in something opaque that only exposes simple lookup and foreach operations?

Drive-by comment: I see stuff like this frequently. It makes me wonder if we
should have better support for providing read-only access to collections.

[Johnni Winther, 2012-10-09 09:47:10]

Major refactoring. PTAL

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
File lib/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/elements/elements.dart:107: static const ElementKind
ERRONEOUS =
On 2012/09/28 07:45:36, Lasse Reichstein Nielsen wrote:
> It should be called ERROR - a noun - and not ERRONEOUS - an adjective.
> It represents an error condition. It does so correctly, not erroneously.

Done.

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/elements/elements.dart:564: Map<SourceString,
Element> get exportScope {
On 2012/09/28 07:45:36, Lasse Reichstein Nielsen wrote:
> Please don't export the map. Someone will just do bad stuff to it. Can you
wrap
> it in something opaque that only exposes simple lookup and foreach operations?

Changed to used a linked list.

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/elements/elements.dart:602: if (existing !== element
&& existing !== null) {
On 2012/09/28 07:45:36, Lasse Reichstein Nielsen wrote:
> Add a comment that you are not following the current spec here by testing for
> (existing !== element). Importing the same element twice is specified as an
> error.

Implementation changed to follow spec.

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
File lib/compiler/implementation/scanner/scanner_task.dart (right):

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:50: var importsExports =
new LinkBuilder<ImportExport>();
On 2012/09/28 07:45:36, Lasse Reichstein Nielsen wrote:
> ImportExport is very descriptive, but not very explanatory.
> What is the common concept that covers Import and Export and that makes you
> combine them here? LibraryDependency?

Done.

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:59: if
(tag.uri.dartString.slowToString() == 'dart:core') {
On 2012/09/28 07:45:36, Lasse Reichstein Nielsen wrote:
> Using something called slowToString smells bad.
> How about:
>  Uri tagUri = tag.uri;
>  if (tagUri.scheme == "dart" && tagUri.path == "core") {
> ?
> (Or add an operator== on Uri)

[uri] is a LiteralString, not a Uri.

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:92: (library.uri.scheme !=
'dart' || library.uri.path != 'core')) {
On 2012/09/28 07:45:36, Lasse Reichstein Nielsen wrote:
> I really can't read this. Please use
>   if (!importsDartCore ||
>       !(library.uri.scheme == 'dart' && library.uri.path == 'core')) {
> instead. And then, shouldn't the first '||' be an '&&'?
> 
> You may want a utility function:
>  bool isDartCore(Uri uri) => uri.scheme == "dart" && uri.path == "core";
> It seems to crop up often enough.
> Or maybe just do
>  .. && library != compiler.coreLibrary
> ?

Done.

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:98: for (ImportExport tag
in importsExports.toLink()) {
On 2012/09/28 07:45:36, Lasse Reichstein Nielsen wrote:
> We even have a class called ImportExport? Please change it to something less
> "bag of x and y" like. LibraryDependency is ok but not great.

Done.

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:128: 
On 2012/09/28 07:45:36, Lasse Reichstein Nielsen wrote:
> Why are we passing both tag and compilationUnit?
> I *think* it's in order to give good error messages for the library loading by
> referring to the tag in its source.
> How about combining Tag and compilationUnit into a single object, if they have
> no independent uses?

CompilationUnit not needed anymore due to patch refactoring.

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:205: static const int
IMPORT = 2;
On 2012/09/28 07:45:36, Lasse Reichstein Nielsen wrote:
> Includes export?

Changed to IMPORT_OR_EXPORT.

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:224: final
CompilationUnitElement compilationUnit;
On 2012/09/28 07:45:36, Lasse Reichstein Nielsen wrote:
> What is it a compilationUnit of? What is it used for?

Not needed anymore due to patch refactoring.

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:270: var pendingExportSet
= new Set<Element>.from(data.pendingExportSet);
On 2012/09/28 07:45:36, Lasse Reichstein Nielsen wrote:
> Why a Set? I'd think a list would have lower overhead and faster iteration.

Done.

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:298:
dataMap.forEach((LibraryElement library, ImportExportData data) {
On 2012/09/28 07:45:36, Lasse Reichstein Nielsen wrote:
> Join loops?

Can't do. We need the completion of exports before handling of import. Sort the
whole point in this deferred handling of the imports/exports.

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner_task.dart:362:
compiler.reportError(tag.prefix, 'duplicate defintion');
On 2012/09/28 07:45:36, Lasse Reichstein Nielsen wrote:
> Mmissing 'i" in "definition"

Done.

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
File lib/compiler/implementation/tree/nodes.dart (right):

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/tree/nodes.dart:1645: abstract class ImportExport
extends LibraryTag {
On 2012/09/28 07:45:36, Lasse Reichstein Nielsen wrote:
> Call it LibraryDependency (or something better if you have it).
> Add doc-comment: This tag describes a dependency between one library and the
> exported identifiers of another library. The other library is specified by the
> [uri]. Combinators filter away some identifiers from the other library.

Done.

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/tree/nodes.dart:1652: class Import extends
ImportExport {
On 2012/09/28 07:45:36, Lasse Reichstein Nielsen wrote:
> Comment to the effect that an Import is dependency on another library where
the
> exported identifiers are put into the import scope of the importing library.
The
> import scope is only visible inside the library.

Done.

https://codereview.chromium.org/10990060/diff/1017/lib/compiler/implementatio...
lib/compiler/implementation/tree/nodes.dart:1681: class Export extends
ImportExport {
On 2012/09/28 07:45:36, Lasse Reichstein Nielsen wrote:
> And a comment too, describing what an export is relative to a
LibraryDependency.

Done.

[ahe, 2012-10-09 13:07:49]

https://codereview.chromium.org/10990060/diff/14001/lib/compiler/implementati...
File lib/compiler/implementation/elements/elements.dart (left):

https://codereview.chromium.org/10990060/diff/14001/lib/compiler/implementati...
lib/compiler/implementation/elements/elements.dart:361: : super(const
SourceString('erroneous element'), null, enclosing);
Good catch!

https://codereview.chromium.org/10990060/diff/14001/lib/compiler/implementati...
File lib/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/10990060/diff/14001/lib/compiler/implementati...
lib/compiler/implementation/elements/elements.dart:624:
importScope.putIfAbsent(element.name, () => element);
This should be:

Element existing = importScope.putIfAbsent(element.name, () => element);

https://codereview.chromium.org/10990060/diff/14001/lib/compiler/implementati...
lib/compiler/implementation/elements/elements.dart:636: bool get exportsHandled
=> _exports !== null;
What is the purpose of this method?

https://codereview.chromium.org/10990060/diff/14001/lib/compiler/implementati...
lib/compiler/implementation/elements/elements.dart:644: Link<Element> _exports;
Why is this private, and why is it not with all the other fields?

https://codereview.chromium.org/10990060/diff/14001/lib/compiler/implementati...
lib/compiler/implementation/elements/elements.dart:652: void
setExports(Iterable<Element> iterable) {
Why is this not a setter?

[Johnni Winther, 2012-10-09 13:50:07]

https://codereview.chromium.org/10990060/diff/14001/lib/compiler/implementati...
File lib/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/10990060/diff/14001/lib/compiler/implementati...
lib/compiler/implementation/elements/elements.dart:624:
importScope.putIfAbsent(element.name, () => element);
On 2012/10/09 13:07:50, ahe wrote:
> This should be:
> 
> Element existing = importScope.putIfAbsent(element.name, () => element);

Changed to not use [putIfAbsent].

https://codereview.chromium.org/10990060/diff/14001/lib/compiler/implementati...
lib/compiler/implementation/elements/elements.dart:636: bool get exportsHandled
=> _exports !== null;
On 2012/10/09 13:07:50, ahe wrote:
> What is the purpose of this method?

To test whether the export scope has been computed for this library.

https://codereview.chromium.org/10990060/diff/14001/lib/compiler/implementati...
lib/compiler/implementation/elements/elements.dart:644: Link<Element> _exports;
On 2012/10/09 13:07:50, ahe wrote:
> Why is this private, and why is it not with all the other fields?

It is private so that setExports can ensure that it is only set once. It should
be with the other fields.

https://codereview.chromium.org/10990060/diff/14001/lib/compiler/implementati...
lib/compiler/implementation/elements/elements.dart:652: void
setExports(Iterable<Element> iterable) {
On 2012/10/09 13:07:50, ahe wrote:
> Why is this not a setter?

Because it is not symmetric to the getter. It has a different type and must only
be called once.

[ahe, 2012-10-09 14:41:32]

https://codereview.chromium.org/10990060/diff/14001/lib/compiler/implementati...
File lib/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/10990060/diff/14001/lib/compiler/implementati...
lib/compiler/implementation/elements/elements.dart:644: Link<Element> _exports;
On 2012/10/09 13:50:07, Johnni Winther wrote:
> It is private so that setExports can ensure that it is only set once.

How does privacy ensure that it can only be set once?

[Bob Nystrom, 2012-10-09 16:20:37]

https://codereview.chromium.org/10990060/diff/14001/lib/compiler/implementati...
File lib/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/10990060/diff/14001/lib/compiler/implementati...
lib/compiler/implementation/elements/elements.dart:652: void
setExports(Iterable<Element> iterable) {
On 2012/10/09 13:50:07, Johnni Winther wrote:
> On 2012/10/09 13:07:50, ahe wrote:
> > Why is this not a setter?
> 
> Because it is not symmetric to the getter. It has a different type and must
only
> be called once.

I agree with Johnni, I wouldn't make this a setter either. Setters should have
"field-like" semantics and one thing that implies (to me at least) is that you
can invoke them repeatedly.

My personal convention for methods like this is to call them "bind___". "bind"
as opposed to "set" to me implies more of a one-time occurrence. (Although it
might be confusing here since "bind" is already meaningful in the context of a
compiler.)

[Lasse Reichstein Nielsen, 2012-10-10 08:07:33]

LGTM

https://codereview.chromium.org/10990060/diff/15024/lib/compiler/implementati...
File lib/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/10990060/diff/15024/lib/compiler/implementati...
lib/compiler/implementation/compiler.dart:398: LibraryLoader.importLibrary(this,
library, jsHelperLibrary, null);
capital 'L'?

https://codereview.chromium.org/10990060/diff/15024/lib/compiler/implementati...
lib/compiler/implementation/compiler.dart:835: bool invariant(Spannable
spannable, var condition, {String message: null}) {
"Spannable" is a bad name. It's something that is able to "span", which is
technically a verb, but you don't really mean that here. It suggests that you
can ask the object to "span". I assume it's something that can *provide* a Span
if you ask it. That perhaps makes it "Spanned". Or try finding a better name.

https://codereview.chromium.org/10990060/diff/15024/lib/compiler/implementati...
File lib/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/10990060/diff/15024/lib/compiler/implementati...
lib/compiler/implementation/elements/elements.dart:595: *
[LibraryDependencyHandler] sets this map when the library is loaded.
It's the classic set-once pattern.
I would prefer to have this class create the map itself, but I also want to not
expose it externally, and the LibraryLoader needs to know about it.

https://codereview.chromium.org/10990060/diff/15024/lib/compiler/implementati...
lib/compiler/implementation/elements/elements.dart:660: void
setExports(Iterable<Element> iterable) {
'iterable' -> 'exportedElements'.
Name for the meaning, not the implementation.

https://codereview.chromium.org/10990060/diff/15024/lib/compiler/implementati...
File lib/compiler/implementation/library_loader.dart (right):

https://codereview.chromium.org/10990060/diff/15024/lib/compiler/implementati...
lib/compiler/implementation/library_loader.dart:23: static void
importLibrary(Compiler compiler,
Don't make this static. The caller has compiler, so he can call
compiler.libraryLoader.importLibrary(...) instead of
LibraryLoader.importLibrary(compiler, ...). 
It's more consistent.

https://codereview.chromium.org/10990060/diff/15024/lib/compiler/implementati...
lib/compiler/implementation/library_loader.dart:78: class LibraryLoaderTask
extends CompilerTask implements LibraryLoader {
Document classes so the difference between LibraryLoader and LibraryLoaderTask
become obvious.

https://codereview.chromium.org/10990060/diff/15024/lib/compiler/implementati...
lib/compiler/implementation/library_loader.dart:115: void
processLibraryTags(LibraryDependencyHandler handler, LibraryElement library) {
long line.

https://codereview.chromium.org/10990060/diff/15024/lib/compiler/implementati...
lib/compiler/implementation/library_loader.dart:216: compiler.coreLibrary =
loadLibraryInternal(handler, coreUri, null, coreUri);
Long line.

https://codereview.chromium.org/10990060/diff/15024/lib/compiler/implementati...
lib/compiler/implementation/library_loader.dart:315: * An [import] tag and the
[library] imported trought [import].
"throught" -> "through"

https://codereview.chromium.org/10990060/diff/15024/lib/compiler/implementati...
lib/compiler/implementation/library_loader.dart:317: class ImportLink {
Still this data class. Is there really no behavior that fits better on this
class?

[Johnni Winther, 2012-10-10 09:20:59]

https://codereview.chromium.org/10990060/diff/14001/lib/compiler/implementati...
File lib/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/10990060/diff/14001/lib/compiler/implementati...
lib/compiler/implementation/elements/elements.dart:644: Link<Element> _exports;
On 2012/10/09 14:41:33, ahe wrote:
> On 2012/10/09 13:50:07, Johnni Winther wrote:
> > It is private so that setExports can ensure that it is only set once.
> 
> How does privacy ensure that it can only be set once?

It doesn't. Renamed to [slotForExports] and comment expanded to tell that this
field should be not be accessed by other methods that the getter and setExports.

https://codereview.chromium.org/10990060/diff/15024/lib/compiler/implementati...
File lib/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/10990060/diff/15024/lib/compiler/implementati...
lib/compiler/implementation/compiler.dart:398: LibraryLoader.importLibrary(this,
library, jsHelperLibrary, null);
On 2012/10/10 08:07:33, Lasse Reichstein Nielsen wrote:
> capital 'L'?

Done.

https://codereview.chromium.org/10990060/diff/15024/lib/compiler/implementati...
lib/compiler/implementation/compiler.dart:835: bool invariant(Spannable
spannable, var condition, {String message: null}) {
On 2012/10/10 08:07:33, Lasse Reichstein Nielsen wrote:
> "Spannable" is a bad name. It's something that is able to "span", which is
> technically a verb, but you don't really mean that here. It suggests that you
> can ask the object to "span". I assume it's something that can *provide* a
Span
> if you ask it. That perhaps makes it "Spanned". Or try finding a better name.

Added a TODO.

https://codereview.chromium.org/10990060/diff/15024/lib/compiler/implementati...
File lib/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/10990060/diff/15024/lib/compiler/implementati...
lib/compiler/implementation/elements/elements.dart:595: *
[LibraryDependencyHandler] sets this map when the library is loaded.
On 2012/10/10 08:07:33, Lasse Reichstein Nielsen wrote:
> It's the classic set-once pattern.
> I would prefer to have this class create the map itself, but I also want to
not
> expose it externally, and the LibraryLoader needs to know about it.

Done.

https://codereview.chromium.org/10990060/diff/15024/lib/compiler/implementati...
lib/compiler/implementation/elements/elements.dart:660: void
setExports(Iterable<Element> iterable) {
On 2012/10/10 08:07:33, Lasse Reichstein Nielsen wrote:
> 'iterable' -> 'exportedElements'.
> Name for the meaning, not the implementation.

Done.

https://codereview.chromium.org/10990060/diff/15024/lib/compiler/implementati...
File lib/compiler/implementation/library_loader.dart (right):

https://codereview.chromium.org/10990060/diff/15024/lib/compiler/implementati...
lib/compiler/implementation/library_loader.dart:23: static void
importLibrary(Compiler compiler,
On 2012/10/10 08:07:33, Lasse Reichstein Nielsen wrote:
> Don't make this static. The caller has compiler, so he can call
> compiler.libraryLoader.importLibrary(...) instead of
> LibraryLoader.importLibrary(compiler, ...). 
> It's more consistent.

Done.

https://codereview.chromium.org/10990060/diff/15024/lib/compiler/implementati...
lib/compiler/implementation/library_loader.dart:78: class LibraryLoaderTask
extends CompilerTask implements LibraryLoader {
On 2012/10/10 08:07:33, Lasse Reichstein Nielsen wrote:
> Document classes so the difference between LibraryLoader and LibraryLoaderTask
> become obvious.

Done.

https://codereview.chromium.org/10990060/diff/15024/lib/compiler/implementati...
lib/compiler/implementation/library_loader.dart:115: void
processLibraryTags(LibraryDependencyHandler handler, LibraryElement library) {
On 2012/10/10 08:07:33, Lasse Reichstein Nielsen wrote:
> long line.

Done.

https://codereview.chromium.org/10990060/diff/15024/lib/compiler/implementati...
lib/compiler/implementation/library_loader.dart:216: compiler.coreLibrary =
loadLibraryInternal(handler, coreUri, null, coreUri);
On 2012/10/10 08:07:33, Lasse Reichstein Nielsen wrote:
> Long line.

Done.

https://codereview.chromium.org/10990060/diff/15024/lib/compiler/implementati...
lib/compiler/implementation/library_loader.dart:315: * An [import] tag and the
[library] imported trought [import].
On 2012/10/10 08:07:33, Lasse Reichstein Nielsen wrote:
> "throught" -> "through"

Done.

https://codereview.chromium.org/10990060/diff/15024/lib/compiler/implementati...
lib/compiler/implementation/library_loader.dart:317: class ImportLink {
On 2012/10/10 08:07:33, Lasse Reichstein Nielsen wrote:
> Still this data class. Is there really no behavior that fits better on this
> class?

Done.

[ahe, 2012-10-10 12:06:26]

I would appreciate if you could walk me through the logic.

https://codereview.chromium.org/10990060/diff/12018/lib/compiler/implementati...
File lib/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/10990060/diff/12018/lib/compiler/implementati...
lib/compiler/implementation/elements/elements.dart:664: message: 'Exports
already set to $slotForExports on $this'));
Long line.

https://codereview.chromium.org/10990060/diff/12018/lib/compiler/implementati...
File lib/compiler/implementation/library_loader.dart (right):

https://codereview.chromium.org/10990060/diff/12018/lib/compiler/implementati...
lib/compiler/implementation/library_loader.dart:24: LibraryDependency tag);
Add newline.

https://codereview.chromium.org/10990060/diff/12018/lib/compiler/implementati...
lib/compiler/implementation/library_loader.dart:26: * Adds the elements in the
export scope if [importedLibrary] to the import
if -> of

https://codereview.chromium.org/10990060/diff/12018/lib/compiler/implementati...
lib/compiler/implementation/library_loader.dart:42: class LibraryLoaderTask
extends CompilerTask implements LibraryLoader {
I feel it would be simpler to extend LibraryLoader and have LibaryLoader extend
CompilerTask.

https://codereview.chromium.org/10990060/diff/12018/lib/compiler/implementati...
lib/compiler/implementation/library_loader.dart:61: LibraryElement
loadLibrary(Uri uri, Node node, Uri canonicalUri) {
Do you have to repeat documentation like this?

https://codereview.chromium.org/10990060/diff/12018/lib/compiler/implementati...
lib/compiler/implementation/library_loader.dart:105: // It is not safe to import
other libraries at this point as
Isn't this comment outdated by now?

https://codereview.chromium.org/10990060/diff/12018/lib/compiler/implementati...
lib/compiler/implementation/library_loader.dart:109: if
(tag.uri.dartString.slowToString() == 'dart:core') {
We should convert tag.uri to an instance of Uri earlier so you can use
isDartCore. Perhaps the libraryDependencies object should be able to answer this
question.

https://codereview.chromium.org/10990060/diff/12018/lib/compiler/implementati...
lib/compiler/implementation/library_loader.dart:139: // Now that we have
processed all the source tags, it is safe to
Is this still relevant?

https://codereview.chromium.org/10990060/diff/12018/lib/compiler/implementati...
lib/compiler/implementation/library_loader.dart:198: * is, any resolution should
be done beforehand.
resolution -> URI resolution.

https://codereview.chromium.org/10990060/diff/12018/lib/compiler/implementati...
lib/compiler/implementation/library_loader.dart:200: void loadPart(Part part,
Uri path, LibraryElement library) {
if (!path.isAbsolute()) throw ArgumenError(path);

https://codereview.chromium.org/10990060/diff/12018/lib/compiler/implementati...
lib/compiler/implementation/library_loader.dart:200: void loadPart(Part part,
Uri path, LibraryElement library) {
Rename method to "scanPart" or "scanCompilationUnit"?

https://codereview.chromium.org/10990060/diff/12018/lib/compiler/implementati...
lib/compiler/implementation/library_loader.dart:252:
compiler.onLibraryLoaded(library, uri);
So this is called before the library is fully loaded?

https://codereview.chromium.org/10990060/diff/12018/lib/compiler/implementati...
lib/compiler/implementation/library_loader.dart:268: 'no #library tag found in
${importedLibrary.uri}');
Why is this error reported twice?

https://codereview.chromium.org/10990060/diff/12018/lib/compiler/implementati...
lib/compiler/implementation/library_loader.dart:293: compiler.reportWarning(new
Identifier(existing.position()),
This can be cleaned up. See checkDuplicatedLibraryName above.

https://codereview.chromium.org/10990060/diff/12018/lib/compiler/implementati...
lib/compiler/implementation/library_loader.dart:381: Map<SourceString, Element>
exportScope = new Map<SourceString, Element>();
Nit: I'm glad this isn't a [Scope] because scope doesn't feel like the right
word to use here.

https://codereview.chromium.org/10990060/diff/12018/lib/compiler/implementati...
lib/compiler/implementation/library_loader.dart:456:
exportScope.putIfAbsent(name, () => element);
Another weird use of putIfAbsent.

https://codereview.chromium.org/10990060/diff/12018/lib/compiler/implementati...
File lib/compiler/implementation/tree/nodes.dart (right):

https://codereview.chromium.org/10990060/diff/12018/lib/compiler/implementati...
lib/compiler/implementation/tree/nodes.dart:22: R visitExport(Export node) =>
visitLibraryTag(node);
Should return "visitLibraryDependency(node)"

https://codereview.chromium.org/10990060/diff/12018/lib/compiler/implementati...
lib/compiler/implementation/tree/nodes.dart:32: R visitImport(Import node) =>
visitLibraryTag(node);
Ditto.

https://codereview.chromium.org/10990060/diff/12018/lib/compiler/implementati...
File lib/compiler/implementation/util/util.dart (right):

https://codereview.chromium.org/10990060/diff/12018/lib/compiler/implementati...
lib/compiler/implementation/util/util.dart:12: // TODO(johnniwinther): Find a
better name.
Bolt? A "bolt" is something you use a spanner on. So a "bolt" is something that
can be spanned.

https://codereview.chromium.org/10990060/diff/12018/tests/co19/co19-dart2js.s...
File tests/co19/co19-dart2js.status (left):

https://codereview.chromium.org/10990060/diff/12018/tests/co19/co19-dart2js.s...
tests/co19/co19-dart2js.status:756: 
Restore this line.

[ahe, 2012-10-10 13:35:28]

LGTM

[Johnni Winther, 2012-10-10 14:56:00]

https://chromiumcodereview.appspot.com/10990060/diff/12018/lib/compiler/imple...
File lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10990060/diff/12018/lib/compiler/imple...
lib/compiler/implementation/elements/elements.dart:664: message: 'Exports
already set to $slotForExports on $this'));
On 2012/10/10 12:06:27, ahe wrote:
> Long line.

Done.

https://chromiumcodereview.appspot.com/10990060/diff/12018/lib/compiler/imple...
File lib/compiler/implementation/library_loader.dart (right):

https://chromiumcodereview.appspot.com/10990060/diff/12018/lib/compiler/imple...
lib/compiler/implementation/library_loader.dart:24: LibraryDependency tag);
On 2012/10/10 12:06:27, ahe wrote:
> Add newline.

Done.

https://chromiumcodereview.appspot.com/10990060/diff/12018/lib/compiler/imple...
lib/compiler/implementation/library_loader.dart:26: * Adds the elements in the
export scope if [importedLibrary] to the import
On 2012/10/10 12:06:27, ahe wrote:
> if -> of

Done.

https://chromiumcodereview.appspot.com/10990060/diff/12018/lib/compiler/imple...
lib/compiler/implementation/library_loader.dart:42: class LibraryLoaderTask
extends CompilerTask implements LibraryLoader {
On 2012/10/10 12:06:27, ahe wrote:
> I feel it would be simpler to extend LibraryLoader and have LibaryLoader
extend
> CompilerTask.

Done.

https://chromiumcodereview.appspot.com/10990060/diff/12018/lib/compiler/imple...
lib/compiler/implementation/library_loader.dart:61: LibraryElement
loadLibrary(Uri uri, Node node, Uri canonicalUri) {
On 2012/10/10 12:06:27, ahe wrote:
> Do you have to repeat documentation like this?

No. Docs removed.

https://chromiumcodereview.appspot.com/10990060/diff/12018/lib/compiler/imple...
lib/compiler/implementation/library_loader.dart:105: // It is not safe to import
other libraries at this point as
On 2012/10/10 12:06:27, ahe wrote:
> Isn't this comment outdated by now?

Yes. Removed.

https://chromiumcodereview.appspot.com/10990060/diff/12018/lib/compiler/imple...
lib/compiler/implementation/library_loader.dart:139: // Now that we have
processed all the source tags, it is safe to
On 2012/10/10 12:06:27, ahe wrote:
> Is this still relevant?

No. Removed.

https://chromiumcodereview.appspot.com/10990060/diff/12018/lib/compiler/imple...
lib/compiler/implementation/library_loader.dart:198: * is, any resolution should
be done beforehand.
On 2012/10/10 12:06:27, ahe wrote:
> resolution -> URI resolution.

Done.

https://chromiumcodereview.appspot.com/10990060/diff/12018/lib/compiler/imple...
lib/compiler/implementation/library_loader.dart:200: void loadPart(Part part,
Uri path, LibraryElement library) {
On 2012/10/10 12:06:27, ahe wrote:
> if (!path.isAbsolute()) throw ArgumenError(path);

Done.

https://chromiumcodereview.appspot.com/10990060/diff/12018/lib/compiler/imple...
lib/compiler/implementation/library_loader.dart:200: void loadPart(Part part,
Uri path, LibraryElement library) {
On 2012/10/10 12:06:27, ahe wrote:
> Rename method to "scanPart" or "scanCompilationUnit"?

Done.

https://chromiumcodereview.appspot.com/10990060/diff/12018/lib/compiler/imple...
lib/compiler/implementation/library_loader.dart:252:
compiler.onLibraryLoaded(library, uri);
On 2012/10/10 12:06:27, ahe wrote:
> So this is called before the library is fully loaded?

Yes. Comment added on [onLibraryLoaded].

https://chromiumcodereview.appspot.com/10990060/diff/12018/lib/compiler/imple...
lib/compiler/implementation/library_loader.dart:268: 'no #library tag found in
${importedLibrary.uri}');
On 2012/10/10 12:06:27, ahe wrote:
> Why is this error reported twice?

Bad rebase. Removed from here.

https://chromiumcodereview.appspot.com/10990060/diff/12018/lib/compiler/imple...
lib/compiler/implementation/library_loader.dart:293: compiler.reportWarning(new
Identifier(existing.position()),
On 2012/10/10 12:06:27, ahe wrote:
> This can be cleaned up. See checkDuplicatedLibraryName above.

Added a TODO.

https://chromiumcodereview.appspot.com/10990060/diff/12018/lib/compiler/imple...
lib/compiler/implementation/library_loader.dart:456:
exportScope.putIfAbsent(name, () => element);
On 2012/10/10 12:06:27, ahe wrote:
> Another weird use of putIfAbsent.

Done.

https://chromiumcodereview.appspot.com/10990060/diff/12018/lib/compiler/imple...
File lib/compiler/implementation/tree/nodes.dart (right):

https://chromiumcodereview.appspot.com/10990060/diff/12018/lib/compiler/imple...
lib/compiler/implementation/tree/nodes.dart:22: R visitExport(Export node) =>
visitLibraryTag(node);
On 2012/10/10 12:06:27, ahe wrote:
> Should return "visitLibraryDependency(node)"

Done.

https://chromiumcodereview.appspot.com/10990060/diff/12018/lib/compiler/imple...
lib/compiler/implementation/tree/nodes.dart:32: R visitImport(Import node) =>
visitLibraryTag(node);
On 2012/10/10 12:06:27, ahe wrote:
> Ditto.

Done.

https://chromiumcodereview.appspot.com/10990060/diff/12018/tests/co19/co19-da...
File tests/co19/co19-dart2js.status (left):

https://chromiumcodereview.appspot.com/10990060/diff/12018/tests/co19/co19-da...
tests/co19/co19-dart2js.status:756: 
On 2012/10/10 12:06:27, ahe wrote:
> Restore this line.

Done.