Fix incremental resolution of files referenced by multiple contexts.

pkg/analysis_server/lib/src/analysis_server.dart

 // Copyright (c) 2014, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 library analysis.server;
 
 import 'dart:async';
 import 'dart:collection';
 
 import 'package:analysis_server/src/analysis_logger.dart';
@@ skipping 818 matching lines @@
       }
     });
     // remember new subscriptions
     this.analysisServices = subscriptions;
   }
 
   /**
    * Set the priority files to the given [files].
    */
   void setPriorityFiles(String requestId, List<String> files) {
+    // Note: when a file is a priority file, that information needs to be
+    // propagated to all contexts that analyze the file, so that all contexts
+    // will be able to do incremental resolution of the file.  See
+    // dartbug.com/22209.
     Map<AnalysisContext, List<Source>> sourceMap =
         new HashMap<AnalysisContext, List<Source>>();
     List<String> unanalyzed = new List<String>();
     files.forEach((file) {
-      AnalysisContext analysisContext = getAnalysisContext(file);
-      if (analysisContext == null) {
+      AnalysisContext preferredContext = getAnalysisContext(file);
+      Source source = getSource(file);
+      bool contextFound = false;
+      for (AnalysisContext context in folderMap.values) {
+        if (context == preferredContext ||
+            context.getKindOf(source) != SourceKind.UNKNOWN) {

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

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.

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

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.
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.

+          sourceMap.putIfAbsent(context, () => <Source>[]).add(source);
+          contextFound = true;
+        }
+      }
+      if (!contextFound) {
         unanalyzed.add(file);
-      } else {
-        List<Source> sourceList = sourceMap[analysisContext];
-        if (sourceList == null) {
-          sourceList = <Source>[];
-          sourceMap[analysisContext] = sourceList;
-        }
-        sourceList.add(getSource(file));
       }
     });
     if (unanalyzed.isNotEmpty) {
       StringBuffer buffer = new StringBuffer();
       buffer.writeAll(unanalyzed, ', ');
       throw new RequestFailure(
           new Response.unanalyzedPriorityFiles(requestId, buffer.toString()));
     }
     folderMap.forEach((Folder folder, AnalysisContext context) {
       List<Source> sourceList = sourceMap[context];
       if (sourceList == null) {
         sourceList = Source.EMPTY_ARRAY;
       }
       context.analysisPriorityOrder = sourceList;
+      // Schedule the context for analysis so that it has the opportunity to
+      // cache the AST's for the priority sources as soon as possible.
+      schedulePerformAnalysisOperation(context);

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

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]

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]

You are right, _getNextAnalysisTaskForSource handles priority sources
differently, and does prepare a resolved unit.

     });
     operationQueue.reschedule();
     Source firstSource = files.length > 0 ? getSource(files[0]) : null;
     _onPriorityChangeController.add(new PriorityChangeEvent(firstSource));
   }
 
   /**
    * Returns `true` if errors should be reported for [file] with the given
    * absolute path.
    */
@@ skipping 258 matching lines @@
    * [packageUriResolver].
    */
   SourceFactory _createSourceFactory(UriResolver packageUriResolver) {
     List<UriResolver> resolvers = <UriResolver>[
         new DartUriResolver(analysisServer.defaultSdk),
         new ResourceUriResolver(resourceProvider),
         packageUriResolver];
     return new SourceFactory(resolvers);
   }
 }