Add hook for listening to implicitly analyzed files

Add hook for listening to implicitly analyzed files

R=paulberry@google.com

Committed: https://github.com/dart-lang/sdk/commit/503e2b68f1768cd8cb13cf1facaaa6c0afde1d5a

[Brian Wilkerson, 2015-07-15 18:28:45]

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

[Paul Berry, 2015-07-15 19:50:01]

I think there's a corner case we're not addressing: what if a file is being
implicitly analyzed, and then it gets explicitly added?  I think we should
either (a) send back a notification saying the file is no longer being
implicitly analyzed, or (b) document that no notification is expected in this
case.  I think my preference would be for (a).

Similarly, if a file is being explicitly analyzed, and the client requests to
stop explicitly analyzing it, but it is still reachable from other files (and
hence will now be implicitly analyzed), we should either (a) send a notification
saying the file is now being implicitly analyzed, or (b) document that no
notification is expected in this case.  For this, I have an even stronger
preference for (a), because if we do (b) there's no way for the client to know
that the file is still being analyzed.

The other comments (below) are nit picks.

https://codereview.chromium.org/1239863002/diff/1/pkg/analyzer/lib/src/contex...
File pkg/analyzer/lib/src/context/cache.dart (right):

https://codereview.chromium.org/1239863002/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/cache.dart:219: * return the entry associated with
the target.
Let's also document the fact that we return `null` if there was no cache entry
for the target.

https://codereview.chromium.org/1239863002/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/cache.dart:901: * and return the entry associated
with the target.
Similar comment here.

https://codereview.chromium.org/1239863002/diff/1/pkg/analyzer/lib/src/contex...
File pkg/analyzer/lib/src/context/context.dart (right):

https://codereview.chromium.org/1239863002/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/context.dart:173:
StreamController<AnalyzedSourcesEvent> _analyzedSourcesController;
How about naming this "_implicitlyAnalyzedSourcesController", just so that
there's no confusion about explicit vs. implicit sources.

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

https://codereview.chromium.org/1239863002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/engine.dart:212: * return the entry associated
with the source.
Document when we would return `null` here too.

https://codereview.chromium.org/1239863002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/engine.dart:349: Stream<AnalyzedSourcesEvent> get
analyzedSources;
Similarly, consider renaming this to "implicitlyAnalyzedSources".

https://codereview.chromium.org/1239863002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/engine.dart:6814: * and return the entry
associated with the source.
Document when we might return `null`.

[Brian Wilkerson, 2015-07-15 21:23:37]

> ... what if a file is being implicitly analyzed, and then it gets
> explicitly added?  I think we should either (a) send back a
> notification saying the file is no longer being implicitly analyzed,
> or (b) ...  I think my preference would be for (a).
>
> Similarly, if a file is being explicitly analyzed, and the client
> requests to stop explicitly analyzing it, but it is still reachable
> from other files (and hence will now be implicitly analyzed), we
> should either (a) send a notification saying the file is now being
> implicitly analyzed, or (b) ...  For this, I have an even stronger
> preference for (a) ...

In both cases, I also like (a) better. I think I missed these cases because we
also don't update the cache entry to mark it as being explicit or implicit when
it changes in this way.

I'm not sure how often these cases will occur in practice, though, so I'm not
sure how important it is to fix them before committing. Thoughts?

https://codereview.chromium.org/1239863002/diff/1/pkg/analyzer/lib/src/contex...
File pkg/analyzer/lib/src/context/cache.dart (right):

https://codereview.chromium.org/1239863002/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/cache.dart:219: * return the entry associated with
the target.
Done

https://codereview.chromium.org/1239863002/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/cache.dart:901: * and return the entry associated
with the target.
Done

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

https://codereview.chromium.org/1239863002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/engine.dart:212: * return the entry associated
with the source.
Done

https://codereview.chromium.org/1239863002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/engine.dart:349: Stream<AnalyzedSourcesEvent> get
analyzedSources;
> Similarly, consider renaming this to "implicitlyAnalyzedSources".

I struggled a bit with the name. I suppose what you're proposing is a little
better than what I ended up with, but
(a) both sound like they should be returning a List<Source> rather than a
Stream, and
(b) the real name of this getter probably wants to be something like
onImplicitlyAnalyzedSourcesChanged, which is a bit long even for me.

Interested to hear what you think.

https://codereview.chromium.org/1239863002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/engine.dart:6814: * and return the entry
associated with the source.
Done

[Paul Berry, 2015-07-15 21:41:57]

