Add _embedder.yaml support to analyzer and analysis_server

Add _embedder.yaml support to analyzer and analysis_server

- If a package has an '_embedder.yaml'  file with an 'embedder_libs' key two things happen:
  1) We do not use the DartUriResolver to resolve dart: libraries.
  2) We use the EmbedderUriResolver to resolve all dart: libraries

- If multiple packages have an '_embedder.yaml' file we merge them.
  - This might not be the final behaviour that we want but I'm not sure how to surface errors to the end user.

- The '_embedder.yaml' file has a top level key 'embedder_libs' which is a map from dart: library uri to source path. Other keys are ignored by the EmbedderUriResolver.

- Unit tests for analyzer
- Integration test for analysis_server.

R=pquitslund@google.com

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

[Cutch, 2015-11-11 17:23:30]

Description was changed from

==========
Add _embedder.yaml support to analyzer and analysis_server
==========

to

==========
Add _embedder.yaml support to analyzer and analysis_server

- If a package has an '_embedder.yaml' file two things happen:
  1) We do not use the DartUriResolver to resolve dart: libraries.
  2) We use the EmbedderUriResolver to resolve all dart: libraries

- If multiple packages have an '_embedder.yaml' file we merge them.
  - This might not be the final behaviour that we want but I'm not sure how to
surface errors to the end user.

- The '_embedder.yaml' file has a top level key 'embedder_libs' which is a map
from dart: library uri to source path. Other keys are ignored by the
EmbedderUriResolver.

- Unit tests for analyzer
- Integration test for analysis_server.
==========

[Cutch, 2015-11-11 17:23:30]

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

[Cutch, 2015-11-11 17:25:18]

Description was changed from

==========
Add _embedder.yaml support to analyzer and analysis_server

- If a package has an '_embedder.yaml' file two things happen:
  1) We do not use the DartUriResolver to resolve dart: libraries.
  2) We use the EmbedderUriResolver to resolve all dart: libraries

- If multiple packages have an '_embedder.yaml' file we merge them.
  - This might not be the final behaviour that we want but I'm not sure how to
surface errors to the end user.

- The '_embedder.yaml' file has a top level key 'embedder_libs' which is a map
from dart: library uri to source path. Other keys are ignored by the
EmbedderUriResolver.

- Unit tests for analyzer
- Integration test for analysis_server.
==========

to

==========
Add _embedder.yaml support to analyzer and analysis_server

- If a package has an '_embedder.yaml'  file with an 'embedder_libs' key two
things happen:
  1) We do not use the DartUriResolver to resolve dart: libraries.
  2) We use the EmbedderUriResolver to resolve all dart: libraries

- If multiple packages have an '_embedder.yaml' file we merge them.
  - This might not be the final behaviour that we want but I'm not sure how to
surface errors to the end user.

- The '_embedder.yaml' file has a top level key 'embedder_libs' which is a map
from dart: library uri to source path. Other keys are ignored by the
EmbedderUriResolver.

- Unit tests for analyzer
- Integration test for analysis_server.
==========

[sethladd, 2015-11-11 17:38:47]

Thanks John!

@Phil, what's the best way to log a message when we run into two embedder.yaml
files? Is there a logger we can use?

[Cutch, 2015-11-11 17:40:59]

On 2015/11/11 17:38:47, sethladd wrote:
> Thanks John!
> 
> @Phil, what's the best way to log a message when we run into two embedder.yaml
> files? Is there a logger we can use?

Not only the case of two embedder.yaml files but what if an embedder.yaml file
has a syntax error. How do I surface something in general to the end user
developer (i.e. in the analysis window in Atom)?

[pquitslund, 2015-11-11 19:07:04]

On 2015/11/11 17:40:59, Cutch wrote:
> On 2015/11/11 17:38:47, sethladd wrote:
> > Thanks John!

Yeah, thanks!

