Lazily throw pubspec parsing exceptions.

Lazily throw pubspec parsing exceptions.

The user-visible effect of this is to make operations that only look at part of
a pubspec work if other parts of the pubspec are somehow invalid. This is
particularly important for the following scenario:

1. We release a version of pub in SDK 1.0.0 that parses pubspec field "foo" as
   an integer.

2. Sally uploads version 0.1.0 of her package, libsally.

3. We release a new version of pub in SDK 1.1.0 that supports strings in "foo".

4. Sally uploads libsally 0.2.0, which uses a string in "foo". The new version
   of libsally has an SDK constraint of ">= 1.1.0", since it uses the new "foo"
   feature.

5. Tom is using SDK 1.0.0 and runs "pub install" in a package that depends on
   libsally.

Everyone's done everything right here; we made a backwards-compatible pubspec
change, Sally chose the correct SDK constraint, and Tom has a dependency on a
package that has a compatible version available.

However, prior to this change, Tom's "pub install" would crash. His outdated
version of pub would try and fail to parse libsally 0.2.0's "foo" field, and his
version resolution would crash. With this change, it will only attempt to parse
the SDK constraint portion of Sally's pubspec, and ignore anything else it
doesn't understand.

R=rnystrom@google.com
BUG=

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

[Bob Nystrom, 2013-09-23 17:11:22]

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/pubspec.dart (right):

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:31: /// This will be null if this was
created this [new Pubspec] or [new
"created this" -> "created using"

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:43: final Map fields;
Can we make this private?

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:45: /// This package's name.
Nit: "This" -> "The".

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:63: /// This package's version.
Ditto.

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:91: /// The packages this package
depends on when it is the root package.
"packages" -> "additional packages".

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:142: id =
libraryIdentifierToId(transformer.keys.single);
Do we need to handle a FormatException from here?

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:153: 'that\'s not listed in
"dependencies".');
Pathological question: can you have a dev dependency on a transformer? Does it
make sense to have dev_transformers?

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:260: // into load(), parse(), and
_parse() like LockFile does.
I think you can remove this TODO now.

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:281: /// Returns a list of all errors
in this pubspec.
Technically, it doesn't get all of them. A given field may contain multiple
errors but will only throw on the first. For example, if you have:

dependencies:
  123: any
  234: any

There are two errors since both "123" and "234" are not strings, but we'll only
get the first.

Maybe just document this more clearly.

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:297: _getError(() =>
this.environment);
I worry that we'll add more lazy-initialized fields to Pubspec and forget to
check them here. Maybe add a doc comment at the top of the class reminding us to
add here?

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:318: 'dependency.');
Would be good to say "dev dependency" here when those are being parsed.

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:399: devDependencies.map((dep) =>
dep.name).toSet());
Calling dependencies and devDependencies recursively in here skeezes me out.

How about calling this from _parseDependencies() and having it use _dependencies
and _devDependencies instead? It can then bail if either of them are null.

You'd probably want to have dependencies and devDependencies set their
respective _ field first and pass in the collection, so that it exists when
_parseDependencies() is invoked.

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:417: var errorString =
e.toString().replaceFirst('FormatException: ', '');
Can you just use e.message here?

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:454: /// This can be null if the
pubspec didn't specify a name and no external name
"can" -> "may" here and below.

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/utils.dart (right):

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/utils.dart:517: if (split[0] == '..' && split[1]
== '..') return path.absolute(inputPath);
Need to make sure split has length > 1 first.

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/test/pu...
File sdk/lib/_internal/pub/test/pubspec_test.dart (left):

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/test/pu...
sdk/lib/_internal/pub/test/pubspec_test.dart:214: expectFormatError('{author:
abe, authors: ted}');
Can you move all of these tests over to validator? I see tests for the scheme
for homepage and documentation, but not the other stuff.

