Partial implementation of the 'setAnalysisRoots' API.

Partial implementation of the 'setAnalysisRoots' API.

R=brianwilkerson@google.com, paulberry@google.com
BUG=

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

[Brian Wilkerson, 2014-05-27 14:09:56]

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

https://codereview.chromium.org/300023004/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:235: void
setAnalysisRoots(Set<Folder> includedFolders, Set<Folder> excludedFolders) {
This doesn't look for nested pub folders.

https://codereview.chromium.org/300023004/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:245: PubFolder pubFolder = new
PubFolder(folder);
What about analysis roots that do not contain a pubspec?

https://codereview.chromium.org/300023004/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:257: if
(path.startsWith(folder.fullName)) {
Would it be better to use the names of folders as keys rather than the folders
themselves?

https://codereview.chromium.org/300023004/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:344: static final DartSdk
DEFAULT_SDK = DirectoryBasedDartSdk.defaultSdk;
I don't think this is the best place to store the default SDK. I think the
instance of the AnalysisServer would be better.

Also, this cannot be final because clients can reset the default SDK.

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

https://codereview.chromium.org/300023004/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/domain_analysis.dart:214: throw new RequestFailure(
The specification says that files can be included. The intent, which I don't
think is documented yet, is that we will look up the directory chain to find a
directory that contains a pubspec, or use the parent directory if there is no
ancestor with a pubspec.

https://codereview.chromium.org/300023004/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/domain_analysis.dart:224: if
(excludedPaths.isNotEmpty) {
Seems like the better place to fail would be in server.setAnalysisRoots (because
it will be harder to forget to remove the failure while implementing the
functionality).

[Paul Berry, 2014-05-27 16:30:29]

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

https://codereview.chromium.org/300023004/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:236: Set<Folder> currentFolders
= new Set<Folder>.from(folderMap.keys);
What about analysis roots that aren't folders?

https://codereview.chromium.org/300023004/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:255: AnalysisContext
test_getAnalysisContext(String path) {
What does it mean that this function (and test_getResolvedCompilationUnit) begin
with "test_"?

https://codereview.chromium.org/300023004/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:265: * Return the
[CompilationUnit] of the Dart file with the given [path].
What happens if the dart file in question isn't underneath a directory with a
pubspec?  This could happen if the file in question is inside the SDK.  It will
also be possible for files in packages, once we move away from relying on the
"packages" links (which I'm hoping we can do ASAP).

https://codereview.chromium.org/300023004/diff/1/pkg/analysis_server/test/dom...
File pkg/analysis_server/test/domain_analysis_test.dart (right):

https://codereview.chromium.org/300023004/diff/1/pkg/analysis_server/test/dom...
pkg/analysis_server/test/domain_analysis_test.dart:75: return
pumpEventQueue(1000).then((_) {
Why do we need to pump the event queue 1000 times?  This makes me suspicious
that either (a) there is a design flaw that's causing us to defer way too many
things to asynchronous processing, or (b) this test is actually dependent on IO,
in which case we should explicitly wait for the IO to complete rather than loop
1000 times.

[scheglov, 2014-05-27 18:24:23]

PTAL

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

https://codereview.chromium.org/300023004/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:235: void
setAnalysisRoots(Set<Folder> includedFolders, Set<Folder> excludedFolders) {
On 2014/05/27 14:09:57, Brian Wilkerson wrote:
> This doesn't look for nested pub folders.

Yes, and it is intentional.
I've added comment to this method to specify this temporary limitation.

https://codereview.chromium.org/300023004/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:255: AnalysisContext
test_getAnalysisContext(String path) {
On 2014/05/27 16:30:29, Paul Berry wrote:
> What does it mean that this function (and test_getResolvedCompilationUnit)
begin
> with "test_"?

These methods are supposed to be used only by tests.

https://codereview.chromium.org/300023004/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:257: if
(path.startsWith(folder.fullName)) {
On 2014/05/27 14:09:57, Brian Wilkerson wrote:
> Would it be better to use the names of folders as keys rather than the folders
> themselves?

I don't know.
In theory Resource can provide some services, like Folder.containsPath(path).

But I don't have a strong opinion.

https://codereview.chromium.org/300023004/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:265: * Return the
[CompilationUnit] of the Dart file with the given [path].
On 2014/05/27 16:30:29, Paul Berry wrote:
> What happens if the dart file in question isn't underneath a directory with a
> pubspec?  This could happen if the file in question is inside the SDK.  It
will
> also be possible for files in packages, once we move away from relying on the
> "packages" links (which I'm hoping we can do ASAP).

I guess we will have to check if the file is under SDK directory and ask SDK for
the context instead of a PubFolder.

https://codereview.chromium.org/300023004/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:344: static final DartSdk
DEFAULT_SDK = DirectoryBasedDartSdk.defaultSdk;
On 2014/05/27 14:09:57, Brian Wilkerson wrote:
> I don't think this is the best place to store the default SDK. I think the
> instance of the AnalysisServer would be better.
> 
> Also, this cannot be final because clients can reset the default SDK.

Done.

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

https://codereview.chromium.org/300023004/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/domain_analysis.dart:214: throw new RequestFailure(
On 2014/05/27 14:09:57, Brian Wilkerson wrote:
> The specification says that files can be included. The intent, which I don't
> think is documented yet, is that we will look up the directory chain to find a
> directory that contains a pubspec, or use the parent directory if there is no
> ancestor with a pubspec.

I have changed the message and moved it into AnalysisServer.

https://codereview.chromium.org/300023004/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/domain_analysis.dart:224: if
(excludedPaths.isNotEmpty) {
On 2014/05/27 14:09:57, Brian Wilkerson wrote:
> Seems like the better place to fail would be in server.setAnalysisRoots
(because
> it will be harder to forget to remove the failure while implementing the
> functionality).

Done.

https://codereview.chromium.org/300023004/diff/1/pkg/analysis_server/test/dom...
File pkg/analysis_server/test/domain_analysis_test.dart (right):

https://codereview.chromium.org/300023004/diff/1/pkg/analysis_server/test/dom...
pkg/analysis_server/test/domain_analysis_test.dart:75: return
pumpEventQueue(1000).then((_) {
On 2014/05/27 16:30:29, Paul Berry wrote:
> Why do we need to pump the event queue 1000 times?  This makes me suspicious
> that either (a) there is a design flaw that's causing us to defer way too many
> things to asynchronous processing, or (b) this test is actually dependent on
IO,
> in which case we should explicitly wait for the IO to complete rather than
loop
> 1000 times.

We need do perform about 400 analysis tasks to analyze SDK.
I guess we need to the extract SDK analysis into a separate task and do it in
setUp().
Maybe even use a limited SDK from the memory FS.

[Brian Wilkerson, 2014-05-27 18:57:58]

LGTM

[scheglov, 2014-05-27 19:01:24]

Committed patchset #2 manually as r36702 (presubmit successful).