Implement URI resolution in the front end.

Implement URI resolution in the front end.

R=danrubel@google.com, sigmund@google.com

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

[Paul Berry, 2016-12-13 19:05:54]

paulberry@google.com changed reviewers:
+ danrubel@google.com, sigmund@google.com

[Brian Wilkerson, 2016-12-13 19:13:54]

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

[Brian Wilkerson, 2016-12-13 19:13:54]

DBC

https://codereview.chromium.org/2577503002/diff/1/pkg/front_end/lib/src/base/...
File pkg/front_end/lib/src/base/uri_resolver.dart (right):

https://codereview.chromium.org/2577503002/diff/1/pkg/front_end/lib/src/base/...
pkg/front_end/lib/src/base/uri_resolver.dart:9: class UriResolver {
I don't know whether this class needs to support the 'http' scheme, but I
believe that the VM does currently have support for it.

https://codereview.chromium.org/2577503002/diff/1/pkg/front_end/lib/src/base/...
pkg/front_end/lib/src/base/uri_resolver.dart:13: final Map<String, Uri>
packages;
I'm probably missing context here, but I don't believe this will work for the
Bazel case because we can't provide a simple map that allows us to locate
generated files. Is that case going to be supported?

[Paul Berry, 2016-12-13 19:22:34]

https://codereview.chromium.org/2577503002/diff/1/pkg/front_end/lib/src/base/...
File pkg/front_end/lib/src/base/uri_resolver.dart (right):

https://codereview.chromium.org/2577503002/diff/1/pkg/front_end/lib/src/base/...
pkg/front_end/lib/src/base/uri_resolver.dart:9: class UriResolver {
On 2016/12/13 19:13:54, Brian Wilkerson wrote:
> I don't know whether this class needs to support the 'http' scheme, but I
> believe that the VM does currently have support for it.

I don't know either.  I've added a TODO for now.  I will follow up with VM
folks.

https://codereview.chromium.org/2577503002/diff/1/pkg/front_end/lib/src/base/...
pkg/front_end/lib/src/base/uri_resolver.dart:13: final Map<String, Uri>
packages;
On 2016/12/13 19:13:54, Brian Wilkerson wrote:
> I'm probably missing context here, but I don't believe this will work for the
> Bazel case because we can't provide a simple map that allows us to locate
> generated files. Is that case going to be supported?

The plan for how to support the Bazel case is that the URI will be resolved to a
path of the form "file:///bazel-root/a/b/c.dart".  Then, a special
implementation of the FileSystem class will be used which understands that file
paths of this form should be looked up in a variety of Bazel-specific locations
until the first file which exists is found.

The advantages of this scheme are that is keeps Bazel-specific logic isolated
from the rest of the front end, and it avoids requiring file system access in
order to do URI resolution (so URI resolution remains fast even if files are
located on a network file system).  It's also consistent with what happens in
dart2js.

[danrubel, 2016-12-13 20:22:10]

LGTM

[Siggi Cherem (dart-lang), 2016-12-13 20:25:57]

lgtm

https://codereview.chromium.org/2577503002/diff/1/pkg/front_end/lib/src/base/...
File pkg/front_end/lib/src/base/uri_resolver.dart (right):

https://codereview.chromium.org/2577503002/diff/1/pkg/front_end/lib/src/base/...
pkg/front_end/lib/src/base/uri_resolver.dart:44: String resolve(Uri uri) {
General question about the design here: how much do we expect to use paths and
URIs in the rest of frontend? What if we use URIs everywhere except in
FileSystem? If so, this could become a URI => URI (output guaranteed to be a
file:// URI)

https://codereview.chromium.org/2577503002/diff/1/pkg/front_end/lib/src/base/...
pkg/front_end/lib/src/base/uri_resolver.dart:66: }
maybe add an else to throw for unsupported schemes?

https://codereview.chromium.org/2577503002/diff/1/pkg/front_end/lib/src/base/...
pkg/front_end/lib/src/base/uri_resolver.dart:69: if (fileUri.scheme !=
FILE_SCHEME) return null;
I'd also be OK validating that the map is initialized correctly instead.

[Paul Berry, 2016-12-13 21:15:17]

https://codereview.chromium.org/2577503002/diff/1/pkg/front_end/lib/src/base/...
File pkg/front_end/lib/src/base/uri_resolver.dart (right):

https://codereview.chromium.org/2577503002/diff/1/pkg/front_end/lib/src/base/...
pkg/front_end/lib/src/base/uri_resolver.dart:44: String resolve(Uri uri) {
On 2016/12/13 20:25:57, Siggi Cherem (dart-lang) wrote:
> General question about the design here: how much do we expect to use paths and
> URIs in the rest of frontend? What if we use URIs everywhere except in
> FileSystem? If so, this could become a URI => URI (output guaranteed to be a
> file:// URI)

Hmm, I like your idea.  Taking it a step further, maybe we should drop the
requirement that the output is guaranteed to be a file URI--that would allow us
to support things like "http" URIs by putting the logic for fetching from http
into the FileSystem abstraction.

I'm going to commit the code as is so that I can make forward progress, but I'll
revisit the question soon.

https://codereview.chromium.org/2577503002/diff/1/pkg/front_end/lib/src/base/...
pkg/front_end/lib/src/base/uri_resolver.dart:66: }
On 2016/12/13 20:25:57, Siggi Cherem (dart-lang) wrote:
> maybe add an else to throw for unsupported schemes?

Actually the behavior I'm aiming for is that unsupported schemes cause us to
return `null`--that allows the client to handle all mal-formed URIs using the
same logic.

As with the above comment, I'm going to commit as is for now, but I'll revisit
it soon.

https://codereview.chromium.org/2577503002/diff/1/pkg/front_end/lib/src/base/...
pkg/front_end/lib/src/base/uri_resolver.dart:69: if (fileUri.scheme !=
FILE_SCHEME) return null;
On 2016/12/13 20:25:57, Siggi Cherem (dart-lang) wrote:
> I'd also be OK validating that the map is initialized correctly instead.

Acknowledged.

[Paul Berry, 2016-12-13 21:21:18]

Description was changed from

==========
Implement URI resolution in the front end.
==========

to

==========
Implement URI resolution in the front end.

R=danrubel@google.com, sigmund@google.com

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

[Paul Berry, 2016-12-13 21:21:19]

Committed patchset #3 (id:40001) manually as
b29fa08444217a5d7acbd3cfe4e64be0029b0cfa (presubmit successful).