[nweiz, 2013-09-23 22:06:10]

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/pubspec.dart (right):

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:31: /// This will be null if this was
created this [new Pubspec] or [new
On 2013/09/23 17:11:22, Bob Nystrom wrote:
> "created this" -> "created using"

Done.

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:43: final Map fields;
On 2013/09/23 17:11:22, Bob Nystrom wrote:
> Can we make this private?

It's used in the pubspec field validator.

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:45: /// This package's name.
On 2013/09/23 17:11:22, Bob Nystrom wrote:
> Nit: "This" -> "The".

Done.

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:63: /// This package's version.
On 2013/09/23 17:11:22, Bob Nystrom wrote:
> Ditto.

Done.

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:91: /// The packages this package
depends on when it is the root package.
On 2013/09/23 17:11:22, Bob Nystrom wrote:
> "packages" -> "additional packages".

Done.

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:142: id =
libraryIdentifierToId(transformer.keys.single);
On 2013/09/23 17:11:22, Bob Nystrom wrote:
> Do we need to handle a FormatException from here?

Yes, done.

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:153: 'that\'s not listed in
"dependencies".');
On 2013/09/23 17:11:22, Bob Nystrom wrote:
> Pathological question: can you have a dev dependency on a transformer? Does it
> make sense to have dev_transformers?

Let's cross that bridge when we come to it. If users want it, it will be a
backwards-compatible change.

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:260: // into load(), parse(), and
_parse() like LockFile does.
On 2013/09/23 17:11:22, Bob Nystrom wrote:
> I think you can remove this TODO now.

Done.

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:281: /// Returns a list of all errors
in this pubspec.
On 2013/09/23 17:11:22, Bob Nystrom wrote:
> Technically, it doesn't get all of them. A given field may contain multiple
> errors but will only throw on the first. For example, if you have:
> 
> dependencies:
>   123: any
>   234: any
> 
> There are two errors since both "123" and "234" are not strings, but we'll
only
> get the first.
> 
> Maybe just document this more clearly.

Done.

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:297: _getError(() =>
this.environment);
On 2013/09/23 17:11:22, Bob Nystrom wrote:
> I worry that we'll add more lazy-initialized fields to Pubspec and forget to
> check them here. Maybe add a doc comment at the top of the class reminding us
to
> add here?

It doesn't seem like that makes sense in the public documentation, but I've
added it as a non-doc comment.

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:318: 'dependency.');
On 2013/09/23 17:11:22, Bob Nystrom wrote:
> Would be good to say "dev dependency" here when those are being parsed.

[field] will include "dev_depenendency", so I don't think it's necessary. The
general principle is that packages can't depend on themselves.

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:399: devDependencies.map((dep) =>
dep.name).toSet());
On 2013/09/23 17:11:22, Bob Nystrom wrote:
> Calling dependencies and devDependencies recursively in here skeezes me out.
> 
> How about calling this from _parseDependencies() and having it use
_dependencies
> and _devDependencies instead? It can then bail if either of them are null.
> 
> You'd probably want to have dependencies and devDependencies set their
> respective _ field first and pass in the collection, so that it exists when
> _parseDependencies() is invoked.

Done, sort of. I really dislike passing in collections to be mutated rather than
just returning new ones, so I've structured this slightly differently than your
suggestion.

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:417: var errorString =
e.toString().replaceFirst('FormatException: ', '');
On 2013/09/23 17:11:22, Bob Nystrom wrote:
> Can you just use e.message here?

Yeah, not sure why I didn't.

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:454: /// This can be null if the
pubspec didn't specify a name and no external name
On 2013/09/23 17:11:22, Bob Nystrom wrote:
> "can" -> "may" here and below.

That feels wrong to me. I like "may" when I'm talking about what users are
allowed to do, but "can" seems more accurate when I'm talking about what values
an API may emit.

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/utils.dart (right):

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/utils.dart:517: if (split[0] == '..' && split[1]
== '..') return path.absolute(inputPath);
On 2013/09/23 17:11:22, Bob Nystrom wrote:
> Need to make sure split has length > 1 first.

Done.

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/test/pu...
File sdk/lib/_internal/pub/test/pubspec_test.dart (left):

https://codereview.chromium.org/24246002/diff/1/sdk/lib/_internal/pub/test/pu...
sdk/lib/_internal/pub/test/pubspec_test.dart:214: expectFormatError('{author:
abe, authors: ted}');
On 2013/09/23 17:11:22, Bob Nystrom wrote:
> Can you move all of these tests over to validator? I see tests for the scheme
> for homepage and documentation, but not the other stuff.

Done.

[Bob Nystrom, 2013-09-23 22:09:27]

LGTM!

[nweiz, 2013-09-23 22:21:52]

Committed patchset #2 manually as r27784 (presubmit successful).