Fix for resolving 'bar.dart' against 'dart:foo'. Test for TranslateUri.

Fix for resolving 'bar.dart' against 'dart:foo'. Test for TranslateUri.

This allows us to compile more libraries, although we still cannot
compile pkg/analyzer. We use too much memory, and there might be another
bug - it got into infinite loop.

R=ahe@google.com, paulberry@google.com, sigmund@google.com
BUG=

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

[Siggi Cherem (dart-lang), 2017-05-17 18:17:43]

lgtm

[scheglov, 2017-05-17 18:34:00]

Description was changed from

==========
Fix for resolving 'bar.dart' against 'dart:foo'. Test for TranslateUri.

This allows us to compile more libraries, although we still cannot
compile pkg/analyzer. We use too much memory, and there might be another
bug - it got into infinite loop.

R=ahe@google.com, paulberry@google.com, sigmund@google.com
BUG=
==========

to

==========
Fix for resolving 'bar.dart' against 'dart:foo'. Test for TranslateUri.

This allows us to compile more libraries, although we still cannot
compile pkg/analyzer. We use too much memory, and there might be another
bug - it got into infinite loop.

R=ahe@google.com, paulberry@google.com, sigmund@google.com
BUG=

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

[scheglov, 2017-05-17 18:34:01]

Committed patchset #1 (id:1) manually as
abc656f3c3390dfef78b2b9074f6ae4921ec5768 (presubmit successful).

[ahe, 2017-05-19 11:01:04]

I don't understand what this CL is fixing. Could you please give an example of a
program that can't be compiled without this change?

[scheglov, 2017-05-19 15:23:51]

On 2017/05/19 11:01:04, ahe wrote:
> I don't understand what this CL is fixing. Could you please give an example of
a
> program that can't be compiled without this change?

The example is in the added unit test.
We should be able to resolve "string.dart" relative to "dart:code", get
"dart:core/string.dart" and then resolve it to the file URI.

[ahe, 2017-05-22 08:55:40]

On 2017/05/19 15:23:51, scheglov wrote:
> On 2017/05/19 11:01:04, ahe wrote:
> > I don't understand what this CL is fixing. Could you please give an example
of
> a
> > program that can't be compiled without this change?
> 
> The example is in the added unit test.
> We should be able to resolve "string.dart" relative to "dart:code", get
> "dart:core/string.dart" and then resolve it to the file URI.

I'm not asking for a unit test, I'm asking for an example input to fasta that
doesn't work as expected.

[Siggi Cherem (dart-lang), 2017-05-22 16:55:22]

On 2017/05/22 08:55:40, ahe wrote:
> On 2017/05/19 15:23:51, scheglov wrote:
> > On 2017/05/19 11:01:04, ahe wrote:
> > > I don't understand what this CL is fixing. Could you please give an
example
> of
> > a
> > > program that can't be compiled without this change?
> > 
> > The example is in the added unit test.
> > We should be able to resolve "string.dart" relative to "dart:code", get
> > "dart:core/string.dart" and then resolve it to the file URI.
> 
> I'm not asking for a unit test, I'm asking for an example input to fasta that
> doesn't work as expected.

The point here I think is that there are two possible ways to go about
translating a program readable uri to a file resource uri: convert first then
resolve relative paths, or do it in the opposite order.

For `package:*` uris, we do resolve-then-convert, for `dart:*` uris, fasta did
convert-then-resolve. I should have noticed this earlier because dart2js does
the same way that fasta does.

We must do resolve-then-convert for `package:*` Uris to guarantee we do the
correct canonicalization of libraries. But for `dart:*` turns out that we only
need to do resolve relative paths to load part files, but a Uri like
"dart:core/string.dart" is invalid in application code. I'm thinking that
fasta's approach might be a good way to ensure that such Uris are not
accidentally supported.

Where in the IKG did you need this? I'd like that if we revert to the previous
fasta's invariant here, then it doesn't mean that we duplicate how this is
handled in two places.

[scheglov, 2017-05-23 00:05:43]

