Change all sources associated with changed files (issue 22680)

Change all sources associated with changed files (issue 22680)

R=paulberry@google.com

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

[Brian Wilkerson, 2015-03-06 21:30:00]

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

[Paul Berry, 2015-03-06 21:56:42]

lgtm as a first step.  I think there are two other cases that need to be
addressed, though:

1. In ContextManager._handleWatchEvent, when handling a REMOVE event, I believe
we need to create a ChangeSet that removes all sources associated with the file.
 Otherwise if a file is removed that is reachable by multiple URI's, some
Sources for that file will remain in the context's cache.

2. In AnalysisServer.updateContent, when processing a content change from the
client, I believe we need to call context.handleContentsChanged for all sources
associated with the file.  Otherwise if a file is edited that is reachable by
multiple URI's, we will only trigger reanalysis of the files that reach it
through one URI.

https://codereview.chromium.org/981283003/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/context_manager.dart (right):

https://codereview.chromium.org/981283003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/context_manager.dart:521: List<Source> sources =
info.context.getSourcesWithFullName(path);
I just noticed that there's a pre-existing bug here: if a file is explicitly
included in one context but implicitly referenced in another context, we'll only
send a changeSet to the context that explicitly includes the file (since that's
the only context that's watching the file).  Would you mind adding a TODO
comment to this effect?

https://codereview.chromium.org/981283003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/context_manager.dart:524: sources.forEach((Source
source) {
Nit: this is inefficient since it copies Source objects one at a time from the
"sources" list (which is about to be GC'ed) to the newly allocated
changeSet.changedSources list.

Consider replacing this with just:

changeSet.changedSources = sources;

[Brian Wilkerson, 2015-03-06 23:17:11]

PTAL

https://codereview.chromium.org/981283003/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/context_manager.dart (right):

https://codereview.chromium.org/981283003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/context_manager.dart:521: List<Source> sources =
info.context.getSourcesWithFullName(path);
Done.

https://codereview.chromium.org/981283003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/context_manager.dart:524: sources.forEach((Source
source) {
> this is inefficient ...

Yes it is. But the field is 'final', so making it more efficient would require
bigger changes.

[Paul Berry, 2015-03-09 18:05:34]

lgtm assuming changedAllSources is removed.

https://codereview.chromium.org/981283003/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/context_manager.dart (right):

https://codereview.chromium.org/981283003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/context_manager.dart:524: sources.forEach((Source
source) {
On 2015/03/06 23:17:11, Brian Wilkerson wrote:
> > this is inefficient ...
> 
> Yes it is. But the field is 'final', so making it more efficient would require
> bigger changes.

Fair enough.

https://codereview.chromium.org/981283003/diff/20001/pkg/analyzer/lib/src/gen...
File pkg/analyzer/lib/src/generated/engine.dart (right):

https://codereview.chromium.org/981283003/diff/20001/pkg/analyzer/lib/src/gen...
pkg/analyzer/lib/src/generated/engine.dart:7563: void
changedAllSources(List<Source> sources) {
Was this added by mistake?  It looks like it's not used and not implemented.

[Brian Wilkerson, 2015-03-09 18:24:46]

https://codereview.chromium.org/981283003/diff/20001/pkg/analyzer/lib/src/gen...
File pkg/analyzer/lib/src/generated/engine.dart (right):

https://codereview.chromium.org/981283003/diff/20001/pkg/analyzer/lib/src/gen...
pkg/analyzer/lib/src/generated/engine.dart:7563: void
changedAllSources(List<Source> sources) {
> Was this added by mistake?  It looks like it's not used and not implemented.

Yes. Thanks!

I added it when looking at the inefficiency in ContextManager, but realized that
this wouldn't help either.

[Brian Wilkerson, 2015-03-09 18:28:37]

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