Lexical support for configurable imports (DEP 40)

Lexical support for configurable imports (DEP 40)

R=paulberry@google.com

Committed: https://github.com/dart-lang/sdk/commit/4e3a1a468a0b37036a3f8ec5b210784a39a56525

[Brian Wilkerson, 2015-10-21 15:30:28]

brianwilkerson@google.com changed reviewers:
+ paulberry@google.com

[Paul Berry, 2015-10-21 17:10:48]

As we discussed earlier, this probably shouldn't be landed until after 1.13 is
released because it's a breaking change.

With that caveat, lgtm.

https://codereview.chromium.org/1406253004/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/ast.dart (right):

https://codereview.chromium.org/1406253004/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/ast.dart:6574: class DottedName extends AstNode {
This looks almost identical to the definition of LibraryIdentifier (in fact it
looks like you copied it and removed some methods).  Is there any way we could
share the implementations rather than duplicate code?

(I suspect that the extra methods in LibraryIdentifier, and the fact that it
extends Identifier, are not actually necessary, because LibraryIdentifier is
only syntactically valid inside "library" and "part of" declarations, where the
expression semantics don't make sense.  So maybe we could just drop
LibraryIdentifier entirely and use DottedName in its place.  That's a breaking
change, of course, but so is the rest of this CL).

https://codereview.chromium.org/1406253004/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/ast.dart:6581: * Initialize a newly created
prefixed identifier.
s/prefixed identifier/dotted name/

https://codereview.chromium.org/1406253004/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/parser.dart (right):

https://codereview.chromium.org/1406253004/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/parser.dart:5290: if (configuration != null) {
This is unnecessarily defensive.  _parseConfiguration() can never return null.

https://codereview.chromium.org/1406253004/diff/1/pkg/analyzer/test/generated...
File pkg/analyzer/test/generated/parser_test.dart (right):

https://codereview.chromium.org/1406253004/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/parser_test.dart:7284: // TODO(brianwilkerson) Test
the configurations.
I'd recommend resolving these testing TODOs before landing the CL.  (Otherwise I
think there's a big risk of us getting busy and never getting back to it).

Note that since the tests for parseConfiguration are pretty thorough I think
it's ok to just spot check the configurations here to make sure that there's the
right number of them and they're in the right order.  E.g. I think this would be
adequate:

expect(directive.configurations, hasLength(2));
_expectDottedName(directive.configurations[0].name, ['a']);
_expectDottedName(directive.configurations[1].name, ['b']);

[Brian Wilkerson, 2015-10-21 20:05:23]

https://codereview.chromium.org/1406253004/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/ast.dart (right):

https://codereview.chromium.org/1406253004/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/ast.dart:6574: class DottedName extends AstNode {
I like the idea of removing LibraryIdentifier. I'll look into it.

https://codereview.chromium.org/1406253004/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/ast.dart:6581: * Initialize a newly created
prefixed identifier.
Done

https://codereview.chromium.org/1406253004/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/parser.dart (right):

https://codereview.chromium.org/1406253004/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/parser.dart:5290: if (configuration != null) {
Done

https://codereview.chromium.org/1406253004/diff/1/pkg/analyzer/test/generated...
File pkg/analyzer/test/generated/parser_test.dart (right):

https://codereview.chromium.org/1406253004/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/parser_test.dart:7284: // TODO(brianwilkerson) Test
the configurations.
Done

[Bob Nystrom, 2015-10-22 00:24:46]

rnystrom@google.com changed reviewers:
+ rnystrom@google.com

[Bob Nystrom, 2015-10-22 00:24:47]

Drive-by!

https://codereview.chromium.org/1406253004/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/parser.dart (right):

https://codereview.chromium.org/1406253004/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/parser.dart:5273: value = parseStringLiteral();
This will allow interpolation, right? The proposal forbids that, but if you
report that error later, I suppose that's fine too.

[Brian Wilkerson, 2015-10-22 02:33:42]

https://codereview.chromium.org/1406253004/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/parser.dart (right):

https://codereview.chromium.org/1406253004/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/parser.dart:5273: value = parseStringLiteral();
Good catch. Yes, it will allow string interpolation. The analyzer's parser
excepts a superset of the Dart language, then adds warnings where appropriate.
(In this case, I have added one here.)

[Brian Wilkerson, 2015-12-02 19:20:11]

Description was changed from

==========
Lexical support for configurable imports (DEP 40)
==========

to

==========
Lexical support for configurable imports (DEP 40)

R=paulberry@google.com

Committed:
https://github.com/dart-lang/sdk/commit/4e3a1a468a0b37036a3f8ec5b210784a39a56525
==========

[Brian Wilkerson, 2015-12-02 19:20:13]

Committed patchset #1 (id:1) manually as
4e3a1a468a0b37036a3f8ec5b210784a39a56525 (presubmit successful).