On 2017/05/22 16:55:22, Siggi Cherem (dart-lang) wrote:
> On 2017/05/22 08:55:40, ahe wrote:
> > On 2017/05/19 15:23:51, scheglov wrote:
> > > On 2017/05/19 11:01:04, ahe wrote:
> > > > I don't understand what this CL is fixing. Could you please give an
> example
> > of
> > > a
> > > > program that can't be compiled without this change?
> > > 
> > > The example is in the added unit test.
> > > We should be able to resolve "string.dart" relative to "dart:code", get
> > > "dart:core/string.dart" and then resolve it to the file URI.
> > 
> > I'm not asking for a unit test, I'm asking for an example input to fasta
that
> > doesn't work as expected.
> 
> The point here I think is that there are two possible ways to go about
> translating a program readable uri to a file resource uri: convert first then
> resolve relative paths, or do it in the opposite order.
> 
> For `package:*` uris, we do resolve-then-convert, for `dart:*` uris, fasta did
> convert-then-resolve. I should have noticed this earlier because dart2js does
> the same way that fasta does.
> 
> We must do resolve-then-convert for `package:*` Uris to guarantee we do the
> correct canonicalization of libraries. But for `dart:*` turns out that we only
> need to do resolve relative paths to load part files, but a Uri like
> "dart:core/string.dart" is invalid in application code. I'm thinking that
> fasta's approach might be a good way to ensure that such Uris are not
> accidentally supported.
> 
> Where in the IKG did you need this? I'd like that if we revert to the previous
> fasta's invariant here, then it doesn't mean that we duplicate how this is
> handled in two places.

It seems more logical to me to handle all kinds of URIs consistently and use
actual URIs everywhere.
If there is a risk of importing non-library files, then it should be verified
explicitly and an error reported.
Analyzer does this.
URIs file:///my/dart/sdk/lib/core/core.dart and dart:core are different, so we
should report errors when the user tries to use the type X from one as the type
X in another.

[ahe, 2017-05-23 13:19:18]

On 2017/05/23 00:05:43, scheglov wrote:
> On 2017/05/22 16:55:22, Siggi Cherem (dart-lang) wrote:
> > On 2017/05/22 08:55:40, ahe wrote:
> > > On 2017/05/19 15:23:51, scheglov wrote:
> > > > On 2017/05/19 11:01:04, ahe wrote:
> > > > > I don't understand what this CL is fixing. Could you please give an
> > example
> > > of
> > > > a
> > > > > program that can't be compiled without this change?
> > > > 
> > > > The example is in the added unit test.
> > > > We should be able to resolve "string.dart" relative to "dart:code", get
> > > > "dart:core/string.dart" and then resolve it to the file URI.
> > > 
> > > I'm not asking for a unit test, I'm asking for an example input to fasta
> that
> > > doesn't work as expected.
> > 
> > The point here I think is that there are two possible ways to go about
> > translating a program readable uri to a file resource uri: convert first
then
> > resolve relative paths, or do it in the opposite order.

For clarity, I want to define two operations:

1. translate: converts a package: or dart: URI to a file: URI.

2. resolution: Uri.resolve.

These operations have to be applied in the correct order, and using the opposite
order can lead to incorrect results, both for dart: and package: URIs.

For example, let's say that I have the URI "dart:core". This particular URI can
be translated to, for example,
file:///.../ReleaseX64/patched_sdk/lib/core/core.dart because there exists a
mapping from "core" to that particular file in dartLibraries (or equivalently in
sdk/lib/_internal/sdk_library_metadata/lib/libraries.dart,
sdk/lib/dart_server.platform, or sdk/lib/dart_server.yaml).

Continuing this example, .../ReleaseX64/patched_sdk/lib/core/core.dart contains
a part declaration:

part "annotations.dart";

We can resolve this against dart:core and get "dart:annotations.dart". If you
get any other result, you're not using URI resolution (which would be wrong).

Does dartLibraries or any of the other files have an entry for
dart:annotations.dart? No. Is this a mistake? No.

So what is one supposed to do instead? Resolve against the translated file URI
which yields file:///.../ReleaseX64/patched_sdk/lib/core/annotations.dart. This
is the correct location for the part.

One can only handle dart: libraries by first translating the library URI and
then resolving against that (translate before resolution).

