Change the deferred library check tool to allow unspecified packages to be in

Change the deferred library check tool to allow unspecified packages to be in
 any part. Also allow part specifications to specifically exclude packages.

BUG=
R=sigmund@google.com

Committed: https://github.com/dart-lang/dart2js_info/commit/44ab8ea4359a24dc3d1653240b1239b5cb6df43f

[Harry Terkelsen, 2015-12-18 01:44:20]

het@google.com changed reviewers:
+ sigmund@google.com

[Siggi Cherem (dart-lang), 2015-12-18 02:29:41]

https://codereview.chromium.org/1535893002/diff/1/README.md
File README.md (right):

https://codereview.chromium.org/1535893002/diff/1/README.md#newcode211
README.md:211: - some_package
I like that the new syntax allows you to extend this to support libraries
without a breaking change. You would be able to say that if the item is an
identifier it is the name of the package, but if it's a URI, it is the specific
library with that uri.

https://codereview.chromium.org/1535893002/diff/1/README.md#newcode240
README.md:240: instance, if you have `import 'package:foo/foo.dart' deferred as
foo;` in your
to make it less ambiguous, it might be worth changing this example to:

`import 'package:foo/bar.dart' deferred as baz`

Or:
`import 'package:pkg/lib.dart' deferred as prefix` and say to use 'pkg'

https://codereview.chromium.org/1535893002/diff/1/lib/deferred_library_check....
File lib/deferred_library_check.dart (right):

https://codereview.chromium.org/1535893002/diff/1/lib/deferred_library_check....
lib/deferred_library_check.dart:15: ///         - foo
should we always implicitly include the foo package in a part named foo?

