Don't remove indexing operations on potential source changes.

Don't remove indexing operations on potential source changes.

This covers the following scenario:

1. User performs an undo operation in a save editor.

2. Editor updates the file buffer, so sends 'add overlay'.
   Server performs an incremental analysis and schedules an 'analyze' operation.

3. The 'analyze' operation runs and schedules 'index' operation.

4. Editor saves the file buffer.
   Editor sends 'remove overlay command'.
   Server removes all the scheduled notification and index operations for the file.
   Server notices that there is no real change for the file, so the is nothing to do.
   But it still reschedules notifications, because the client might have lost some information.

5. Notifications are resent, but indexing is not performed.

  So, the solution is to reschedule indexing operation.

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

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

[Brian Wilkerson, 2015-02-27 21:56:21]

LGTM, modulo the one concern.

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

https://codereview.chromium.org/958373003/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/engine.dart:1856: List<CompilationUnit>
ensureResolvedDartUnits(Source unitSource) {
I might be misreading this code, but...

It looks like we are requiring that the unit be resolved in all of its libraries
simultaneously. If the act of resolving the unit in one library causes the AST
for a different library to be flushed from the cache, this could lead to an
infinite loop. It won't be a common case, but we should guard against it.

[Paul Berry, 2015-02-27 22:05:01]

lgtm once the issue with the assertion is resolved.

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

https://codereview.chromium.org/958373003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:664:
assert(performOperationPending);
Why remove this assertion?  AFAICT this invariant should still hold, and if it
doesn't we are likely introducing a bug.

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

https://codereview.chromium.org/958373003/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/engine.dart:1892: // We cannot provide a resolved
unit right now,
s/a resolved unit/resolved units/

[scheglov, 2015-02-28 01:34:35]

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

https://codereview.chromium.org/958373003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:664:
assert(performOperationPending);
On 2015/02/27 22:05:00, Paul Berry wrote:
> Why remove this assertion?  AFAICT this invariant should still hold, and if it
> doesn't we are likely introducing a bug.

I found the problem in my test code.
And you are right, this assertion actually helper my to avoid having two
parallel Future sequences.
We cannot call performOperation() directly, we have to rely on the existing
scheduled instance and have another way to be notified after a single operation
execution.

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

https://codereview.chromium.org/958373003/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/engine.dart:1856: List<CompilationUnit>
ensureResolvedDartUnits(Source unitSource) {
On 2015/02/27 21:56:21, Brian Wilkerson wrote:
> I might be misreading this code, but...
> 
> It looks like we are requiring that the unit be resolved in all of its
libraries
> simultaneously. If the act of resolving the unit in one library causes the AST
> for a different library to be flushed from the cache, this could lead to an
> infinite loop. It won't be a common case, but we should guard against it.

We don't rerun this method.
We ask it only only - if all the resolved units are present.
If yes, we use them.
If not, we rely on the normal perform tasks cycle to provide us this these
resolved units later.

https://codereview.chromium.org/958373003/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/engine.dart:1892: // We cannot provide a resolved
unit right now,
On 2015/02/27 22:05:00, Paul Berry wrote:
> s/a resolved unit/resolved units/

Done.

[scheglov, 2015-02-28 01:35:15]

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