Keep known valid Dart results and invalidate all the other results during incremental resolution.

Keep known valid Dart results and invalidate all the other results during incremental resolution.

By doing this we invalidate Angular results, so cause their recomputing and
updating Angular specific errors and resolved ranges.

R=brianwilkerson@google.com
BUG=

Committed: https://github.com/dart-lang/sdk/commit/57958d76433c9192b08428387ce89576571719c3

[Brian Wilkerson, 2015-09-10 20:06:52]

I don't understand the need for this change. I thought we had implemented a
general dependency tracking mechanism so that if some value that an Angular
result depended on was invalidated then the Angular result would automatically
be invalidated. Is that not working? Is the incremental resolver not
invalidating the result before updating it? Or am I missing something?

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

https://codereview.chromium.org/1331223003/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/incremental_resolver.dart:889: false) {
???

[scheglov, 2015-09-10 20:22:56]

On 2015/09/10 20:06:52, Brian Wilkerson wrote:
> I don't understand the need for this change. I thought we had implemented a
> general dependency tracking mechanism so that if some value that an Angular
> result depended on was invalidated then the Angular result would automatically
> be invalidated. Is that not working? Is the incremental resolver not
> invalidating the result before updating it? Or am I missing something?

  Yes, the incremental resolver does not invalidate results.
  It updates them in place.

  We don't want to invalidate all of the Dart results.
  For example invalidating COMPILATION_UNIT_ELEMENT would invalidate almost all
the Dart results, which we know we can keep.

  OTOH, there might be some contributed tasks that get some portion of
COMPILATION_UNIT_ELEMENT, copy its data, e.g. offsets, and handle somehow.
  The incremental resolver cannot know about these results, on what they depend,
so does not know whether some of them are safe to keep or they should be
invalidated.

  So, we need a way to keep some results and invalidate others.
  Using Delta is a way to achieve this.

[Brian Wilkerson, 2015-09-10 21:31:08]

> We don't want to invalidate all of the Dart results.
> For example invalidating COMPILATION_UNIT_ELEMENT would invalidate almost all
> the Dart results, which we know we can keep.

Here's another way to think of this: The incremental resolver should be marking
all of the dependencies as being INVALID, then updating a subset of results (the
ones it knows how to update), then marking the subset as VALID by setting them
to the updated values. We're short-cutting this process today by not marking
anything as INVALID, but that shortcut doesn't work when we don't know all of
the possible results ahead of time.

It seems like the approach in this CL has three drawbacks:

(1) If we add a new result to a "known" task, but forget to update the
incremental resolver to update the value of the result, we will keep outdated
data in the cache (because we're using the task rather than the specific
ResultDescriptor to identify results that were updated by the incremental
resolver).

(2) If there is a result associated with a target other than the
LibrarySpecificUnit being incrementally resolved, and that result is dependent
on the portion of the unit that was incrementally resolved, then I don't think
it will get marked as being invalid.

(3) If there is a result associated with the target that isn't produced by any
of those tasks and that isn't dependent on the resolved unit, I think it will
unnecessarily be marked as being invalid.

It seems like it would be less error prone to just implement the algorithm above
without the optimization.

[Brian Wilkerson, 2015-09-11 14:18:59]

Ok, after an out-of-band conversation, I'm still concerned about this approach,
but I agree that the issue is much bigger than can be solved immediately and
that this change doesn't make things any worse, so LGTM.

[scheglov, 2015-09-11 14:59:27]

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