Use a poor man's incremental parser to perform incremental resolution.

Use a poor man's incremental parser to perform incremental resolution.

Also turns it on in the analysis server.

Limitations:
1. It works only inside a BlockFunctionBody.
2. It does not update errors.
3. Incremental resolution fails to resolve unqualified class member invocations.

But it works in general.
It take 3-4 milliseconds to perform incremental_resolver.dart resolution.
It was multiple seconds before (see my yesterday's log).
So, it is something like 1000 times faster now :-)

And it should be now much faster to fix the limitations :-)

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

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

[Paul Berry, 2014-11-24 15:02:25]

lgtm with comments addressed, however before we commit this I'd like to better
understand the consequences of enabling it with the current limitations.

When you say "It does not update errors", does this mean that with this feature
enabled, the errors that show up in the "problems" view will be incorrect after
an incremental update, and I will have to do "reanalyze sources" to fix them? 
If so, I think this would be a great inconvenience because I would be doing
"reanalyze sources" all the time, and that takes even longer than waiting for
non-incremental analysis.

Similarly, when you say "Incremental resolution fails to resolve unqualified
class member invocations", does this mean that the resolution will be wrong
after an incremental update, until I do "reanalyze sources"?

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

https://codereview.chromium.org/753803002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/incremental_resolver.dart:61:
lastPair.b.setNext(firstPair.a.next);
I think this should be:

  lastPair.b.setNext(lastPair.a.next);

https://codereview.chromium.org/753803002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/incremental_resolver.dart:79: }
Can we report the exception using the "server.error" notification?  I don't like
silently ignoring exceptions because it means they don't get noticed and fixed.

https://codereview.chromium.org/753803002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/incremental_resolver.dart:102: final Token a;
It looks like all uses of _TokenPair use "a" to represent the "before" state and
"b" to represent the "after" state.  Can we rename these fields to "before" and
"after"?

https://codereview.chromium.org/753803002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/incremental_resolver.dart:108: _TokenPair
_findFirstDifferentToken(Token a, Token b) {
The same rename would be nice here too (and also in _findLastDifferenceToken)

https://codereview.chromium.org/753803002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/incremental_resolver.dart:109: //  print('first
------------');
Commented out debug code should be removed (and also in
_findLastDifferenceToken)

[Brian Wilkerson, 2014-11-24 15:09:14]

I have concerns about enabling this before everything is fully functional unless
the results will always be discarded and full resolution will be performed. If
we could catch the cases where we would be left with invalid state (such as when
there is an error that we didn't add or remove), then that would be different,
but I think it's better to just finish the work than to add stop-gap tests.

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

https://codereview.chromium.org/753803002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/engine.dart:2923:
!_tryPoorMansIncrementalResolution(source, contents)) {
I have concerns about doing this much work while handling a content changed
notification. Let's talk off-line.

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

https://codereview.chromium.org/753803002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/incremental_resolver.dart:38: List<AstNode>
oldParents = _getParents(oldNode);
Why compute a list? Why not just walk up both parent chains simultaneously? It
would be faster and create fewer temporary objects.

https://codereview.chromium.org/753803002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/incremental_resolver.dart:116: if (a.type ==
TokenType.EOF) {
Why not make this part of the while loop's condition (along with "b.type !=
TokenType.EOF")?

https://codereview.chromium.org/753803002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/incremental_resolver.dart:138: }
For what it's worth, I would find the code easier to read if there were an
explicit return of null rather than the implicit return.

https://codereview.chromium.org/753803002/diff/1/pkg/analyzer/test/generated/...
File pkg/analyzer/test/generated/incremental_resolver_test.dart (right):

https://codereview.chromium.org/753803002/diff/1/pkg/analyzer/test/generated/...
pkg/analyzer/test/generated/incremental_resolver_test.dart:1370: class
PoorMansIncrementalResolutionTest extends ResolverTestCase {
It would be good to test some cases where incremental resolution isn't expected
to work, such as changing the parameters to a function, adding a new top-level
function/variable, new directives, etc.

[scheglov, 2014-11-24 18:58:45]

Committed patchset #2 (id:20001) manually as 41937 (presubmit successful).

[scheglov, 2014-11-24 18:59:06]

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

https://codereview.chromium.org/753803002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/engine.dart:2923:
!_tryPoorMansIncrementalResolution(source, contents)) {
On 2014/11/24 15:09:13, Brian Wilkerson wrote:
> I have concerns about doing this much work while handling a content changed
> notification. Let's talk off-line.

As discussed, file size in target applications are not too large, and
non-incremental scanning and parsing is fast enough.
So, we can do it here for now.

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

https://codereview.chromium.org/753803002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/incremental_resolver.dart:61:
lastPair.b.setNext(firstPair.a.next);
On 2014/11/24 15:02:25, Paul Berry wrote:
> I think this should be:
> 
>   lastPair.b.setNext(lastPair.a.next);

Done.

https://codereview.chromium.org/753803002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/incremental_resolver.dart:79: }
On 2014/11/24 15:02:25, Paul Berry wrote:
> Can we report the exception using the "server.error" notification?  I don't
like
> silently ignoring exceptions because it means they don't get noticed and
fixed.

We're in a wrong project to do this.
I'll add TODO to add logging later.

https://codereview.chromium.org/753803002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/incremental_resolver.dart:102: final Token a;
On 2014/11/24 15:02:25, Paul Berry wrote:
> It looks like all uses of _TokenPair use "a" to represent the "before" state
and
> "b" to represent the "after" state.  Can we rename these fields to "before"
and
> "after"?

Done.

https://codereview.chromium.org/753803002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/incremental_resolver.dart:108: _TokenPair
_findFirstDifferentToken(Token a, Token b) {
On 2014/11/24 15:02:25, Paul Berry wrote:
> The same rename would be nice here too (and also in _findLastDifferenceToken)

Done.

https://codereview.chromium.org/753803002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/incremental_resolver.dart:109: //  print('first
------------');
On 2014/11/24 15:02:25, Paul Berry wrote:
> Commented out debug code should be removed (and also in
> _findLastDifferenceToken)

I'd prefer to keep it in for some time and remove after the implementation is
stable enough.

https://codereview.chromium.org/753803002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/incremental_resolver.dart:116: if (a.type ==
TokenType.EOF) {
On 2014/11/24 15:09:13, Brian Wilkerson wrote:
> Why not make this part of the while loop's condition (along with "b.type !=
> TokenType.EOF")?

Done.

https://codereview.chromium.org/753803002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/incremental_resolver.dart:138: }
On 2014/11/24 15:09:14, Brian Wilkerson wrote:
> For what it's worth, I would find the code easier to read if there were an
> explicit return of null rather than the implicit return.

Done.

https://codereview.chromium.org/753803002/diff/1/pkg/analyzer/test/generated/...
File pkg/analyzer/test/generated/incremental_resolver_test.dart (right):

https://codereview.chromium.org/753803002/diff/1/pkg/analyzer/test/generated/...
pkg/analyzer/test/generated/incremental_resolver_test.dart:1370: class
PoorMansIncrementalResolutionTest extends ResolverTestCase {
On 2014/11/24 15:09:14, Brian Wilkerson wrote:
> It would be good to test some cases where incremental resolution isn't
expected
> to work, such as changing the parameters to a function, adding a new top-level
> function/variable, new directives, etc.

Agree, but not in this CL.