Add support for .sdkext file to analysis server

Add support for .sdkext file to analysis server

- Add SdkExtUriResolver which is populated with any dependency provided sdk extensions.
- Adjust AnalysisServer._createSourceFactory to setup a SdkExtUriResolver whenever the roots are set.
- Add unit tests for SdkExtUriResolver.

R=brianwilkerson@google.com

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

[Cutch, 2015-07-07 19:35:36]

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

[Cutch, 2015-07-07 19:39:26]

I'm interested in some early feedback.

AFAIK this should work.

[Brian Wilkerson, 2015-07-07 19:59:12]

It needs some tests, of course, but I agree, this looks good.

https://codereview.chromium.org/1213613017/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/analysis_server.dart (right):

https://codereview.chromium.org/1213613017/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/analysis_server.dart:1436: (packageUriResolver is
PackageMapUriResolver)) {
Given that "packageUriResolver is PackageMapUriResolver" will return false if
packageUriResolver is null, this could be simplified (and then the cast below
should become unnecessary).

https://codereview.chromium.org/1213613017/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/sdk_ext.dart (right):

https://codereview.chromium.org/1213613017/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/sdk_ext.dart:5: library sdk.ext;
nit: I think following the library naming convention would make this
"analysis_server.src.sdk_ext".

https://codereview.chromium.org/1213613017/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/sdk_ext.dart:17: // Given a packageMap (from a
PackageMapProvider), check in each package's lib
nit: The comments should be valid dartdoc comments.

https://codereview.chromium.org/1213613017/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/sdk_ext.dart:48: return new
FileBasedSource(javaFile);
This needs to be "new FileBasedSource(javaFile, uri)" in order to preserve the
original URI.

[Cutch, 2015-07-07 20:33:48]

I've tested this locally but could use some pointers on how this should be
tested within the analysis_server project.

https://codereview.chromium.org/1213613017/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/analysis_server.dart (right):

https://codereview.chromium.org/1213613017/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/analysis_server.dart:1436: (packageUriResolver is
PackageMapUriResolver)) {
On 2015/07/07 19:59:11, Brian Wilkerson wrote:
> Given that "packageUriResolver is PackageMapUriResolver" will return false if
> packageUriResolver is null, this could be simplified (and then the cast below
> should become unnecessary).

Done.

https://codereview.chromium.org/1213613017/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/sdk_ext.dart (right):

https://codereview.chromium.org/1213613017/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/sdk_ext.dart:5: library sdk.ext;
On 2015/07/07 19:59:12, Brian Wilkerson wrote:
> nit: I think following the library naming convention would make this
> "analysis_server.src.sdk_ext".

Done.

https://codereview.chromium.org/1213613017/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/sdk_ext.dart:17: // Given a packageMap (from a
PackageMapProvider), check in each package's lib
On 2015/07/07 19:59:11, Brian Wilkerson wrote:
> nit: The comments should be valid dartdoc comments.

Done.

https://codereview.chromium.org/1213613017/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/sdk_ext.dart:48: return new
FileBasedSource(javaFile);
On 2015/07/07 19:59:11, Brian Wilkerson wrote:
> This needs to be "new FileBasedSource(javaFile, uri)" in order to preserve the
> original URI.

Done.

[Brian Wilkerson, 2015-07-07 20:48:01]

I'd recommend some unit tests for SdkExtUriResolver (presumably in a file named
"test/src/sdk_ext_test.dart"). We use the unittest framework, and have also
written support for more JUnit-like tests, if that's more familiar/comfortable
for you (see test/generated/source_factory_test.dart for an example).

It would also be good to add a test (or tests) in SourceFactoryTest that tests
that when there's both a DartUriResolver and an SdkExtUriResolver that both the
built-in and added libraries resolve correctly.

[Cutch, 2015-07-07 21:47:43]

PTAL

[Brian Wilkerson, 2015-07-07 22:10:18]

https://codereview.chromium.org/1213613017/diff/60001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/analysis_server.dart (right):

https://codereview.chromium.org/1213613017/diff/60001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/analysis_server.dart:1445: sdkExtResolver,
If the packageUriResolver is null, then packageMap will be null. Is there any
point in adding the sdkExtResolver to the list in this case (or creating one)?

https://codereview.chromium.org/1213613017/diff/60001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/sdk_ext.dart (right):

https://codereview.chromium.org/1213613017/diff/60001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/sdk_ext.dart:29: class SdkExtUriResolver extends
UriResolver {
The lack of an implementation of restoreAbsolute will probably cause problems.
We use that to method to get back to a string like "dart:sky" for the purposes
of inserting import statements, refactorings, etc. (Though now that I'm thinking
about it, we might be able to get rid of that method at this point.) Still, we
probably ought to implement it until we can fix the callers.

[Cutch, 2015-07-08 14:18:17]

PTAL

https://codereview.chromium.org/1213613017/diff/60001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/analysis_server.dart (right):

https://codereview.chromium.org/1213613017/diff/60001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/analysis_server.dart:1445: sdkExtResolver,
On 2015/07/07 22:10:18, Brian Wilkerson wrote:
> If the packageUriResolver is null, then packageMap will be null. Is there any
> point in adding the sdkExtResolver to the list in this case (or creating one)?

Done.

https://codereview.chromium.org/1213613017/diff/60001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/sdk_ext.dart (right):

https://codereview.chromium.org/1213613017/diff/60001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/sdk_ext.dart:29: class SdkExtUriResolver extends
UriResolver {
On 2015/07/07 22:10:18, Brian Wilkerson wrote:
> The lack of an implementation of restoreAbsolute will probably cause problems.
> We use that to method to get back to a string like "dart:sky" for the purposes
> of inserting import statements, refactorings, etc. (Though now that I'm
thinking
> about it, we might be able to get rid of that method at this point.) Still, we
> probably ought to implement it until we can fix the callers.

I've implemented this and added tests but I'll note that both |DartUriResolver|
and |CustomUriResolver| lack implementations of restoreAbsolute. I wonder if
this is already unused?

[Brian Wilkerson, 2015-07-08 14:48:28]

LGTM, and thanks for doing this!

https://codereview.chromium.org/1213613017/diff/60001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/sdk_ext.dart (right):

https://codereview.chromium.org/1213613017/diff/60001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/sdk_ext.dart:29: class SdkExtUriResolver extends
UriResolver {
> I wonder if this is already unused?

It's used in 5 places in the analysis server. I'm not sure why it isn't
implemented in those places.

[Cutch, 2015-07-08 14:51:46]

Committed patchset #6 (id:100001) manually as
598336788d7a4c76853225cb225c5c5b69f258cd (presubmit successful).