For package: URIs, the situation is different. Now you have to resolve before
translating. Otherwise you can escape the package root.

A LibraryBuilder has two properties related to this:

* uri (this is the untranslated aka canonical URI).

* fileUri (this is the translated aka file URI). 

The uri property is used as a key in the Loader to ensure uniqueness of
libraries with the same (untranslated) URI.

The fileUri property should be used for all messages.

> > For `package:*` uris, we do resolve-then-convert, for `dart:*` uris, fasta
did
> > convert-then-resolve. I should have noticed this earlier because dart2js
does
> > the same way that fasta does.
> > 
> > We must do resolve-then-convert for `package:*` Uris to guarantee we do the
> > correct canonicalization of libraries. But for `dart:*` turns out that we
only
> > need to do resolve relative paths to load part files, but a Uri like
> > "dart:core/string.dart" is invalid in application code. I'm thinking that
> > fasta's approach might be a good way to ensure that such Uris are not
> > accidentally supported.
> > 
> > Where in the IKG did you need this? I'd like that if we revert to the
previous
> > fasta's invariant here, then it doesn't mean that we duplicate how this is
> > handled in two places.
> 
> It seems more logical to me to handle all kinds of URIs consistently and use
> actual URIs everywhere.
> If there is a risk of importing non-library files, then it should be verified
> explicitly and an error reported.
> Analyzer does this.
> URIs file:///my/dart/sdk/lib/core/core.dart and dart:core are different, so we
> should report errors when the user tries to use the type X from one as the
type
> X in another.

[scheglov, 2017-05-23 16:28:12]

On 2017/05/23 13:19:18, ahe wrote:
> On 2017/05/23 00:05:43, scheglov wrote:
> > On 2017/05/22 16:55:22, Siggi Cherem (dart-lang) wrote:
> > > On 2017/05/22 08:55:40, ahe wrote:
> > > > On 2017/05/19 15:23:51, scheglov wrote:
> > > > > On 2017/05/19 11:01:04, ahe wrote:
> > > > > > I don't understand what this CL is fixing. Could you please give an
> > > example
> > > > of
> > > > > a
> > > > > > program that can't be compiled without this change?
> > > > > 
> > > > > The example is in the added unit test.
> > > > > We should be able to resolve "string.dart" relative to "dart:code",
get
> > > > > "dart:core/string.dart" and then resolve it to the file URI.
> > > > 
> > > > I'm not asking for a unit test, I'm asking for an example input to fasta
> > that
> > > > doesn't work as expected.
> > > 
> > > The point here I think is that there are two possible ways to go about
> > > translating a program readable uri to a file resource uri: convert first
> then
> > > resolve relative paths, or do it in the opposite order.
> 
> For clarity, I want to define two operations:
> 
> 1. translate: converts a package: or dart: URI to a file: URI.
> 
> 2. resolution: Uri.resolve.
> 
> These operations have to be applied in the correct order, and using the
opposite
> order can lead to incorrect results, both for dart: and package: URIs.
> 
> For example, let's say that I have the URI "dart:core". This particular URI
can
> be translated to, for example,
> file:///.../ReleaseX64/patched_sdk/lib/core/core.dart because there exists a
> mapping from "core" to that particular file in dartLibraries (or equivalently
in
> sdk/lib/_internal/sdk_library_metadata/lib/libraries.dart,
> sdk/lib/dart_server.platform, or sdk/lib/dart_server.yaml).
> 
> Continuing this example, .../ReleaseX64/patched_sdk/lib/core/core.dart
contains
> a part declaration:
> 
> part "annotations.dart";
> 
> We can resolve this against dart:core and get "dart:annotations.dart". If you
> get any other result, you're not using URI resolution (which would be wrong).
> 
> Does dartLibraries or any of the other files have an entry for
> dart:annotations.dart? No. Is this a mistake? No.
> 
> So what is one supposed to do instead? Resolve against the translated file URI
> which yields file:///.../ReleaseX64/patched_sdk/lib/core/annotations.dart.
This
> is the correct location for the part.
> 
> One can only handle dart: libraries by first translating the library URI and
> then resolving against that (translate before resolution).
> 
> For package: URIs, the situation is different. Now you have to resolve before
> translating. Otherwise you can escape the package root.
> 
> A LibraryBuilder has two properties related to this:
> 
> * uri (this is the untranslated aka canonical URI).
> 
> * fileUri (this is the translated aka file URI). 
> 
> The uri property is used as a key in the Loader to ensure uniqueness of
> libraries with the same (untranslated) URI.
> 
> The fileUri property should be used for all messages.
> 
> > > For `package:*` uris, we do resolve-then-convert, for `dart:*` uris, fasta
> did
> > > convert-then-resolve. I should have noticed this earlier because dart2js
> does
> > > the same way that fasta does.
> > > 
> > > We must do resolve-then-convert for `package:*` Uris to guarantee we do
the
> > > correct canonicalization of libraries. But for `dart:*` turns out that we
> only
> > > need to do resolve relative paths to load part files, but a Uri like
> > > "dart:core/string.dart" is invalid in application code. I'm thinking that
> > > fasta's approach might be a good way to ensure that such Uris are not
> > > accidentally supported.
> > > 
> > > Where in the IKG did you need this? I'd like that if we revert to the
> previous
> > > fasta's invariant here, then it doesn't mean that we duplicate how this is
> > > handled in two places.
> > 
> > It seems more logical to me to handle all kinds of URIs consistently and use
> > actual URIs everywhere.
> > If there is a risk of importing non-library files, then it should be
verified
> > explicitly and an error reported.
> > Analyzer does this.
> > URIs file:///my/dart/sdk/lib/core/core.dart and dart:core are different, so
we
> > should report errors when the user tries to use the type X from one as the
> type
> > X in another.

