Change MirrorSystem.libraries to Map<Uri, String>

Change MirrorSystem.libraries to Map<Uri, String>


BUG=

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

[Johnni Winther, 2013-04-08 13:22:33]

https://codereview.chromium.org/13797002/diff/1/utils/apidoc/apidoc.dart
File utils/apidoc/apidoc.dart (left):

https://codereview.chromium.org/13797002/diff/1/utils/apidoc/apidoc.dart#oldc...
utils/apidoc/apidoc.dart:442: if (member is MethodMirror &&
(member.isConstructor || member.isFactory)) {
isFactory should have been isFactoryConstructor which is implied by
isConstructor.

[gbracha, 2013-04-08 20:28:51]

I think we need to be very clear what the API change means.  Since libraries are
not required to have names, and we do not even encourage a unique naming scheme,
it is true that the old API does not necessarily make sense.

On the other hand, the change doesn't either. The canonical URI of a library may
not match the URI used to import it, which may in fact not be unique across the
program. The user may not even know what the canonical URI is. 

We could try and come up with some more intelligent behavior. Could we
canonicalize the incoming URI and use it for lookup, so any valid URI the user
submits will work? 
The implementation doesn't do this at the moment. What the reyurne dmao actually
is is not well specified.

[gbracha, 2013-04-08 20:29:10]

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

https://codereview.chromium.org/13797002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/mirrors/mirrors.dart:20: Map<Uri,
LibraryMirror> get libraries;
When was this API change discussed? As far as I can tell, URIs are not
canonicalized. And so, the question is how does a URI is reliably identify a
library?  In other words, how do I find a given library in this map?

[ahe, 2013-04-09 08:23:03]

On 2013/04/08 20:28:51, gbracha wrote:
> On the other hand, the change doesn't either. The canonical URI of a library
may
> not match the URI used to import it, which may in fact not be unique across
the
> program. The user may not even know what the canonical URI is. 

There is no such thing as a canonical URI.  The URI used to import a library is
the URI it will have during one instance of a mirror system.  As such, the URI
used to import a library is unique in one instance of a mirror system (or in one
isolate).

> We could try and come up with some more intelligent behavior. Could we
> canonicalize the incoming URI and use it for lookup, so any valid URI the user
> submits will work? 

There is no way to canonicalize URIs in general, and there is no way to
canonicalize file names.

> The implementation doesn't do this at the moment. What the reyurne dmao
actually
> is is not well specified.

I'm not sure what that means.

I'd suggest adding a method like this to MirrorSystem:

List<LibraryMirror> getMirrorsNamed(String name); // Or Symbol, whatever.

Library names are not unique for a given isolate (or instance of a mirror
system), but URIs are.  However, the user may be interested in a library with a
given name, not its URI, so it makes sense to have both the "libraries" getter
and the method getMirrorsNamed.

[ahe, 2013-04-09 08:28:44]

LGTM.

In the future, I'd prefer to keep changes on topic.

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

https://codereview.chromium.org/13797002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/dart2js.dart:73: Uri libraryRoot =
currentDirectory;
This change is unrelated to the mirror system change.

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

https://codereview.chromium.org/13797002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/source_file_provider.dart:30: Uri cwd
= getCurrentDirectory();
I think it makes sense to keep the local variable.

[Andrei Mouravski, 2013-04-09 15:30:03]

lgtm

[Johnni Winther, 2013-04-22 13:00:02]

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

https://codereview.chromium.org/13797002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/dart2js.dart:73: Uri libraryRoot =
currentDirectory;
On 2013/04/09 08:28:44, ahe wrote:
> This change is unrelated to the mirror system change.

Sorry for the pollution.

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

https://codereview.chromium.org/13797002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/mirrors/mirrors.dart:20: Map<Uri,
LibraryMirror> get libraries;
On 2013/04/08 20:29:10, gbracha wrote:
> When was this API change discussed? As far as I can tell, URIs are not
> canonicalized. And so, the question is how does a URI is reliably identify a
> library?  In other words, how do I find a given library in this map?

It wasn't prior. I made this CL to get your input.

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

https://codereview.chromium.org/13797002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/source_file_provider.dart:30: Uri cwd
= getCurrentDirectory();
On 2013/04/09 08:28:44, ahe wrote:
> I think it makes sense to keep the local variable.

Done.

[Johnni Winther, 2013-04-22 13:02:08]

Committed patchset #3 manually as r21812 (presubmit successful).