Internal libraries supported.

Internal libraries supported.

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

[ngeoffray, 2013-01-16 15:40:57]

LGTM, but I would wait for Peter's comments.

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/apiimpl.dart (right):

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:149: if
(LIBRARIES[uri.path] != null &&
Store LIBRARIES[uri.path] in a local variable?

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:161: log('Internal
library $uri is not accessible '
Why do you log that? Isn't that something that the user will face?

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart
(right):

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:145: if
(library.isPlatformLibrary && !library.isPrivateLibrary) {
Should it be called InternalLibrary? Private already means something in dart.

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/library_loader.dart (right):

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/library_loader.dart:141:
createLibrary(currentHandler, null, uri, node, canonicalUri);
Why is that null here?

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/library_loader.dart:249: =
createLibrary(handler, null, coreUri, null, coreUri);
ditto

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/patch_parser.dart (right):

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/patch_parser.dart:155:
compiler.libraryLoader.registerLibraryFromTag(handler, patchLibrary, tag);
line too long

https://codereview.chromium.org/11967010/diff/1/tests/language/private_librar...
File tests/language/private_library_negative_test.dart (right):

https://codereview.chromium.org/11967010/diff/1/tests/language/private_librar...
tests/language/private_library_negative_test.dart:8: import
'dart:_foreign_helper';
Make this a multi test with a compile-time error?

[ahe, 2013-01-18 11:03:10]

LGTM, if you get rid of the check for "_" in the beginning of uri.path (or add a
TODO).

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/apiimpl.dart (left):

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:107: if (uri.scheme ==
'dart') uri = translateDartUri(uri, node);
I really like that you got rid of this.

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/apiimpl.dart (right):

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:107: * used to grant
access to private libraries for platform libraries.
Please document that this method is responsible for reporting errors, and that
it will return null if an error occurred.

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:109: Uri
resolveScriptUri(Uri importingUri, Uri uri, [tree.Node node]) {
I like this method.

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:120: if
(!uri.isAbsolute()) throw new ArgumentError(uri);
You should be able to report an (internal) error instead.

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:153: if (sourceUri.scheme
== 'dart' || sourceUri.scheme == 'patch') {
What is the patch scheme?

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:161: log('Internal
library $uri is not accessible '
On 2013/01/16 15:40:57, ngeoffray wrote:
> Why do you log that? Isn't that something that the user will face?

The user will see an error message like this:

fisk.dart:1:8: Error: library not found dart:_private
import 'dart:_private';
       ^^^^^^^^^^^

But the log is helpful, I think.

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/compiler.dart:971: Uri
resolveScriptUri(Uri importingUri, Uri uri, [Node node]) {
I think this should be documented here, not in the subclass.

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart
(right):

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:145: if
(library.isPlatformLibrary && !library.isPrivateLibrary) {
On 2013/01/16 15:40:57, ngeoffray wrote:
> Should it be called InternalLibrary? Private already means something in dart.

Excellent point.

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:479: * library
for [:dart:x:] the canonical uri is [:patch:_x:].
Why add an underscore?

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:489: * [:true:]
if this library is part of the platform, i.e. its canonical
i.e. -> that is (not all of us have studied ancient languages ;-)

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:495: * [:true:]
if this library is a platform library whose path starts with
I'm concerned about this approach. It seems brittle to base this on the first
character of the path.

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/elements/modelx.dart (right):

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:741: bool get
isPrivateLibrary => isPlatformLibrary && uri.path.startsWith('_');
This scares me. The uri.path is potentially from user code, so it cannot be
trusted.

https://codereview.chromium.org/11967010/diff/1/tests/compiler/dart2js_native...
File tests/compiler/dart2js_native/private_library_test.dart (right):

https://codereview.chromium.org/11967010/diff/1/tests/compiler/dart2js_native...
tests/compiler/dart2js_native/private_library_test.dart:5: // Test that a
private library can be accessed from libraries in this special
git cl upload --similarity=0

https://codereview.chromium.org/11967010/diff/1/tests/compiler/dart2js_native...
tests/compiler/dart2js_native/private_library_test.dart:11: print(lazyPort);
How about: Expect.isNotNull(lazyPort)?

https://codereview.chromium.org/11967010/diff/1/tests/language/private_librar...
File tests/language/private_library_negative_test.dart (right):

https://codereview.chromium.org/11967010/diff/1/tests/language/private_librar...
tests/language/private_library_negative_test.dart:8: import
'dart:_foreign_helper';
On 2013/01/16 15:40:57, ngeoffray wrote:
> Make this a multi test with a compile-time error?

+1

[Johnni Winther, 2013-01-21 09:27:54]

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/apiimpl.dart (right):

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:107: * used to grant
access to private libraries for platform libraries.
On 2013/01/18 11:03:10, ahe wrote:
> Please document that this method is responsible for reporting errors, and that
> it will return null if an error occurred.

Done.

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:120: if
(!uri.isAbsolute()) throw new ArgumentError(uri);
On 2013/01/18 11:03:10, ahe wrote:
> You should be able to report an (internal) error instead.

Done.

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:149: if
(LIBRARIES[uri.path] != null &&
On 2013/01/16 15:40:57, ngeoffray wrote:
> Store LIBRARIES[uri.path] in a local variable?

Done.

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:153: if (sourceUri.scheme
== 'dart' || sourceUri.scheme == 'patch') {
On 2013/01/18 11:03:10, ahe wrote:
> What is the patch scheme?

A new scheme used to provide patch libraries with a canonical uri.

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:161: log('Internal
library $uri is not accessible '
On 2013/01/18 11:03:10, ahe wrote:
> On 2013/01/16 15:40:57, ngeoffray wrote:
> > Why do you log that? Isn't that something that the user will face?
> 
> The user will see an error message like this:
> 
> fisk.dart:1:8: Error: library not found dart:_private
> import 'dart:_private';
>        ^^^^^^^^^^^
> 
> But the log is helpful, I think.

Changed to [reportError].

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/compiler.dart:971: Uri
resolveScriptUri(Uri importingUri, Uri uri, [Node node]) {
On 2013/01/18 11:03:10, ahe wrote:
> I think this should be documented here, not in the subclass.

Done.

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart
(right):

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:145: if
(library.isPlatformLibrary && !library.isPrivateLibrary) {
On 2013/01/16 15:40:57, ngeoffray wrote:
> Should it be called InternalLibrary? Private already means something in dart.

Good idea. Renamed the concept to 'internal library'.

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:479: * library
for [:dart:x:] the canonical uri is [:patch:_x:].
On 2013/01/18 11:03:10, ahe wrote:
> Why add an underscore?

No need. The scheme is enough protection that we cannot load a patch library
from the outside.

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:489: * [:true:]
if this library is part of the platform, i.e. its canonical
On 2013/01/18 11:03:10, ahe wrote:
> i.e. -> that is (not all of us have studied ancient languages ;-)

Well, I didn't write 'id est' ;-)

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:495: * [:true:]
if this library is a platform library whose path starts with
On 2013/01/18 11:03:10, ahe wrote:
> I'm concerned about this approach. It seems brittle to base this on the first
> character of the path.

The security is based on the [isPlatformLibrary] call as well. This checks the
canonical uri to have the 'dart' scheme, and this we set ourselves.

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/elements/modelx.dart (right):

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:741: bool get
isPrivateLibrary => isPlatformLibrary && uri.path.startsWith('_');
On 2013/01/18 11:03:10, ahe wrote:
> This scares me. The uri.path is potentially from user code, so it cannot be
> trusted.

[uri] is the canonical uri which we filter in [translateDartUri] in apiimpl.

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/library_loader.dart (right):

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/library_loader.dart:141:
createLibrary(currentHandler, null, uri, node, canonicalUri);
On 2013/01/16 15:40:57, ngeoffray wrote:
> Why is that null here?

The importing uri. Since this is a load from the outside, there is no importing
uri, thus no access to internal libraries.

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/library_loader.dart:249: =
createLibrary(handler, null, coreUri, null, coreUri);
On 2013/01/16 15:40:57, ngeoffray wrote:
> ditto

The import is synthetic and we need no special access to load dart:core so the
import uri is null.

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/patch_parser.dart (right):

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/patch_parser.dart:155:
compiler.libraryLoader.registerLibraryFromTag(handler, patchLibrary, tag);
On 2013/01/16 15:40:57, ngeoffray wrote:
> line too long

Done.

https://codereview.chromium.org/11967010/diff/1/tests/compiler/dart2js_native...
File tests/compiler/dart2js_native/private_library_test.dart (right):

https://codereview.chromium.org/11967010/diff/1/tests/compiler/dart2js_native...
tests/compiler/dart2js_native/private_library_test.dart:5: // Test that a
private library can be accessed from libraries in this special
On 2013/01/18 11:03:10, ahe wrote:
> git cl upload --similarity=0

Ahh, that's how!

https://codereview.chromium.org/11967010/diff/1/tests/compiler/dart2js_native...
tests/compiler/dart2js_native/private_library_test.dart:11: print(lazyPort);
On 2013/01/18 11:03:10, ahe wrote:
> How about: Expect.isNotNull(lazyPort)?

Is is actually null, since we access the field in the internal library directly.
In any case we do not rely on the value of the field but only the accessibility
of it.

https://codereview.chromium.org/11967010/diff/1/tests/language/private_librar...
File tests/language/private_library_negative_test.dart (right):

https://codereview.chromium.org/11967010/diff/1/tests/language/private_librar...
tests/language/private_library_negative_test.dart:8: import
'dart:_foreign_helper';
On 2013/01/16 15:40:57, ngeoffray wrote:
> Make this a multi test with a compile-time error?

Done.

[ahe, 2013-01-21 09:39:38]

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/apiimpl.dart (right):

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:153: if (sourceUri.scheme
== 'dart' || sourceUri.scheme == 'patch') {
On 2013/01/21 09:27:54, Johnni Winther wrote:
> On 2013/01/18 11:03:10, ahe wrote:
> > What is the patch scheme?
> 
> A new scheme used to provide patch libraries with a canonical uri.

I don't think it is a good idea to invent new schemes. We have registered the
"dart" scheme, and I'll like to stick to standard schemes.

[Johnni Winther, 2013-01-21 09:45:55]

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/apiimpl.dart (right):

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:153: if (sourceUri.scheme
== 'dart' || sourceUri.scheme == 'patch') {
On 2013/01/21 09:39:38, ahe wrote:
> On 2013/01/21 09:27:54, Johnni Winther wrote:
> > On 2013/01/18 11:03:10, ahe wrote:
> > > What is the patch scheme?
> > 
> > A new scheme used to provide patch libraries with a canonical uri.
> 
> I don't think it is a good idea to invent new schemes. We have registered the
> "dart" scheme, and I'll like to stick to standard schemes.

Then I'll have to encode it in the path, like dart:core_patch for the
core_patch. Is that better or do you have another idea?

[ahe, 2013-01-21 10:08:24]

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/apiimpl.dart (right):

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:153: if (sourceUri.scheme
== 'dart' || sourceUri.scheme == 'patch') {
On 2013/01/21 09:45:55, Johnni Winther wrote:
> On 2013/01/21 09:39:38, ahe wrote:
> > On 2013/01/21 09:27:54, Johnni Winther wrote:
> > > On 2013/01/18 11:03:10, ahe wrote:
> > > > What is the patch scheme?
> > > 
> > > A new scheme used to provide patch libraries with a canonical uri.
> > 
> > I don't think it is a good idea to invent new schemes. We have registered
the
> > "dart" scheme, and I'll like to stick to standard schemes.
> 
> Then I'll have to encode it in the path, like dart:core_patch for the
> core_patch. Is that better or do you have another idea?

I was hoping we could get rid of encoding this information in the URI. Is it
possible to set a bit on the library element?

But using the dart scheme to enable private library access seems much safer, as
it relies solely on a scheme that we control. This means that there is no
conceivable way for a user to ever be in a situation where he can present us
with a URI whose legitimate scheme is something we think means something else
(for example, "patch"), or whose path component fools us into thinking it is an
implementation detail.

Although the dart2js command line can only handle file URIs, it should be
possible to use the compiler API with arbitrary URIs and have them passed
through the compiler without the compiler getting the wrong idea.

[Johnni Winther, 2013-01-21 10:15:15]

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/apiimpl.dart (right):

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:153: if (sourceUri.scheme
== 'dart' || sourceUri.scheme == 'patch') {
On 2013/01/21 10:08:25, ahe wrote:
> On 2013/01/21 09:45:55, Johnni Winther wrote:
> > On 2013/01/21 09:39:38, ahe wrote:
> > > On 2013/01/21 09:27:54, Johnni Winther wrote:
> > > > On 2013/01/18 11:03:10, ahe wrote:
> > > > > What is the patch scheme?
> > > > 
> > > > A new scheme used to provide patch libraries with a canonical uri.
> > > 
> > > I don't think it is a good idea to invent new schemes. We have registered
> the
> > > "dart" scheme, and I'll like to stick to standard schemes.
> > 
> > Then I'll have to encode it in the path, like dart:core_patch for the
> > core_patch. Is that better or do you have another idea?
> 
> I was hoping we could get rid of encoding this information in the URI. Is it
> possible to set a bit on the library element?
> 
> But using the dart scheme to enable private library access seems much safer,
as
> it relies solely on a scheme that we control. This means that there is no
> conceivable way for a user to ever be in a situation where he can present us
> with a URI whose legitimate scheme is something we think means something else
> (for example, "patch"), or whose path component fools us into thinking it is
an
> implementation detail.
> 
> Although the dart2js command line can only handle file URIs, it should be
> possible to use the compiler API with arbitrary URIs and have them passed
> through the compiler without the compiler getting the wrong idea.

Then I think I'd rather go back to having no canonical uri on patch libraries
and use the importing LibraryElement instead of its canonical uri to grant
access to internal libraries. The LibraryElement must then be either
[isPlatformLibrary] or [isPatch].

[ahe, 2013-01-21 10:18:09]

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/apiimpl.dart (right):

https://codereview.chromium.org/11967010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:153: if (sourceUri.scheme
== 'dart' || sourceUri.scheme == 'patch') {
On 2013/01/21 10:15:15, Johnni Winther wrote:
> On 2013/01/21 10:08:25, ahe wrote:
> > On 2013/01/21 09:45:55, Johnni Winther wrote:
> > > On 2013/01/21 09:39:38, ahe wrote:
> > > > On 2013/01/21 09:27:54, Johnni Winther wrote:
> > > > > On 2013/01/18 11:03:10, ahe wrote:
> > > > > > What is the patch scheme?
> > > > > 
> > > > > A new scheme used to provide patch libraries with a canonical uri.
> > > > 
> > > > I don't think it is a good idea to invent new schemes. We have
registered
> > the
> > > > "dart" scheme, and I'll like to stick to standard schemes.
> > > 
> > > Then I'll have to encode it in the path, like dart:core_patch for the
> > > core_patch. Is that better or do you have another idea?
> > 
> > I was hoping we could get rid of encoding this information in the URI. Is it
> > possible to set a bit on the library element?
> > 
> > But using the dart scheme to enable private library access seems much safer,
> as
> > it relies solely on a scheme that we control. This means that there is no
> > conceivable way for a user to ever be in a situation where he can present us
> > with a URI whose legitimate scheme is something we think means something
else
> > (for example, "patch"), or whose path component fools us into thinking it is
> an
> > implementation detail.
> > 
> > Although the dart2js command line can only handle file URIs, it should be
> > possible to use the compiler API with arbitrary URIs and have them passed
> > through the compiler without the compiler getting the wrong idea.
> 
> Then I think I'd rather go back to having no canonical uri on patch libraries
> and use the importing LibraryElement instead of its canonical uri to grant
> access to internal libraries. The LibraryElement must then be either
> [isPlatformLibrary] or [isPatch].

SGTM

[Johnni Winther, 2013-01-22 13:42:22]

PTAL

Found a bug regarding package URIs and ended up cleaning up the use and
terminology on URIs. See the comment on the [LibraryLoader] class.

[ahe, 2013-01-23 12:08:23]

Sorry, I just have a few comments. I'm not sure I have more mileage in my brain
today.

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/apiimpl.dart (right):

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:69: //
TODO(johnniwinther): Merge better with [translateDartUri] when
I don't understand this comment.

https://codereview.chromium.org/11967010/diff/24001/tests/language/internal_l...
File tests/language/internal_library_test.dart (right):

https://codereview.chromium.org/11967010/diff/24001/tests/language/internal_l...
tests/language/internal_library_test.dart:11: import 'patch:core'; /// 02:
compile-time error
Did you remove this scheme? If so, could you test that you cannot import
'dart:core_patch' or whatever it is called.

[ahe, 2013-01-24 10:00:57]

LGTM!

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/apiimpl.dart (right):

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:103: Uri
resolveAbsoluteUri(elements.LibraryElement importingLibrary,
Actually, this method name doesn't really make sense. In terms of URI RFC
(standard), resolving an absolute URI is a no-op. I'd rather call it
"translate".

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:103: Uri
resolveAbsoluteUri(elements.LibraryElement importingLibrary,
I think it would be nice with a documentation comment like:

/// See [leg.Compiler.resolveAbsoluteUri].

Or something like that to inform the reader to look there for documentation.
When reading the library loader, it is nice to be able to go to leg.Compiler and
find documentation, but when reading this class, it's not so nice.

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:135: return new
leg.Script(readableUri, sourceFile);
Do you know why we use the "readableUri" here? I still get two different
libraries if I use resourceUri when I import a package through different paths:

import 'package:foo/foo.dart' as a;
import 'packages/foo/foo.dart' as b;

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:168: reportError(node,
Could this be reportDiagnostic?

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:174: path = null;
How about not setting path to null? The compiler will then resolve everything,
but stop before codegen.

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:956: * not accessible
from [importingLibrary] and returns [:null:] in this case.
Depending on how you respond to my comment in apiimpl line 174, this might need
to be tweaked.

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/dart2js.dart (right):

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/dart2js.dart:203: throw new
ArgumentError("Invalid uri '$uri'");
How about: "Unknown scheme in uri '$uri'"

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:477: * library
for [:dart:x:] the canonical uri is [:patch:x:].
I thought you'd get rid of the "patch" scheme?

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/library_loader.dart (right):

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/library_loader.dart:30: * ## Absolute
URI ##
I would call this "resolved".

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/library_loader.dart:67: * ## Readable
URI ##
I don't understand why we need both "readable URIs" and "resource URIs". Would
it be possible to translate dart: and package: URIs at the same time?

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/library_loader.dart:76: * name exacts
and in case of internal libraries whether access is granted.
exacts -> exists

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/library_loader.dart:86: *
'file:///current/working/dir/' then the package URI 'package:../foo/bar.dart'
This is a good example of something that shouldn't be allowed. A package URI
should not start with ".." and generally should not be able to "escape" the
package root.

[Johnni Winther, 2013-01-24 13:04:59]

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/apiimpl.dart (right):

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:69: //
TODO(johnniwinther): Merge better with [translateDartUri] when
On 2013/01/23 12:08:23, ahe wrote:
> I don't understand this comment.

The lookup into LIBRARIES is done both here and in [translateDartUri] which also
calls this method. I want to avoid that.

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:103: Uri
resolveAbsoluteUri(elements.LibraryElement importingLibrary,
On 2013/01/24 10:00:57, ahe wrote:
> I think it would be nice with a documentation comment like:
> 
> /// See [leg.Compiler.resolveAbsoluteUri].
> 
> Or something like that to inform the reader to look there for documentation.
> When reading the library loader, it is nice to be able to go to leg.Compiler
and
> find documentation, but when reading this class, it's not so nice.

Done.

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:103: Uri
resolveAbsoluteUri(elements.LibraryElement importingLibrary,
On 2013/01/24 10:00:57, ahe wrote:
> Actually, this method name doesn't really make sense. In terms of URI RFC
> (standard), resolving an absolute URI is a no-op. I'd rather call it
> "translate".

Good idea. Couldn't come up with a good name.

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:135: return new
leg.Script(readableUri, sourceFile);
On 2013/01/24 10:00:57, ahe wrote:
> Do you know why we use the "readableUri" here? I still get two different
> libraries if I use resourceUri when I import a package through different
paths:
> 
> import 'package:foo/foo.dart' as a;
> import 'packages/foo/foo.dart' as b;

We need to preserve the scheme in the script since is used for resolving
relative URIs mentioned in the script. Your example should result in two
different libraries (since the package root might not be the packages folder)
but importing 'foo.dart' in library 'a' must be the resolve to itself. That is,
the library loaded through the package scheme.

It was this subtlety that provoked the whole investigation into URIs.

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:168: reportError(node,
On 2013/01/24 10:00:57, ahe wrote:
> Could this be reportDiagnostic?

Done.

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:174: path = null;
On 2013/01/24 10:00:57, ahe wrote:
> How about not setting path to null? The compiler will then resolve everything,
> but stop before codegen.

Done.

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:956: * not accessible
from [importingLibrary] and returns [:null:] in this case.
On 2013/01/24 10:00:57, ahe wrote:
> Depending on how you respond to my comment in apiimpl line 174, this might
need
> to be tweaked.

Done.

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/dart2js.dart (right):

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/dart2js.dart:203: throw new
ArgumentError("Invalid uri '$uri'");
On 2013/01/24 10:00:57, ahe wrote:
> How about: "Unknown scheme in uri '$uri'"

Done.

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:477: * library
for [:dart:x:] the canonical uri is [:patch:x:].
On 2013/01/24 10:00:57, ahe wrote:
> I thought you'd get rid of the "patch" scheme?

It is.

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/library_loader.dart (right):

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/library_loader.dart:30: * ## Absolute
URI ##
On 2013/01/24 10:00:57, ahe wrote:
> I would call this "resolved".

Done.

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/library_loader.dart:67: * ## Readable
URI ##
On 2013/01/24 10:00:57, ahe wrote:
> I don't understand why we need both "readable URIs" and "resource URIs". Would
> it be possible to translate dart: and package: URIs at the same time?

Added a discussion of this.

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/library_loader.dart:76: * name exacts
and in case of internal libraries whether access is granted.
On 2013/01/24 10:00:57, ahe wrote:
> exacts -> exists

Done.

https://codereview.chromium.org/11967010/diff/24001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/library_loader.dart:86: *
'file:///current/working/dir/' then the package URI 'package:../foo/bar.dart'
On 2013/01/24 10:00:57, ahe wrote:
> This is a good example of something that shouldn't be allowed. A package URI
> should not start with ".." and generally should not be able to "escape" the
> package root.

Changed the example since that was not crucial to the point.

https://codereview.chromium.org/11967010/diff/24001/tests/language/internal_l...
File tests/language/internal_library_test.dart (right):

https://codereview.chromium.org/11967010/diff/24001/tests/language/internal_l...
tests/language/internal_library_test.dart:11: import 'patch:core'; /// 02:
compile-time error
On 2013/01/23 12:08:23, ahe wrote:
> Did you remove this scheme? If so, could you test that you cannot import
> 'dart:core_patch' or whatever it is called.

Patch libraries have no canonical URI - as before.