> > 
> > @Phil, what's the best way to log a message when we run into two
embedder.yaml
> > files? Is there a logger we can use?
> 
> Not only the case of two embedder.yaml files but what if an embedder.yaml file
> has a syntax error. How do I surface something in general to the end user
> developer (i.e. in the analysis window in Atom)?

I'm late to this party because we were actually just talking about this very
thing!  Short answer: there's not a good way.  I hope to have a better answer to
that before long.  In the meantime, let's just add a few TODOs.

[pquitslund, 2015-11-11 19:19:10]

LGTM w/ a possible refinement.

https://codereview.chromium.org/1437893003/diff/20001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/context_manager.dart (right):

https://codereview.chromium.org/1437893003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/context_manager.dart:1413: new
EmbedderUriResolver(packageMap),
A consequence of doing the _embedder.yaml discovery in the resolver is that
we'll need to do it again for options.  It might be better if we could do that
just once?  (Aside: there's technically a race condition too in case that file
gets changed between reads but that's a bit pathological.)

What about hoisting that logic up and into context_manager and passing in the
discovered yaml file(s)?

https://codereview.chromium.org/1437893003/diff/20001/pkg/analyzer/lib/source...
File pkg/analyzer/lib/source/embedder.dart (right):

https://codereview.chromium.org/1437893003/diff/20001/pkg/analyzer/lib/source...
pkg/analyzer/lib/source/embedder.dart:136: } catch (e) {
Maybe a TODO to do some error handling down the road?

https://codereview.chromium.org/1437893003/diff/20001/pkg/analyzer/lib/source...
pkg/analyzer/lib/source/embedder.dart:143: if ((embedder_libs == null) ||
(embedder_libs is! YamlMap)) {
Maybe just:

if (embedder_libs is! YamlMap) {
   ...
}

The null check is unneeded I think?

[sethladd, 2015-11-11 19:52:56]

Phil, can you open an issue on analyzer for a way to log, so we can make it
easier to diagnose issues if they pop up? Thanks!

[pquitslund, 2015-11-11 21:19:34]

Nits.

https://codereview.chromium.org/1437893003/diff/20001/pkg/analysis_server/tes...
File pkg/analysis_server/test/context_manager_test.dart (right):

https://codereview.chromium.org/1437893003/diff/20001/pkg/analysis_server/tes...
pkg/analysis_server/test/context_manager_test.dart:600: expect(contexts.length,
equals(1));
[Nit]: can be simplified to:

expect(contexts.length, 1);

https://codereview.chromium.org/1437893003/diff/20001/pkg/analysis_server/tes...
pkg/analysis_server/test/context_manager_test.dart:604: expect(source.fullName,
equals('/my/proj/sdk_ext/entry.dart'));
[Nit]: can be simplified to:

expect(source.fullName, '/my/proj/sdk_ext/entry.dart');

[pquitslund, 2015-11-11 21:24:52]

On 2015/11/11 19:52:56, sethladd wrote:
> Phil, can you open an issue on analyzer for a way to log, so we can make it
> easier to diagnose issues if they pop up? Thanks!

Sure thing.

https://github.com/dart-lang/sdk/issues/24889

Feel free to fill in any more details.

Thanks!

[Brian Wilkerson, 2015-11-11 22:09:14]

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

[Brian Wilkerson, 2015-11-11 22:09:14]

https://codereview.chromium.org/1437893003/diff/20001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/context_manager.dart (right):

https://codereview.chromium.org/1437893003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/context_manager.dart:1413: new
EmbedderUriResolver(packageMap),
> ... we'll need to do it again for options.

What do you mean by "options"?

https://codereview.chromium.org/1437893003/diff/20001/pkg/analysis_server/tes...
File pkg/analysis_server/test/context_manager_test.dart (right):

https://codereview.chromium.org/1437893003/diff/20001/pkg/analysis_server/tes...
pkg/analysis_server/test/context_manager_test.dart:600: expect(contexts.length,
equals(1));
or, better yet:

expect(contexts, hasLength(1));

https://codereview.chromium.org/1437893003/diff/20001/pkg/analyzer/lib/source...
File pkg/analyzer/lib/source/embedder.dart (right):

https://codereview.chromium.org/1437893003/diff/20001/pkg/analyzer/lib/source...
pkg/analyzer/lib/source/embedder.dart:143: if ((embedder_libs == null) ||
(embedder_libs is! YamlMap)) {
> The null check is unneeded I think?

Nope. e is! T is always true (unless T is Object, dynamic, or Null).

[pquitslund, 2015-11-11 23:17:16]

https://codereview.chromium.org/1437893003/diff/20001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/context_manager.dart (right):

https://codereview.chromium.org/1437893003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/context_manager.dart:1413: new
EmbedderUriResolver(packageMap),
On 2015/11/11 22:09:14, Brian Wilkerson wrote:
> > ... we'll need to do it again for options.
> 
> What do you mean by "options"?
> 

Analysis options can also be set by embedders in this file.  They will set the
defaults (available for overriding in a local `.analysis_options` file).

[Brian Wilkerson, 2015-11-11 23:59:36]

https://codereview.chromium.org/1437893003/diff/20001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/context_manager.dart (right):

https://codereview.chromium.org/1437893003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/context_manager.dart:1413: new
EmbedderUriResolver(packageMap),
Right! I'd forgotten. Thanks.

[thaboss1018, 2015-11-12 00:19:05]

thaboss1018@gmail.com changed reviewers:
+ thaboss1018@gmail.com

[Cutch, 2015-11-12 14:13:43]

- I've separated the finding / reading of _embedder.yaml files from the uri
resolver. This should make it easier to use for options and removes the race
condition of reading it twice.

PTAL.

https://codereview.chromium.org/1437893003/diff/20001/pkg/analyzer/lib/source...
File pkg/analyzer/lib/source/embedder.dart (right):

https://codereview.chromium.org/1437893003/diff/20001/pkg/analyzer/lib/source...
pkg/analyzer/lib/source/embedder.dart:136: } catch (e) {
On 2015/11/11 19:19:09, pquitslund wrote:
> Maybe a TODO to do some error handling down the road?

Done.

https://codereview.chromium.org/1437893003/diff/20001/pkg/analyzer/lib/source...
pkg/analyzer/lib/source/embedder.dart:143: if ((embedder_libs == null) ||
(embedder_libs is! YamlMap)) {
On 2015/11/11 22:09:14, Brian Wilkerson wrote:
> > The null check is unneeded I think?
> 
> Nope. e is! T is always true (unless T is Object, dynamic, or Null).

Acknowledged.

https://codereview.chromium.org/1437893003/diff/20001/pkg/analyzer/lib/source...
pkg/analyzer/lib/source/embedder.dart:143: if ((embedder_libs == null) ||
(embedder_libs is! YamlMap)) {
On 2015/11/11 19:19:09, pquitslund wrote:
> Maybe just:
> 
> if (embedder_libs is! YamlMap) {
>    ...
> }
> 
> The null check is unneeded I think?

Acknowledged.

[pquitslund, 2015-11-12 17:43:09]

LGTM % an API question that I'll punt to Brian.

Thanks a million for breaking this up!

https://codereview.chromium.org/1437893003/diff/40001/pkg/analyzer/lib/source...
File pkg/analyzer/lib/source/embedder.dart (right):

https://codereview.chromium.org/1437893003/diff/40001/pkg/analyzer/lib/source...
pkg/analyzer/lib/source/embedder.dart:64: // TODO(pquitslund): Notify developer
that something is wrong with the
I guess I'm on the hook! :)

https://codereview.chromium.org/1437893003/diff/40001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/engine.dart (right):

https://codereview.chromium.org/1437893003/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/engine.dart:335: EmbedderYamlLocator get
embedderYamlLocator;
I'll defer to Brian as to whether we need this in the legacy `AnalysisContext`,
which is on the brink of disappearing.  Either way, I'd hope we could avoid this
leaking into API.  (That is keep it just in the impl.)

[Cutch, 2015-11-12 17:46:32]

On 2015/11/12 17:43:09, pquitslund wrote:
> LGTM % an API question that I'll punt to Brian.
> 
> Thanks a million for breaking this up!
> 
>
https://codereview.chromium.org/1437893003/diff/40001/pkg/analyzer/lib/source...
> File pkg/analyzer/lib/source/embedder.dart (right):
> 
>
https://codereview.chromium.org/1437893003/diff/40001/pkg/analyzer/lib/source...
> pkg/analyzer/lib/source/embedder.dart:64: // TODO(pquitslund): Notify
developer
> that something is wrong with the
> I guess I'm on the hook! :)
> 
>
https://codereview.chromium.org/1437893003/diff/40001/pkg/analyzer/lib/src/ge...
> File pkg/analyzer/lib/src/generated/engine.dart (right):
> 
>
https://codereview.chromium.org/1437893003/diff/40001/pkg/analyzer/lib/src/ge...
> pkg/analyzer/lib/src/generated/engine.dart:335: EmbedderYamlLocator get
> embedderYamlLocator;
> I'll defer to Brian as to whether we need this in the legacy
`AnalysisContext`,
> which is on the brink of disappearing.  Either way, I'd hope we could avoid
this
> leaking into API.  (That is keep it just in the impl.)

SGTM. I took a stab at where to put it but I lack so much context, please just
tell me where you guys want this to go and I'll update.

[Cutch, 2015-11-12 23:13:25]

*ping*

[Brian Wilkerson, 2015-11-13 19:54:18]

Except as noted in individual comments, the code changes look fine to me. I'd
prefer to wait until 1.13 has shipped in order to reduce the chance for merge
conflicts, but I understand there are some other time constraints.

https://codereview.chromium.org/1437893003/diff/40001/pkg/analyzer/lib/source...
File pkg/analyzer/lib/source/embedder.dart (right):

https://codereview.chromium.org/1437893003/diff/40001/pkg/analyzer/lib/source...
pkg/analyzer/lib/source/embedder.dart:24: final Map<Folder, YamlMap>
embedderYamls = {};
"{}" --> "new HashMap<Folder, YamlMap>()"

It's more efficient, so we use it everywhere.

https://codereview.chromium.org/1437893003/diff/40001/pkg/analyzer/lib/source...
pkg/analyzer/lib/source/embedder.dart:49: for (var libDir in libDirs) {
In the analyzer code base we use type annotations everywhere. (We haven't been
as careful in analysis_server, much to my disappointment.) We like the extra
checking being done when we run our tests in checked mode.

https://codereview.chromium.org/1437893003/diff/40001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/engine.dart (right):

https://codereview.chromium.org/1437893003/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/engine.dart:335: EmbedderYamlLocator get
embedderYamlLocator;
So to be clear, AnalysisContext is the public interface for analysis contexts,
and it won't be going away. (The class AnalysisContextImpl in this file will be
going away.)

But I agree, we don't want this in the public API.

It's kind of hard to search for uses in the code review tool, so I can't see
where this getter is being used, but the way we typically handle this is to add
an API like this to InternalAnalysisContext. Most of our code already uses that
interface rather than the public interface, so the getter would likely still be
available where you need it.

[Cutch, 2015-11-14 00:15:17]

https://codereview.chromium.org/1437893003/diff/40001/pkg/analyzer/lib/source...
File pkg/analyzer/lib/source/embedder.dart (right):

https://codereview.chromium.org/1437893003/diff/40001/pkg/analyzer/lib/source...
pkg/analyzer/lib/source/embedder.dart:24: final Map<Folder, YamlMap>
embedderYamls = {};
On 2015/11/13 19:54:18, Brian Wilkerson wrote:
> "{}" --> "new HashMap<Folder, YamlMap>()"
> 
> It's more efficient, so we use it everywhere.

Done.

https://codereview.chromium.org/1437893003/diff/40001/pkg/analyzer/lib/source...
pkg/analyzer/lib/source/embedder.dart:49: for (var libDir in libDirs) {
On 2015/11/13 19:54:18, Brian Wilkerson wrote:
> In the analyzer code base we use type annotations everywhere. (We haven't been
> as careful in analysis_server, much to my disappointment.) We like the extra
> checking being done when we run our tests in checked mode.

Done here and elsewhere.

https://codereview.chromium.org/1437893003/diff/40001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/engine.dart (right):

https://codereview.chromium.org/1437893003/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/engine.dart:335: EmbedderYamlLocator get
embedderYamlLocator;
Moved to InternalAnalysisContext.

[pquitslund, 2015-11-14 00:32:53]

LGTM!

A few nits but nothing show-stopping.  Thanks for all the care!

https://codereview.chromium.org/1437893003/diff/80001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/context_manager.dart (right):

https://codereview.chromium.org/1437893003/diff/80001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/context_manager.dart:1459: var packageMap =
buildPackageMap(resourceProvider);
Nit: missing type:

  Map<String, List<Folder>> packageMap = ...

https://codereview.chromium.org/1437893003/diff/80001/pkg/analyzer/lib/source...
File pkg/analyzer/lib/source/embedder.dart (right):

https://codereview.chromium.org/1437893003/diff/80001/pkg/analyzer/lib/source...
pkg/analyzer/lib/source/embedder.dart:64: } catch (e) {
Nit: (e) => (_)

https://codereview.chromium.org/1437893003/diff/80001/pkg/analyzer/test/sourc...
File pkg/analyzer/test/source/embedder_test.dart (right):

https://codereview.chromium.org/1437893003/diff/80001/pkg/analyzer/test/sourc...
pkg/analyzer/test/source/embedder_test.dart:82: "fart:loudly": "nomatter.dart"
Just caught this. :)

[Cutch, 2015-11-14 00:36:45]

I'll wait until Monday to land this. Brian, let me know if you have any other
issues.

https://codereview.chromium.org/1437893003/diff/80001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/context_manager.dart (right):

https://codereview.chromium.org/1437893003/diff/80001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/context_manager.dart:1459: var packageMap =
buildPackageMap(resourceProvider);
On 2015/11/14 00:32:53, pquitslund wrote:
> Nit: missing type:
> 
>   Map<String, List<Folder>> packageMap = ...

Done.

https://codereview.chromium.org/1437893003/diff/80001/pkg/analyzer/lib/source...
File pkg/analyzer/lib/source/embedder.dart (right):

https://codereview.chromium.org/1437893003/diff/80001/pkg/analyzer/lib/source...
pkg/analyzer/lib/source/embedder.dart:64: } catch (e) {
On 2015/11/14 00:32:53, pquitslund wrote:
> Nit: (e) => (_)

Done.

https://codereview.chromium.org/1437893003/diff/80001/pkg/analyzer/test/sourc...
File pkg/analyzer/test/source/embedder_test.dart (right):

https://codereview.chromium.org/1437893003/diff/80001/pkg/analyzer/test/sourc...
pkg/analyzer/test/source/embedder_test.dart:82: "fart:loudly": "nomatter.dart"
On 2015/11/14 00:32:53, pquitslund wrote:
> Just caught this. :)

:)

[Cutch, 2015-11-16 23:30:22]

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

[scheglov, 2015-11-17 04:05:52]

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

[scheglov, 2015-11-17 04:05:53]

https://codereview.chromium.org/1437893003/diff/100001/pkg/analysis_server/li...
File pkg/analysis_server/lib/src/analysis_server.dart (right):

https://codereview.chromium.org/1437893003/diff/100001/pkg/analysis_server/li...
pkg/analysis_server/lib/src/analysis_server.dart:1537: /// files.
Please use the same style of documentation comments as the file already uses.