Avoid redundant notifications when a file is analyzed in multiple contexts.

Avoid redundant notifications when a file is analyzed in multiple contexts.

BUG=dartbug.com/22210
R=danrubel@google.com

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

[Paul Berry, 2015-01-29 21:26:50]

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

[danrubel, 2015-01-29 21:37:00]

lgtm

https://codereview.chromium.org/887793003/diff/1/pkg/analysis_server/test/ana...
File pkg/analysis_server/test/analysis_server_test.dart (right):

https://codereview.chromium.org/887793003/diff/1/pkg/analysis_server/test/ana...
pkg/analysis_server/test/analysis_server_test.dart:265: // TODO(paulberry): why?
Do we send an outline once when it is parsed and again when it is resolved?

[Paul Berry, 2015-01-29 21:42:56]

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

[Brian Wilkerson, 2015-01-30 16:59:00]

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

[Brian Wilkerson, 2015-01-30 16:59:00]

Unless I'm misunderstanding it, I don't think this is a safe change. The values
being returned in the notifications can be different in different contexts.
Consider, for example, error messages. The same file could be getting analyzed
in two different context against two versions of an underlying package. That
could produce different results from the two contexts. I think the results from
the secondary contexts need to be merged with those from the primary context,
not be ignored.

[Paul Berry, 2015-01-30 19:11:43]

On 2015/01/30 16:59:00, Brian Wilkerson wrote:
> Unless I'm misunderstanding it, I don't think this is a safe change. The
values
> being returned in the notifications can be different in different contexts.
> Consider, for example, error messages. The same file could be getting analyzed
> in two different context against two versions of an underlying package. That
> could produce different results from the two contexts. I think the results
from
> the secondary contexts need to be merged with those from the primary context,
> not be ignored.

(Sorry in advance for the wordy reply.  I have enough to say that I would
normally prefer to just discuss it in person, but since you were concerned
enough to take time out of your vacation to make this comment, I wanted to honor
that concern by responding ASAP.)

You are right that if the same file produces different results when analyzed in
two different contexts, this change will cause the results from one context (the
one returned by getAnalysisContext()) to be favored, and the results from others
will be ignored.

As to whether this is a safe change, I think we need to consider what will
happen if we revert this CL, and what happened before revision 43163 (which
caused the regression that this CL was intended to fix).

If we revert this CL, then if a file produces different results when analyzed in
two different contexts, the results of analyzing the file in all contexts will
be reported to the client.  However, since we have the API convention that newer
results overwrite older results, the effect will only be visible to the user as
an intermittent effect while analysis is in progress.  Once analysis is
complete, the user will only see results from one context, as they do with this
CL in place; the only difference once analysis is complete is that with the CL
reverted, it is indeterminate which context they will see results from.  I don't
think that showing the alternate results briefly during analysis is helpful to
the user, because based on my own experience using editor, when I see an error
or warning appear only briefly during analysis, I tend to assume it is bogus. 
So I believe that reverting this CL would make things worse for the user.

Before 43163, if a file produced different results when analyzed in two
different contexts, then while typing the user would only see the results from
the context returned by getAnalysisContext.  The reason for that was that prior
to 43163, we had a bug where overlays and filesystem changes* only got applied
to that one context (the one returned by getAnalysisContext) rather than all of
them.  So modifying a file would only trigger reanalysis of one context; errors
and warnings would not even be computed for any other context.  From the user's
point of view this is the same effect as what we have with this CL applied.  So
I don't think this CL is introducing a new bug--it is improving things to the
state we were in prior to 43163; unfortunately that state is not ideal.

So I think the CL should be left as is, and we should consider the issue of
different analysis contexts producing different results to be a separate bug
that we address later.  As to how we address that bug, I'm intrigued by your
suggestion to merge the results from all the contexts, and I would like to
discuss it further once you're back.  For now, I will add a bug to the issue
tracker.

Let me know if you think my reasoning is wrong here--I definitely want to revert
this CL if it is causing problems!

(*I realized while typing this up that although revision 43163 completely fixes
the problem of overlays getting only applied to one context, it only partially
fixes problem of filesystem changes getting only applied to one context.  I'll
write up a separate bug report for that.)

[Brian Wilkerson, 2015-01-30 19:55:23]

That sounds reasonable to me. Thanks!