Allow completions to wait for analysis without requiring a busy wait loop.

Allow completions to wait for analysis without requiring a busy wait loop.

Two new methods are added to the AnalysisContext which return
Futures--the futures complete with the result of analysis once it's
available, avoiding the need for a busy wait loop in the completion engine.

The futures are guaranteed to complete eventually (provided that
analysis reaches quiescence).  If it is determined that the requested
analysis will never become available (e.g. because analysis was
requested for a file or file/library combination that is not subject
to analysis), the future will be completed with an
AnalysisNotScheduledError, so that the client isn't stalled forever
waiting for analysis results.

R=brianwilkerson@google.com

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

[Paul Berry, 2014-12-12 00:56:59]

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

[scheglov, 2014-12-12 03:41:55]

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

[scheglov, 2014-12-12 03:41:56]

Just a comment.

https://codereview.chromium.org/800723002/diff/1/pkg/analyzer/lib/src/generat...
File pkg/analyzer/lib/src/generated/engine.dart (right):

https://codereview.chromium.org/800723002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/engine.dart:771: LibraryElement library);
It would be slightly easier for clients to get library Source instead of
LibraryElement.

[Brian Wilkerson, 2014-12-12 15:13:12]

As you know, I don't like the addition of an asynchronous interface to the
engine, but I can't think of any good alternatives. So I'm reluctantly giving
this an LGTM.

https://codereview.chromium.org/800723002/diff/1/pkg/analyzer/lib/src/generat...
File pkg/analyzer/lib/src/generated/engine.dart (right):

https://codereview.chromium.org/800723002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/engine.dart:771: LibraryElement library);
> It would be slightly easier for clients to get library Source instead of
> LibraryElement.

In some cases, yes, in some cases no. And it's more efficient to pass the
element when the client already has it. That's why we have both variants for the
immediate case (getResolvedCompilationUnit and getResolvedCompilationUnit2).

