Package map source factory support.

Package map source factory support.

R=brianwilkerson@google.com

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

[pquitslund, 2015-06-30 22:57:30]

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

[pquitslund, 2015-06-30 22:57:30]

Still a bit of a work in progress but there's enough there to start iterating.

Comments most welcome!

[Brian Wilkerson, 2015-06-30 23:29:38]

LGTM

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

https://codereview.chromium.org/1206323005/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/source.dart:771: if (uri.scheme != 'file') {
I'm a little concerned that longer term we'll need to handle non-file uri's. But
I guess we can deal with that later. (Perhaps add a TODO to remind us.)

[pquitslund, 2015-06-30 23:46:06]

Thanks Brian!

After chatting with Paul, I made some changes to make SourceFactory a bit more
test-friendly WRT to injecting ResourceProviders.

Let me know what you think.

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

https://codereview.chromium.org/1206323005/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/source.dart:771: if (uri.scheme != 'file') {
On 2015/06/30 23:29:38, Brian Wilkerson wrote:
> I'm a little concerned that longer term we'll need to handle non-file uri's.
But
> I guess we can deal with that later. (Perhaps add a TODO to remind us.)

Right.  Added TODO.

[Brian Wilkerson, 2015-07-01 00:43:23]

Other than a small bit of confusion, LGTM.

https://codereview.chromium.org/1206323005/diff/20001/pkg/analyzer/test/gener...
File pkg/analyzer/test/generated/source_factory_test.dart (right):

https://codereview.chromium.org/1206323005/diff/20001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/source_factory_test.dart:67: SourceFactory factory =
new SourceFactory(resolvers, packages);
I was expecting to see the SourceFactories get created with an
InMemoryResourceProvider for performance reasons, but maybe it isn't necessary.

[Paul Berry, 2015-07-01 15:55:40]

https://codereview.chromium.org/1206323005/diff/20001/pkg/analyzer/lib/file_s...
File pkg/analyzer/lib/file_system/physical_file_system.dart (right):

https://codereview.chromium.org/1206323005/diff/20001/pkg/analyzer/lib/file_s...
pkg/analyzer/lib/file_system/physical_file_system.dart:48: File getFile(String
path) => new _PhysicalFile(new io.File(path));
These two methods should be marked with "@override"

Also, typically our convention is not to duplicate the documentation comment for
overridden methods that are already documented in the base class.

https://codereview.chromium.org/1206323005/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/source.dart (right):

https://codereview.chromium.org/1206323005/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/source.dart:775: return
_getPackageMapping(source.uri);
I don't think we want this.  The idea of restoreUri is that it should look at
the source's file path only, and determine a URI that resolves to the given file
path.  Which is what the loop above does.  If the loop above fails to create a
URI, then we should return null because there is no possible URI that resolves
to the given source.

https://codereview.chromium.org/1206323005/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/source.dart:778: Uri _getPackageMapping(Uri uri)
{
I'm concerned about the algorithm for this function.  It seems to me that (a)
there's no need to restrict it to operating on just file URI's, and (b) we
shouldn't be converting the URI to a file path (because that will introduce
unintended behavioral differences between Windows and non-Windows environments).
 We should simply iterate through the key/value pairs in _packages seeing if any
of the values is a valid prefix for the passed-in URI, and then convert it as
appropriate.

https://codereview.chromium.org/1206323005/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/source.dart:789: if
(sourcePath.startsWith(uriPath)) {
Is it guaranteed that "uriPath" will end with a trailing slash?  If not, this
won't work properly because it will falsely match a path like "/foobar/baz" to
the uriPath "/foo".

Consider instead using Uri.pathSegments on both URI's that you are comparing,
and then comparing the path segments one by one.

https://codereview.chromium.org/1206323005/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/source.dart:810: // Check .packages first.
This needs to go after the "if (!containedUri.isAbsolute)" block.  Otherwise we
will resolve relative URI's incorrectly.

https://codereview.chromium.org/1206323005/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/source.dart:814: if (packageUri != null &&
packageUri.scheme == '') {
To reduce churn once the TODO above is addressed, let's write this in a way that
it will do what we want once package_config has been updated.  E.g.:

if (packageUri != null) {
  if (packageUri.scheme = '') {
    packageUri = packageUri.replace(scheme: 'file');
  }
  containedUri = packageUri;
}

Also, it might be good to add a TODO to remind us of the problem that Brian
mentioned in his email this morning (this algorithm will cause us to create a
Source object that contains the wrong URI).

[pquitslund, 2015-07-06 18:13:24]

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