Add prefix constraints for deferred imports.

Add prefix constraints for deferred imports.

R=floitsch@google.com

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

[floitsch, 2014-02-17 14:39:26]

FYI.

https://codereview.chromium.org/169703003/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/deferred_load.dart (right):

https://codereview.chromium.org/169703003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/deferred_load.dart:621: Map<String,
List<Import>> prefixImports = {};
Move outside the loop, and just clear it at every iteration.

https://codereview.chromium.org/169703003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/deferred_load.dart:642: () => new
List<Import>()).add(import);
Instead of allocating a new closure/list every time, can't we just report the
error here?

Since you don't use the original import in the error-message, you can even
switch to a Setlet for the used prefixes.

Something along those lines:

usedPrefixes = new Setlet<String>();

if (usedPrefixes.contains(import.prefix.toString()) {
  if (!_isImportDeferred(import)) {
    // Find deferred import with same prefix and assign it to "import".
  }
  compiler.reportError(import, MessageKind.DEFERRED_LIBRARY_DUPLICATE_PREFIX);
} else {
  usedPrefixes.add(import.prefix.toString());
}

https://codereview.chromium.org/169703003/diff/1/tests/language/deferred_cons...
File tests/language/deferred_constraints_lib.dart (right):

https://codereview.chromium.org/169703003/diff/1/tests/language/deferred_cons...
tests/language/deferred_constraints_lib.dart:1: class C {}
Copyright lines missing.

https://codereview.chromium.org/169703003/diff/1/tests/language/deferred_cons...
File tests/language/deferred_constraints_lib2.dart (right):

https://codereview.chromium.org/169703003/diff/1/tests/language/deferred_cons...
tests/language/deferred_constraints_lib2.dart:1: foo () => 42;
Copyright lines missing.

[sigurdm, 2014-02-18 08:49:52]

https://codereview.chromium.org/169703003/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/deferred_load.dart (right):

https://codereview.chromium.org/169703003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/deferred_load.dart:621: Map<String,
List<Import>> prefixImports = {};
On 2014/02/17 14:39:26, floitsch wrote:
> Move outside the loop, and just clear it at every iteration.

Done.

https://codereview.chromium.org/169703003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/deferred_load.dart:642: () => new
List<Import>()).add(import);
On 2014/02/17 14:39:26, floitsch wrote:
> Instead of allocating a new closure/list every time, can't we just report the
> error here?
> 
> Since you don't use the original import in the error-message, you can even
> switch to a Setlet for the used prefixes.
> 
> Something along those lines:
> 
> usedPrefixes = new Setlet<String>();
> 
> if (usedPrefixes.contains(import.prefix.toString()) {
>   if (!_isImportDeferred(import)) {
>     // Find deferred import with same prefix and assign it to "import".
>   }
>   compiler.reportError(import, MessageKind.DEFERRED_LIBRARY_DUPLICATE_PREFIX);
> } else {
>   usedPrefixes.add(import.prefix.toString());
> }
> 

I wanted to do something like that first - but it seemed less simple to make
that work.

We still want to allow collisions for non-deferred imports:

import 'a.dart' as a;
import 'b.dart' as a;

So we would have to remember for each prefix both if we saw "any imports" and if
we saw "any deferred imports".

https://codereview.chromium.org/169703003/diff/1/tests/language/deferred_cons...
File tests/language/deferred_constraints_lib.dart (right):

https://codereview.chromium.org/169703003/diff/1/tests/language/deferred_cons...
tests/language/deferred_constraints_lib.dart:1: class C {}
On 2014/02/17 14:39:26, floitsch wrote:
> Copyright lines missing.

Done.

https://codereview.chromium.org/169703003/diff/1/tests/language/deferred_cons...
File tests/language/deferred_constraints_lib2.dart (right):

https://codereview.chromium.org/169703003/diff/1/tests/language/deferred_cons...
tests/language/deferred_constraints_lib2.dart:1: foo () => 42;
On 2014/02/17 14:39:26, floitsch wrote:
> Copyright lines missing.

Done.

https://codereview.chromium.org/169703003/diff/1/tests/language/deferred_cons...
File tests/language/deferred_constraints_test.dart (right):

https://codereview.chromium.org/169703003/diff/1/tests/language/deferred_cons...
tests/language/deferred_constraints_test.dart:1: import "dart:async";
Also missing copyright here (fixed).

[floitsch, 2014-02-18 09:45:22]

LGTM with more tests.

https://codereview.chromium.org/169703003/diff/140001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/deferred_load.dart (right):

https://codereview.chromium.org/169703003/diff/140001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/deferred_load.dart:641: if
(import.prefix != null) {
Add comment explaining why simpler approaches wouldn't work.
Make sure to have test-cases.

(Why do you need a list of imports, ...).

[sigurdm, 2014-02-18 11:58:29]

I implemented a version with the tests inside the loop. It might not be so bad
after all.

I also added a few more tests.

[floitsch, 2014-02-18 12:19:25]

LGTM.

https://codereview.chromium.org/169703003/diff/100002/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/deferred_load.dart (right):

https://codereview.chromium.org/169703003/diff/100002/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/deferred_load.dart:648: String prefix
= import.prefix != null ? import.prefix.toString() : null;
parenthesis around the condition.

https://codereview.chromium.org/169703003/diff/100002/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/deferred_load.dart:669: Import
failingImport = previousDeferredImport != null
parenthesis around condition.

https://codereview.chromium.org/169703003/diff/100002/tests/language/deferred...
File tests/language/deferred_constraints_test.dart (right):

https://codereview.chromium.org/169703003/diff/100002/tests/language/deferred...
tests/language/deferred_constraints_test.dart:11: @lazy2 import
"deferred_constraints_lib2.dart" as lib; /// duplicate_prefix3: compile-time
error
I would prefer new tests.

[sigurdm, 2014-02-18 13:31:46]

https://codereview.chromium.org/169703003/diff/100002/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/deferred_load.dart (right):

https://codereview.chromium.org/169703003/diff/100002/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/deferred_load.dart:648: String prefix
= import.prefix != null ? import.prefix.toString() : null;
On 2014/02/18 12:19:25, floitsch wrote:
> parenthesis around the condition.

Done.

https://codereview.chromium.org/169703003/diff/100002/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/deferred_load.dart:669: Import
failingImport = previousDeferredImport != null
On 2014/02/18 12:19:25, floitsch wrote:
> parenthesis around condition.

Done.

https://codereview.chromium.org/169703003/diff/100002/tests/language/deferred...
File tests/language/deferred_constraints_test.dart (right):

https://codereview.chromium.org/169703003/diff/100002/tests/language/deferred...
tests/language/deferred_constraints_test.dart:11: @lazy2 import
"deferred_constraints_lib2.dart" as lib; /// duplicate_prefix3: compile-time
error
On 2014/02/18 12:19:25, floitsch wrote:
> I would prefer new tests.

Ok, I have split the test.

[sigurdm, 2014-02-21 09:40:11]

Committed patchset #5 manually as r32881 (presubmit successful).