Actual URI support for package URI resolution.

Actual URI support for package URI resolution.

R=brianwilkerson@google.com, paulberry@google.com

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

[pquitslund, 2015-07-21 18:14:24]

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

[pquitslund, 2015-07-21 18:14:24]

I'll follow up with version (and DEP) updates.  I'm thinking about more tests as
well.

[Brian Wilkerson, 2015-07-21 18:48:34]

> I'll follow up with version (and DEP) updates.

I'm uncomfortable waiting for the version update. John wants to publish a new
version and I don't want a version to get published with these changes but with
no version update.

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

https://codereview.chromium.org/1245263002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/source.dart:819: Uri actualUri = null;
I'm not convinced that it's ever necessary to pass in `null` rather than the
containedUri, and the code would be easier to read and maintain with fewer
variables. If there's a case I'm not thinking of, a comment explaining it would
be helpful; otherwise a TODO to explore cleaning up the code would be good.

[Paul Berry, 2015-07-21 19:39:27]

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

https://codereview.chromium.org/1245263002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/source.dart:819: Uri actualUri = null;
On 2015/07/21 18:48:34, Brian Wilkerson wrote:
> I'm not convinced that it's ever necessary to pass in `null` rather than the
> containedUri, and the code would be easier to read and maintain with fewer
> variables. If there's a case I'm not thinking of, a comment explaining it
would
> be helpful; otherwise a TODO to explore cleaning up the code would be good.

I agree--AFAIK, always passing containedUri to resolveAbsolute() should produce
correct results all the time.

https://codereview.chromium.org/1245263002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/source.dart:1075: * @param actualUri the actual
uri for this source
We should document that the caller is allowed to pass null, in which case the
source that's created will have a URI of [uri].

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

https://codereview.chromium.org/1245263002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/source_io.dart:283: return new
FileBasedSource(new JavaFile.fromUri(uri), actualUri);
There's a subtle change in behavior in the case where actualUri is null that I'm
not sure is on purpose.  Previously, the new Source object would receive its URI
directly from the "uri" parameter.  Now, when actualUri is null, the Source
object derives its URI from JavaFile.toURI().  I suspect this is ok (since all
JavaFile.toURI() does is absolutize and normalize the path, and I *think* that
the URI's should have been absolutized and normalized before they get here). 
But I would feel more comfortable if either:

(a) we changed the code to match the old behavior when actualUri == null, or
(b) we changed actualUri to a non-optional parameter and ensured that a non-null
value was always passed in (along the lines of Brian's suggestion in his comment
on pkg/analyzer/lib/src/generated/source.dart)

https://codereview.chromium.org/1245263002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/source_io.dart:466:
getCanonicalFile(_packagesDirectories[0], pkgName, relPath), actualUri);
This has the same problem as above, but it's definitely wrong in this case
because we're resolving a package: URI, so passing null (and letting the URI be
chosen by FileBasedSource from the JavaFile will get us the wrong kind of URI). 
We need logic so that if actualUri is null, we pass in uri.

I'm kind of surprised this didn't lead to any test failures, so I'm wondering if
we have a gap in our test coverage.

https://codereview.chromium.org/1245263002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/source_io.dart:548: return new
FileBasedSource(file, actualUri);
Similar problem here, although I can't assess the impact because I can't find
any code that uses this class (RelativeFileResolver)--not even tests.

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

https://codereview.chromium.org/1245263002/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/source_factory_test.dart:183: createSource(path:
uriPath);
Not sure if it matters because this is test code, but technically to satisfy the
interface contract for UriResolver, we should pass actualUri through to
createSource().

[pquitslund, 2015-07-21 20:55:11]

Thanks for the thoughtful review.  I think I've got your comments addressed.

PTAL.

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

https://codereview.chromium.org/1245263002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/source.dart:819: Uri actualUri = null;
On 2015/07/21 19:39:27, Paul Berry wrote:
> On 2015/07/21 18:48:34, Brian Wilkerson wrote:
> > I'm not convinced that it's ever necessary to pass in `null` rather than the
> > containedUri, and the code would be easier to read and maintain with fewer
> > variables. If there's a case I'm not thinking of, a comment explaining it
> would
> > be helpful; otherwise a TODO to explore cleaning up the code would be good.
> 
> I agree--AFAIK, always passing containedUri to resolveAbsolute() should
produce
> correct results all the time.

Ok.  Fixed up!