We use resolveRelativeUri() from
pkg/front_end/lib/src/base/resolve_relative_uri.dart which correctly resolves
"annotations.dart" against "dart:core" to "dart:core/annotations.dart". We're
basically treating "dart:core" as "dart:core/core.dart".

The canonical URI for dart:core is dart:core, not its file URI.
URIs file:///.../ReleaseX64/patched_sdk/lib/core/core.dart and dart:core are
different and cannot be used interexchangeably.

[ahe, 2017-05-23 20:02:45]

On 2017/05/23 16:28:12, scheglov wrote:
> On 2017/05/23 13:19:18, ahe wrote:
> > On 2017/05/23 00:05:43, scheglov wrote:
> > > On 2017/05/22 16:55:22, Siggi Cherem (dart-lang) wrote:
> > > > On 2017/05/22 08:55:40, ahe wrote:
> > > > > On 2017/05/19 15:23:51, scheglov wrote:
> > > > > > On 2017/05/19 11:01:04, ahe wrote:
> > > > > > > I don't understand what this CL is fixing. Could you please give
an
> > > > example
> > > > > of
> > > > > > a
> > > > > > > program that can't be compiled without this change?
> > > > > > 
> > > > > > The example is in the added unit test.
> > > > > > We should be able to resolve "string.dart" relative to "dart:code",
> get
> > > > > > "dart:core/string.dart" and then resolve it to the file URI.
> > > > > 
> > > > > I'm not asking for a unit test, I'm asking for an example input to
fasta
> > > that
> > > > > doesn't work as expected.
> > > > 
> > > > The point here I think is that there are two possible ways to go about
> > > > translating a program readable uri to a file resource uri: convert first
> > then
> > > > resolve relative paths, or do it in the opposite order.
> > 
> > For clarity, I want to define two operations:
> > 
> > 1. translate: converts a package: or dart: URI to a file: URI.
> > 
> > 2. resolution: Uri.resolve.
> > 
> > These operations have to be applied in the correct order, and using the
> opposite
> > order can lead to incorrect results, both for dart: and package: URIs.
> > 
> > For example, let's say that I have the URI "dart:core". This particular URI
> can
> > be translated to, for example,
> > file:///.../ReleaseX64/patched_sdk/lib/core/core.dart because there exists a
> > mapping from "core" to that particular file in dartLibraries (or
equivalently
> in
> > sdk/lib/_internal/sdk_library_metadata/lib/libraries.dart,
> > sdk/lib/dart_server.platform, or sdk/lib/dart_server.yaml).
> > 
> > Continuing this example, .../ReleaseX64/patched_sdk/lib/core/core.dart
> contains
> > a part declaration:
> > 
> > part "annotations.dart";
> > 
> > We can resolve this against dart:core and get "dart:annotations.dart". If
you
> > get any other result, you're not using URI resolution (which would be
wrong).
> > 
> > Does dartLibraries or any of the other files have an entry for
> > dart:annotations.dart? No. Is this a mistake? No.
> > 
> > So what is one supposed to do instead? Resolve against the translated file
URI
> > which yields file:///.../ReleaseX64/patched_sdk/lib/core/annotations.dart.
> This
> > is the correct location for the part.
> > 
> > One can only handle dart: libraries by first translating the library URI and
> > then resolving against that (translate before resolution).
> > 
> > For package: URIs, the situation is different. Now you have to resolve
before
> > translating. Otherwise you can escape the package root.
> > 
> > A LibraryBuilder has two properties related to this:
> > 
> > * uri (this is the untranslated aka canonical URI).
> > 
> > * fileUri (this is the translated aka file URI). 
> > 
> > The uri property is used as a key in the Loader to ensure uniqueness of
> > libraries with the same (untranslated) URI.
> > 
> > The fileUri property should be used for all messages.
> > 
> > > > For `package:*` uris, we do resolve-then-convert, for `dart:*` uris,
fasta
> > did
> > > > convert-then-resolve. I should have noticed this earlier because dart2js
> > does
> > > > the same way that fasta does.
> > > > 
> > > > We must do resolve-then-convert for `package:*` Uris to guarantee we do
> the
> > > > correct canonicalization of libraries. But for `dart:*` turns out that
we
> > only
> > > > need to do resolve relative paths to load part files, but a Uri like
> > > > "dart:core/string.dart" is invalid in application code. I'm thinking
that
> > > > fasta's approach might be a good way to ensure that such Uris are not
> > > > accidentally supported.
> > > > 
> > > > Where in the IKG did you need this? I'd like that if we revert to the
> > previous
> > > > fasta's invariant here, then it doesn't mean that we duplicate how this
is
> > > > handled in two places.
> > > 
> > > It seems more logical to me to handle all kinds of URIs consistently and
use
> > > actual URIs everywhere.
> > > If there is a risk of importing non-library files, then it should be
> verified
> > > explicitly and an error reported.
> > > Analyzer does this.
> > > URIs file:///my/dart/sdk/lib/core/core.dart and dart:core are different,
so
> we
> > > should report errors when the user tries to use the type X from one as the
> > type
> > > X in another.
> 
> We use resolveRelativeUri() from
> pkg/front_end/lib/src/base/resolve_relative_uri.dart which correctly resolves
> "annotations.dart" against "dart:core" to "dart:core/annotations.dart". We're
> basically treating "dart:core" as "dart:core/core.dart".

That's not correct. The resolution of URIs is specified by RFC 3986, right here:
https://tools.ietf.org/html/rfc3986#section-5

Uri.resolve implements this specification.

> The canonical URI for dart:core is dart:core, not its file URI.

Yes. That's why the LibraryBuilder corresponding to dart:core has two different
properties (as I mentioned above):

1. A property named "uri". This is its canonical URI which is "dart:core".

2. A property named "fileUri". This is the URI of the file to read on disk
(file:///.../ReleaseX64/patched_sdk/lib/core/core.dart).

> URIs file:///.../ReleaseX64/patched_sdk/lib/core/core.dart and dart:core are
> different and cannot be used interexchangeably.

Yes.

The canonical URI for a LibraryBuilder is its uri property. The name of the file
that is read is the fileUri property. I explicitly mentioned this above. I also
mentioned explicitly that the uniqueness of a library is determined by its uri
property (which I also called the canonical URI). Hence, if you have one
LibraryBuilder whose uri property is "dart:core", it is different from a
LibraryBuilder whose uri property is
file:///.../ReleaseX64/patched_sdk/lib/core/core.dart. However, both these
libraries will have the same fileUri property (because they read the same file).