On 2015/07/15 21:23:37, Brian Wilkerson wrote:
> > ... what if a file is being implicitly analyzed, and then it gets
> > explicitly added?  I think we should either (a) send back a
> > notification saying the file is no longer being implicitly analyzed,
> > or (b) ...  I think my preference would be for (a).
> >
> > Similarly, if a file is being explicitly analyzed, and the client
> > requests to stop explicitly analyzing it, but it is still reachable
> > from other files (and hence will now be implicitly analyzed), we
> > should either (a) send a notification saying the file is now being
> > implicitly analyzed, or (b) ...  For this, I have an even stronger
> > preference for (a) ...
> 
> In both cases, I also like (a) better. I think I missed these cases because we
> also don't update the cache entry to mark it as being explicit or implicit
when
> it changes in this way.
> 
> I'm not sure how often these cases will occur in practice, though, so I'm not
> sure how important it is to fix them before committing. Thoughts?

Agreed.  Let's put a TODO comment in (and maybe file a ticket?) and get on with
things.

> 
>
https://codereview.chromium.org/1239863002/diff/1/pkg/analyzer/lib/src/contex...
> File pkg/analyzer/lib/src/context/cache.dart (right):
> 
>
https://codereview.chromium.org/1239863002/diff/1/pkg/analyzer/lib/src/contex...
> pkg/analyzer/lib/src/context/cache.dart:219: * return the entry associated
with
> the target.
> Done
> 
>
https://codereview.chromium.org/1239863002/diff/1/pkg/analyzer/lib/src/contex...
> pkg/analyzer/lib/src/context/cache.dart:901: * and return the entry associated
> with the target.
> Done
> 
>
https://codereview.chromium.org/1239863002/diff/1/pkg/analyzer/lib/src/genera...
> File pkg/analyzer/lib/src/generated/engine.dart (right):
> 
>
https://codereview.chromium.org/1239863002/diff/1/pkg/analyzer/lib/src/genera...
> pkg/analyzer/lib/src/generated/engine.dart:212: * return the entry associated
> with the source.
> Done
> 
>
https://codereview.chromium.org/1239863002/diff/1/pkg/analyzer/lib/src/genera...
> pkg/analyzer/lib/src/generated/engine.dart:349: Stream<AnalyzedSourcesEvent>
get
> analyzedSources;
> > Similarly, consider renaming this to "implicitlyAnalyzedSources".
> 
> I struggled a bit with the name. I suppose what you're proposing is a little
> better than what I ended up with, but
> (a) both sound like they should be returning a List<Source> rather than a
> Stream, and
> (b) the real name of this getter probably wants to be something like
> onImplicitlyAnalyzedSourcesChanged, which is a bit long even for me.
> 
> Interested to hear what you think.

Good point.  It occurs to me that the names that make the most sense to me for
streams are plural nouns which make clear that we are talking about a stream of
notifications about some underlying objects rather than the underlying objects
themselves (e.g. Watcher.events makes it clear that the client should expect a
stream of events; Watcher.filesChanged would have been confusing because a
client might expect to receive a stream of files).  Accordingly, maybe we should
call this something like "implicitlyAnalyzedSourceEvents".

Which makes me think of one other thing: maybe we should rename
AnalyzedSourcesEvent to AnalyzedSourceEvent, since "AnalyzedSourcesEvent" makes
it sound like there's a one-to-many relationship between the event and the
sources; in point of fact it's a one-to-one relationship.

I don't want to bikeshed about this more than necessary, though.  LGTM whatever
you decide :)

> 
>
https://codereview.chromium.org/1239863002/diff/1/pkg/analyzer/lib/src/genera...
> pkg/analyzer/lib/src/generated/engine.dart:6814: * and return the entry
> associated with the source.
> Done

[Brian Wilkerson, 2015-07-16 15:10:44]

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

[scheglov, 2015-07-17 18:21:38]

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

[scheglov, 2015-07-17 18:21:39]

https://codereview.chromium.org/1239863002/diff/20001/pkg/analyzer/lib/src/co...
File pkg/analyzer/lib/src/context/context.dart (right):

https://codereview.chromium.org/1239863002/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/context/context.dart:1662: if (entry != null &&
entry.explicitlyAdded) {
Should it be NOT entry.explicitlyAdded?

[Brian Wilkerson, 2015-07-17 19:25:38]

https://codereview.chromium.org/1239863002/diff/20001/pkg/analyzer/lib/src/co...
File pkg/analyzer/lib/src/context/context.dart (right):

https://codereview.chromium.org/1239863002/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/context/context.dart:1662: if (entry != null &&
entry.explicitlyAdded) {
Yes, thanks.