Fix incremental resolution of files referenced by multiple contexts.

Fix incremental resolution of files referenced by multiple contexts.

There were two problems:

1. When a file was set as a priority, we weren't propagating that
information to all contexts; as a result, the resolved AST wasn't
cached in all contexts, so incremental resolution wasn't able to take
place in all contexts.

2. In setPriorityFiles we weren't scheduling analysis operations, so
even after the file was made a priority, its resolved AST wouldn't be
available until some other event triggered analysis in that context.

BUG=dartbug.com/22209
R=scheglov@google.com

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

[Paul Berry, 2015-01-29 23:03:38]

paulberry@google.com changed reviewers:
+ danrubel@google.com, scheglov@google.com

[scheglov, 2015-01-29 23:22:16]

LGTM

Would be nice to have a test.

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

https://codereview.chromium.org/892493003/diff/40001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/analysis_server.dart:852: context.getKindOf(source)
!= SourceKind.UNKNOWN) {
It seems that if analysis isn't complete yet, and some external file is
referenced by a context, we will report an error, because we don't know yet if
the file is included into some context or not.

Maybe list of priority files also should be server-global.

https://codereview.chromium.org/892493003/diff/40001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/analysis_server.dart:875:
schedulePerformAnalysisOperation(context);
I'm not sure we need this.

AFAIK we don't schedule any tasks in AnalysisContextImpl.
So, we will not prepare AST's in advance.
OTOH, if the analysis is already in progress, the next task will be priority
source analysis.

[Paul Berry, 2015-01-30 00:19:45]

(Brian, I've added you to see if you can advise on whether
resolveCompilationUnit2() is safe to call from within analysis server--see
below.)

Regarding Konstantin's comment on testing: I agree that it would be nice to have
a test, but I'm not sure how to write one, since the analysis context doesn't
seem to reveal enough information to allow the test to determine whether
incremental analysis or full analysis was used.  I'm open to suggestions.

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

https://codereview.chromium.org/892493003/diff/40001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/analysis_server.dart:852: context.getKindOf(source)
!= SourceKind.UNKNOWN) {
On 2015/01/29 23:22:16, scheglov wrote:
> It seems that if analysis isn't complete yet, and some external file is
> referenced by a context, we will report an error, because we don't know yet if
> the file is included into some context or not.

Aha, you're right!  I'd forgotten that we'd changed this part of the spec.  I
guess we forgot to change the code to match.  I'll make a follow up CL that
removes the error condition so that the behavior matches the spec.

> 
> Maybe list of priority files also should be server-global.

That's a good idea.  It would fix some corner cases like priority files getting
forgotten about when the user does "reanalyze sources".  Let's discuss on Monday
when you and Brian are back from vacation.

https://codereview.chromium.org/892493003/diff/40001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/analysis_server.dart:875:
schedulePerformAnalysisOperation(context);
On 2015/01/29 23:22:16, scheglov wrote:
> I'm not sure we need this.
> 
> AFAIK we don't schedule any tasks in AnalysisContextImpl.
> So, we will not prepare AST's in advance.
> OTOH, if the analysis is already in progress, the next task will be priority
> source analysis.

Hmm, I tried the CL without this line at first, and it didn't work.  Let me dig
a little deeper to figure out why this line wasn't needed before revision
43163...

Ok, I understand now.  The reason it was working before revision 43163 is that
when a file becomes visible, editor makes two requests of the server: it sets
the file as a priority, and it requests navigation, occurrences, overrides,
outline, and highlights for the file.  Requesting the file as a priority
wouldn't cause any analysis to get scheduled.  But requesting navigation,
occurrences, etc. would cause setAnalysisSubscriptions() to call
getResolvedCompilationUnitToResendNotification(), which would call
context.resolveCompilationUnit2(), and that would compute the resolved AST
synchronously*.

(*It's a little scary, though, because there's a comment above
resolveCompilationUnit2() that says "This method cannot be used in an async
environment."  The analysis server sure seems like an async environment to me,
and yet things were working, so I'm not sure what that comment was warning
about.  I'll cc Brian on this review in case he has any insight.)

After revision 43163, and with this CL applied, but without the call to
schedulePerformAnalysisOperation, things don't work because
getResolvedCompilationUnitToResendNotification() only calls
context.resolveCompilationUnit2() for one context.  That means the other
contexts don't get a resolved AST, so the first attempt at incremental
resolution fails in those contexts.  (Later attempts at incremental resolution
succeed, though, because after a round of full analysis, the context knows that
the file is a priority file so it saves the resolved AST.)

By adding a call to schedulePerformAnalysisOperation here, I worked around the
problem by ensuring that all contexts would notice that they needed to compute a
resolved AST for the new priority file.

I'm going to go ahead with this CL for now, since it improves things and I'm not
sure what the correct approach is.  We should probably discuss this on Monday
and figure out if this is how we really want it to work.

[scheglov, 2015-01-30 00:25:49]

Even more LGTM :-)

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

https://codereview.chromium.org/892493003/diff/40001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/analysis_server.dart:875:
schedulePerformAnalysisOperation(context);
You are right, _getNextAnalysisTaskForSource handles priority sources
differently, and does prepare a resolved unit.

[Paul Berry, 2015-01-30 00:28:03]

Committed patchset #3 (id:40001) manually as 43323 (presubmit successful).