https://codereview.chromium.org/1245263002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/source.dart:1075: * @param actualUri the actual
uri for this source
On 2015/07/21 19:39:27, Paul Berry wrote:
> We should document that the caller is allowed to pass null, in which case the
> source that's created will have a URI of [uri].

Done.

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

https://codereview.chromium.org/1245263002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/source_io.dart:283: return new
FileBasedSource(new JavaFile.fromUri(uri), actualUri);
On 2015/07/21 19:39:27, Paul Berry wrote:
> There's a subtle change in behavior in the case where actualUri is null that
I'm
> not sure is on purpose.  

Good catch!

> Previously, the new Source object would receive its URI
> directly from the "uri" parameter.  Now, when actualUri is null, the Source
> object derives its URI from JavaFile.toURI().  I suspect this is ok (since all
> JavaFile.toURI() does is absolutize and normalize the path, and I *think* that
> the URI's should have been absolutized and normalized before they get here). 

I convinced myself that this would be safe but I share your uneasiness.

> But I would feel more comfortable if either:
> 
> (a) we changed the code to match the old behavior when actualUri == null, or

I picked (a) since it anticipates the same check we need to make below.

> (b) we changed actualUri to a non-optional parameter and ensured that a
non-null
> value was always passed in (along the lines of Brian's suggestion in his
comment
> on pkg/analyzer/lib/src/generated/source.dart)

https://codereview.chromium.org/1245263002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/source_io.dart:466:
getCanonicalFile(_packagesDirectories[0], pkgName, relPath), actualUri);
On 2015/07/21 19:39:27, Paul Berry wrote:
> This has the same problem as above, but it's definitely wrong in this case
> because we're resolving a package: URI, so passing null (and letting the URI
be
> chosen by FileBasedSource from the JavaFile will get us the wrong kind of
URI). 
> We need logic so that if actualUri is null, we pass in uri.

Absolutely.  Spot on.  Thanks!

> 
> I'm kind of surprised this didn't lead to any test failures, so I'm wondering
if
> we have a gap in our test coverage.

https://codereview.chromium.org/1245263002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/source_io.dart:548: return new
FileBasedSource(file, actualUri);
On 2015/07/21 19:39:27, Paul Berry wrote:
> Similar problem here, although I can't assess the impact because I can't find
> any code that uses this class (RelativeFileResolver)--not even tests.

Done.

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

https://codereview.chromium.org/1245263002/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/source_factory_test.dart:183: createSource(path:
uriPath);
On 2015/07/21 19:39:27, Paul Berry wrote:
> Not sure if it matters because this is test code, but technically to satisfy
the
> interface contract for UriResolver, we should pass actualUri through to
> createSource().

I thought about that.  At the moment there's  no API on File that allows you to
pass in an actualUri and I wasn't ready to take that on.  I added a TODO in the
meantime.

[Brian Wilkerson, 2015-07-21 21:07:31]

We still need to update the version number, but other than that, LGTM

[pquitslund, 2015-07-21 21:10:07]

On 2015/07/21 21:07:31, Brian Wilkerson wrote:
> We still need to update the version number, but other than that, LGTM

I did.  That is, analyzer and server pubspecs...  

https://codereview.chromium.org/1245263002/diff/40001/pkg/analyzer/pubspec.yaml
https://codereview.chromium.org/1245263002/diff/40001/pkg/analysis_server/pub...

Was there something else?

[Brian Wilkerson, 2015-07-21 21:19:16]

> > We still need to update the version number, but other than that, LGTM
> 
> I did.

Sorry. Somehow I didn't see those when I looked at the CL the second time. Those
look fine too.

[Paul Berry, 2015-07-21 21:33:47]

lgtm assuming the issue below is fixed.

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

https://codereview.chromium.org/1245263002/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/source.dart:831: actualUri = containedUri;
Delete this line.  It overwrites actualUri with the value you just assigned to
containedUri, which defeats the purpose of the fix.

I'm surprised this didn't cause a test failure--is this not covered by tests, or
am I misunderstanding the code?

[pquitslund, 2015-07-21 22:41:27]

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

https://codereview.chromium.org/1245263002/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/source.dart:831: actualUri = containedUri;
On 2015/07/21 21:33:47, Paul Berry wrote:
> Delete this line.  It overwrites actualUri with the value you just assigned to
> containedUri, which defeats the purpose of the fix.
> 
> I'm surprised this didn't cause a test failure--is this not covered by tests,
or
> am I misunderstanding the code?

Sorry.  Forgot to push this fix!

Already done. :)