https://codereview.chromium.org/1535893002/diff/1/lib/deferred_library_check....
lib/deferred_library_check.dart:36: /// instance, if you have `import
'package:foo/foo.dart' deferred as foo;` in
same here

https://codereview.chromium.org/1535893002/diff/1/lib/deferred_library_check....
lib/deferred_library_check.dart:63: List<String> validParts = ['main']
we might want to validate that there are no 2 parts with the same name (if 2
libraries on the same package are deferred separately).

Not yet needed by our customers, but I'm sure it wont be long until they hit
that issue.

https://codereview.chromium.org/1535893002/diff/1/lib/deferred_library_check....
lib/deferred_library_check.dart:66: .map((unit) => unit.imports.single));
do we need to get the package name from the library URI here?

[Harry Terkelsen, 2015-12-18 21:22:21]

I decided to name the parts after the "as" name rather than the package name
because this was how I was seeing it done by customers in an example manifest
file. I think it makes sense because the "as" name is already a user-defined
identifier for what the user expects to define a deferred part.

https://codereview.chromium.org/1535893002/diff/1/README.md
File README.md (right):

https://codereview.chromium.org/1535893002/diff/1/README.md#newcode211
README.md:211: - some_package
On 2015/12/18 02:29:41, Siggi Cherem (dart-lang) wrote:
> I like that the new syntax allows you to extend this to support libraries
> without a breaking change. You would be able to say that if the item is an
> identifier it is the name of the package, but if it's a URI, it is the
specific
> library with that uri.
Right, we can add that when customers want to check at the library level.

https://codereview.chromium.org/1535893002/diff/1/README.md#newcode240
README.md:240: instance, if you have `import 'package:foo/foo.dart' deferred as
foo;` in your
On 2015/12/18 02:29:41, Siggi Cherem (dart-lang) wrote:
> to make it less ambiguous, it might be worth changing this example to:
> 
> `import 'package:foo/bar.dart' deferred as baz`
> 
> Or:
> `import 'package:pkg/lib.dart' deferred as prefix` and say to use 'pkg'

It's actually the other way. The part after 'as' is the name of the part.
Otherwise, there is no way to defer import 2 different libraries in the same
package. This is also the way that I see customers naturally naming their parts
in the spec file

https://codereview.chromium.org/1535893002/diff/1/lib/deferred_library_check....
File lib/deferred_library_check.dart (right):

https://codereview.chromium.org/1535893002/diff/1/lib/deferred_library_check....
lib/deferred_library_check.dart:15: ///         - foo
On 2015/12/18 02:29:41, Siggi Cherem (dart-lang) wrote:
> should we always implicitly include the foo package in a part named foo?

What I'm seeing is more like:

import 'package:foo.bar.baz/baz.dart' as baz;

and then in the specification file this part is named baz. So the name of the
part is not necessarily the same as the package that the deferred library is in.

https://codereview.chromium.org/1535893002/diff/1/lib/deferred_library_check....
lib/deferred_library_check.dart:36: /// instance, if you have `import
'package:foo/foo.dart' deferred as foo;` in
On 2015/12/18 02:29:41, Siggi Cherem (dart-lang) wrote:
> same here

Done.

https://codereview.chromium.org/1535893002/diff/1/lib/deferred_library_check....
lib/deferred_library_check.dart:63: List<String> validParts = ['main']
On 2015/12/18 02:29:41, Siggi Cherem (dart-lang) wrote:
> we might want to validate that there are no 2 parts with the same name (if 2
> libraries on the same package are deferred separately).
> 
> Not yet needed by our customers, but I'm sure it wont be long until they hit
> that issue.

Do you still think we should do this since the names of the parts are the part
after the "as" instead of the package name?

https://codereview.chromium.org/1535893002/diff/1/lib/deferred_library_check....
lib/deferred_library_check.dart:66: .map((unit) => unit.imports.single));
On 2015/12/18 02:29:41, Siggi Cherem (dart-lang) wrote:
> do we need to get the package name from the library URI here? 

unit.imports is a list of the names of the deferred imports. For instance the
part created when there is "import 'package:foo/bar.dart' deferred as baz;" has
a length-1 list of imports ["baz"].

[Siggi Cherem (dart-lang), 2015-12-18 21:34:25]

lgtm

https://codereview.chromium.org/1535893002/diff/1/README.md
File README.md (right):

https://codereview.chromium.org/1535893002/diff/1/README.md#newcode240
README.md:240: instance, if you have `import 'package:foo/foo.dart' deferred as
foo;` in your
On 2015/12/18 21:22:21, Harry Terkelsen wrote:
> On 2015/12/18 02:29:41, Siggi Cherem (dart-lang) wrote:
> > to make it less ambiguous, it might be worth changing this example to:
> > 
> > `import 'package:foo/bar.dart' deferred as baz`
> > 
> > Or:
> > `import 'package:pkg/lib.dart' deferred as prefix` and say to use 'pkg'
> 
> It's actually the other way. The part after 'as' is the name of the part.
> Otherwise, there is no way to defer import 2 different libraries in the same
> package. This is also the way that I see customers naturally naming their
parts
> in the spec file

I see, thanks for clarifying, that's nice, for some reason I thought we didn't
have that info in dump-info

https://codereview.chromium.org/1535893002/diff/1/lib/deferred_library_check....
File lib/deferred_library_check.dart (right):

https://codereview.chromium.org/1535893002/diff/1/lib/deferred_library_check....
lib/deferred_library_check.dart:63: List<String> validParts = ['main']
On 2015/12/18 21:22:21, Harry Terkelsen wrote:
> On 2015/12/18 02:29:41, Siggi Cherem (dart-lang) wrote:
> > we might want to validate that there are no 2 parts with the same name (if 2
> > libraries on the same package are deferred separately).
> > 
> > Not yet needed by our customers, but I'm sure it wont be long until they hit
> > that issue.
> 
> Do you still think we should do this since the names of the parts are the part
> after the "as" instead of the package name?

Good point - nope, it makes sense to leave it as is.

https://codereview.chromium.org/1535893002/diff/1/lib/deferred_library_check....
lib/deferred_library_check.dart:66: .map((unit) => unit.imports.single));
On 2015/12/18 21:22:21, Harry Terkelsen wrote:
> On 2015/12/18 02:29:41, Siggi Cherem (dart-lang) wrote:
> > do we need to get the package name from the library URI here? 
> 
> unit.imports is a list of the names of the deferred imports. For instance the
> part created when there is "import 'package:foo/bar.dart' deferred as baz;"
has
> a length-1 list of imports ["baz"].

interesting - I really thought that these were the import urls, not the
prefixes. 

What happens if two imports use the same prefix word? does that make our
dump-info ambiguous?

I wonder whether we should record the full thing (url + prefix) and validate
that the prefix is unique when we get here.

[Harry Terkelsen, 2015-12-18 21:42:53]

https://codereview.chromium.org/1535893002/diff/1/lib/deferred_library_check....
File lib/deferred_library_check.dart (right):

https://codereview.chromium.org/1535893002/diff/1/lib/deferred_library_check....
lib/deferred_library_check.dart:66: .map((unit) => unit.imports.single));
On 2015/12/18 21:34:25, Siggi Cherem (dart-lang) wrote:
> On 2015/12/18 21:22:21, Harry Terkelsen wrote:
> > On 2015/12/18 02:29:41, Siggi Cherem (dart-lang) wrote:
> > > do we need to get the package name from the library URI here? 
> > 
> > unit.imports is a list of the names of the deferred imports. For instance
the
> > part created when there is "import 'package:foo/bar.dart' deferred as baz;"
> has
> > a length-1 list of imports ["baz"].
> 
> interesting - I really thought that these were the import urls, not the
> prefixes. 
> 
> What happens if two imports use the same prefix word? does that make our
> dump-info ambiguous?
> 
> I wonder whether we should record the full thing (url + prefix) and validate
> that the prefix is unique when we get here.

I think if there are two with the same name then one fill be "prefix" and the
other will be "prefix1"

[Harry Terkelsen, 2015-12-18 21:43:03]

Description was changed from

==========
Change the deferred library check tool to allow unspecified packages to be in
 any part. Also allow part specifications to specifically exclude packages.

BUG=
==========

to

==========
Change the deferred library check tool to allow unspecified packages to be in
 any part. Also allow part specifications to specifically exclude packages.

BUG=
R=sigmund@google.com

Committed:
https://github.com/dart-lang/dart2js_info/commit/44ab8ea4359a24dc3d1653240b12...
==========

[Harry Terkelsen, 2015-12-18 21:43:07]

Committed patchset #2 (id:20001) manually as
44ab8ea4359a24dc3d1653240b1239b5cb6df43f (presubmit successful).

[Siggi Cherem (dart-lang), 2015-12-18 21:47:30]

On 2015/12/18 21:42:53, Harry Terkelsen wrote:
>
https://codereview.chromium.org/1535893002/diff/1/lib/deferred_library_check....
> File lib/deferred_library_check.dart (right):
> 
>
https://codereview.chromium.org/1535893002/diff/1/lib/deferred_library_check....
> lib/deferred_library_check.dart:66: .map((unit) => unit.imports.single));
> On 2015/12/18 21:34:25, Siggi Cherem (dart-lang) wrote:
> > On 2015/12/18 21:22:21, Harry Terkelsen wrote:
> > > On 2015/12/18 02:29:41, Siggi Cherem (dart-lang) wrote:
> > > > do we need to get the package name from the library URI here? 
> > > 
> > > unit.imports is a list of the names of the deferred imports. For instance
> the
> > > part created when there is "import 'package:foo/bar.dart' deferred as
baz;"
> > has
> > > a length-1 list of imports ["baz"].
> > 
> > interesting - I really thought that these were the import urls, not the
> > prefixes. 
> > 
> > What happens if two imports use the same prefix word? does that make our
> > dump-info ambiguous?
> > 
> > I wonder whether we should record the full thing (url + prefix) and validate
> > that the prefix is unique when we get here.
> 
> I think if there are two with the same name then one fill be "prefix" and the
> other will be "prefix1"

in term of the validation file, how do they know which one is prefix1 and which
one is plain prefix?

[Harry Terkelsen, 2015-12-18 21:57:51]

There is no way to disambiguate them currently. This isn't a problem now but
could become one in the future. Perhaps we could also allow URIs to be keys and
if there is a URI key it must match one of the deferred import URIs in the
program