https://codereview.chromium.org/800723002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/engine.dart:1226: if
(pendingFutures[i].evaluate(sourceEntry)) {
Could the evaluation of the pending future ever cause a change to the list of
pendingFutures? (My limited understanding is that it could if the computation of
the value being waited for depended on some other value from the same source,
but perhaps that doesn't happen.)

Could the evaluation of the pending future ever cause a change to
_pendingFutureSources? Would that cause problems for the outer 'for' loop?
(Subtleties of Dart collections that I haven't learned yet.)

https://codereview.chromium.org/800723002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/engine.dart:1253:
_pendingFutureSources.remove(source);
Could the evaluation of a pending future (on line 1226) ever cause a pending
future to be added to a source *after* the source had already been added to
sourcesToRemove?

https://codereview.chromium.org/800723002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/engine.dart:2011: if
(sourceEntry.hasErrorState()) {
I think that this should be checking explicitly for an error state related to
DartEntry.ELEMENT. If, for example, there were an exception while trying to
compute hints, then the element would be valid but the source entry would still
have an error state. (And if there is an exception while trying to parse the
file, then the element will also be marked as being in an error state.)

https://codereview.chromium.org/800723002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/engine.dart:2105: if
(sourceEntry.hasErrorState()) {
Ditto

https://codereview.chromium.org/800723002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/engine.dart:11711: _completer.completeError(new
AnalysisNotScheduledError());
Should this be

try {
  throw new AnalysisNotScheduledError();
} catch (exception, stackTrace) {
  _completer.completeError(exception, stackTrace);
}

so that we always get a stack trace for debugging purposes?

[scheglov, 2014-12-12 15:25:09]

https://codereview.chromium.org/800723002/diff/1/pkg/analyzer/lib/src/generat...
File pkg/analyzer/lib/src/generated/engine.dart (right):

https://codereview.chromium.org/800723002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/engine.dart:771: LibraryElement library);
On 2014/12/12 15:13:12, Brian Wilkerson wrote:
> > It would be slightly easier for clients to get library Source instead of
> > LibraryElement.
> 
> In some cases, yes, in some cases no. And it's more efficient to pass the
> element when the client already has it. That's why we have both variants for
the
> immediate case (getResolvedCompilationUnit and getResolvedCompilationUnit2).

It's much easier and cheaper for the client to go from LibraryElement to Source
when the other way around.
And implementation uses just Source anyway.

[Brian Wilkerson, 2014-12-12 15:31:42]

https://codereview.chromium.org/800723002/diff/1/pkg/analyzer/lib/src/generat...
File pkg/analyzer/lib/src/generated/engine.dart (right):

https://codereview.chromium.org/800723002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/engine.dart:771: LibraryElement library);
> And implementation uses just Source anyway.

Right. I'd forgotten that.

[danrubel, 2014-12-12 16:06:15]

https://codereview.chromium.org/800723002/diff/1/pkg/analyzer/lib/src/generat...
File pkg/analyzer/lib/src/generated/engine.dart (right):

https://codereview.chromium.org/800723002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/engine.dart:770: Future<CompilationUnit>
getResolvedCompilationUnitFuture(Source source,
How do I cancel waiting for this when I realize that the user has switched to
editing a different file?

[Paul Berry, 2014-12-12 17:36:39]

https://codereview.chromium.org/800723002/diff/1/pkg/analyzer/lib/src/generat...
File pkg/analyzer/lib/src/generated/engine.dart (right):

https://codereview.chromium.org/800723002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/engine.dart:770: Future<CompilationUnit>
getResolvedCompilationUnitFuture(Source source,
On 2014/12/12 16:06:15, danrubel wrote:
> How do I cancel waiting for this when I realize that the user has switched to
> editing a different file?

As discussed over VC, I'll make a follow-up CL that changes this Future into a
CancelableFuture (a new class which implements Future and provides a cancel()
method).  Assuming people like it, we may eventually want to try to get
CancelableFuture folded in to dart:async.

https://codereview.chromium.org/800723002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/engine.dart:771: LibraryElement library);
On 2014/12/12 15:25:09, scheglov wrote:
> On 2014/12/12 15:13:12, Brian Wilkerson wrote:
> > > It would be slightly easier for clients to get library Source instead of
> > > LibraryElement.
> > 
> > In some cases, yes, in some cases no. And it's more efficient to pass the
> > element when the client already has it. That's why we have both variants for
> the
> > immediate case (getResolvedCompilationUnit and getResolvedCompilationUnit2).
> 
> It's much easier and cheaper for the client to go from LibraryElement to
Source
> when the other way around.
> And implementation uses just Source anyway.

Good point.  Also, in this case, the client already had the source, and was
going through the pointless exercise of converting it to a LibraryElement just
so that it could pass it in to getResolvedCompilationUnitFuture.  I've reworked
the API to follow Konstantin's suggestion, and now the function
getLibraryElementFuture isn't needed at all.

https://codereview.chromium.org/800723002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/engine.dart:1226: if
(pendingFutures[i].evaluate(sourceEntry)) {
On 2014/12/12 15:13:12, Brian Wilkerson wrote:
> Could the evaluation of the pending future ever cause a change to the list of
> pendingFutures? (My limited understanding is that it could if the computation
of
> the value being waited for depended on some other value from the same source,
> but perhaps that doesn't happen.)
> 
> Could the evaluation of the pending future ever cause a change to
> _pendingFutureSources? Would that cause problems for the outer 'for' loop?
> (Subtleties of Dart collections that I haven't learned yet.)

You are right in general that it's not safe to modify a collection while you're
iterating through it with a 'for' loop (you'll get a ConcurrentModificationError
in most circumstances).  However, my design intent here was that the evaluate()
call should be free of side effects (except for the side effect of completing
the future), so that shouldn't be a problem.  Note that there's no danger of
reentrancy from the client code which is waiting on the future, because that
code is deferred to a microtask, so it won't be executed until after the loops
complete.

I've updated some comments to clarify that evaluate() (and the closure it
invokes) should be side effect free.

https://codereview.chromium.org/800723002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/engine.dart:1253:
_pendingFutureSources.remove(source);
On 2014/12/12 15:13:12, Brian Wilkerson wrote:
> Could the evaluation of a pending future (on line 1226) ever cause a pending
> future to be added to a source *after* the source had already been added to
> sourcesToRemove?

No, for the same reasons stated above.

https://codereview.chromium.org/800723002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/engine.dart:2011: if
(sourceEntry.hasErrorState()) {
On 2014/12/12 15:13:12, Brian Wilkerson wrote:
> I think that this should be checking explicitly for an error state related to
> DartEntry.ELEMENT. If, for example, there were an exception while trying to
> compute hints, then the element would be valid but the source entry would
still
> have an error state. (And if there is an exception while trying to parse the
> file, then the element will also be marked as being in an error state.)

Fair enough.  This code is gone now as a result of Konstantin's suggestion
above, but if it ever needs to be re-introduced I'll keep this in mind.

https://codereview.chromium.org/800723002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/engine.dart:2105: if
(sourceEntry.hasErrorState()) {
On 2014/12/12 15:13:12, Brian Wilkerson wrote:
> Ditto

Done.

https://codereview.chromium.org/800723002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/engine.dart:11711: _completer.completeError(new
AnalysisNotScheduledError());
On 2014/12/12 15:13:12, Brian Wilkerson wrote:
> Should this be
> 
> try {
>   throw new AnalysisNotScheduledError();
> } catch (exception, stackTrace) {
>   _completer.completeError(exception, stackTrace);
> }
> 
> so that we always get a stack trace for debugging purposes?

Good point.  Done.

[danrubel, 2014-12-12 18:22:27]

lgtm

[Paul Berry, 2014-12-12 18:22:49]

Committed patchset #1 (id:1) manually as 42339 (presubmit successful).