Add ContentCache.getExists() and use it in AnalysisContextImpl.exists().

Add ContentCache.getExists() and use it in AnalysisContextImpl.exists().

This allows us to avoid direct Source IO and be able to make analysis
in an AnalysisContext hermetic.

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

Committed: https://github.com/dart-lang/sdk/commit/9619a9f1570988ef6774658e627c9119331a336e

[Brian Wilkerson, 2017-01-04 18:05:10]

lgtm

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

https://codereview.chromium.org/2610063004/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/source.dart:76: return
_contentMap.containsKey(source.fullName) ? true : null;
Did you intend to return `null` rather than `false`?

[Paul Berry, 2017-01-04 18:28:58]

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

https://codereview.chromium.org/2610063004/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/context.dart:814: if (overriddenExists != null) {
Assuming you accept my proposal below, I think lines 813 through 817 can be
replaced with:

return _contentCache.getExists(source) || source.exists();

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

https://codereview.chromium.org/2610063004/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/source.dart:70: * or `null` if this cache does
not override existence of the source.
It looks like you are trying to plan ahead to a possible future where we might
want the content cache to be able to represent a deletion (i.e. the source
actually exists on disk but the content cache wants to pretend it doesn't).  But
this possible future doesn't exist yet (which is why the code below always
returns either `true` or `null`.

I find this really confusing, and it's hard to test well because we can't easily
write unit tests to exercise a feature that doesn't exist yet.

My recommendation: change this code so that it returns `true` if the given
source is overridden in the content cache, and `false` if it is not overridden. 
Then the implementation just becomes `return
_contentMap.containsKey(source.fullName);` and the caller can be changed the way
I mentioned earlier.

[scheglov, 2017-01-04 20:07:27]

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

https://codereview.chromium.org/2610063004/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/source.dart:70: * or `null` if this cache does
not override existence of the source.
On 2017/01/04 18:28:57, Paul Berry wrote:
> It looks like you are trying to plan ahead to a possible future where we might
> want the content cache to be able to represent a deletion (i.e. the source
> actually exists on disk but the content cache wants to pretend it doesn't). 
But
> this possible future doesn't exist yet (which is why the code below always
> returns either `true` or `null`.
> 
> I find this really confusing, and it's hard to test well because we can't
easily
> write unit tests to exercise a feature that doesn't exist yet.
> 
> My recommendation: change this code so that it returns `true` if the given
> source is overridden in the content cache, and `false` if it is not
overridden. 
> Then the implementation just becomes `return
> _contentMap.containsKey(source.fullName);` and the caller can be changed the
way
> I mentioned earlier.

I'm trying to extend ContentCache to fully control AnalysisContext's view of the
world.
So, we can provide a view of the world in the new analysis driver.
We don't want an AnalysisContext to talk to any Source directly.
So, we cannot do contentCache.getExists() || source.exists().

We already have this model for getContents() - it returns a value, or null as "I
don't know".

[scheglov, 2017-01-04 20:31:51]

Description was changed from

==========
Add ContentCache.getExists() and use it in AnalysisContextImpl.exists().

This allows us to avoid direct Source IO and be able to make analysis
in an AnalysisContext hermetic.

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

to

==========
Add ContentCache.getExists() and use it in AnalysisContextImpl.exists().

This allows us to avoid direct Source IO and be able to make analysis
in an AnalysisContext hermetic.

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

Committed:
https://github.com/dart-lang/sdk/commit/9619a9f1570988ef6774658e627c9119331a336e
==========

[scheglov, 2017-01-04 20:31:52]

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

[Paul Berry, 2017-01-04 20:33:01]

On 2017/01/04 20:07:27, scheglov wrote:
>
https://codereview.chromium.org/2610063004/diff/1/pkg/analyzer/lib/src/genera...
> File pkg/analyzer/lib/src/generated/source.dart (right):
> 
>
https://codereview.chromium.org/2610063004/diff/1/pkg/analyzer/lib/src/genera...
> pkg/analyzer/lib/src/generated/source.dart:70: * or `null` if this cache does
> not override existence of the source.
> On 2017/01/04 18:28:57, Paul Berry wrote:
> > It looks like you are trying to plan ahead to a possible future where we
might
> > want the content cache to be able to represent a deletion (i.e. the source
> > actually exists on disk but the content cache wants to pretend it doesn't). 
> But
> > this possible future doesn't exist yet (which is why the code below always
> > returns either `true` or `null`.
> > 
> > I find this really confusing, and it's hard to test well because we can't
> easily
> > write unit tests to exercise a feature that doesn't exist yet.
> > 
> > My recommendation: change this code so that it returns `true` if the given
> > source is overridden in the content cache, and `false` if it is not
> overridden. 
> > Then the implementation just becomes `return
> > _contentMap.containsKey(source.fullName);` and the caller can be changed the
> way
> > I mentioned earlier.
> 
> I'm trying to extend ContentCache to fully control AnalysisContext's view of
the
> world.
> So, we can provide a view of the world in the new analysis driver.
> We don't want an AnalysisContext to talk to any Source directly.
> So, we cannot do contentCache.getExists() || source.exists().
> 
> We already have this model for getContents() - it returns a value, or null as
"I
> don't know".

Ok, I see.  I'm not thrilled with the idea of using `null` to turn a boolean
into a tri-state, but I see how it's nice to have a parallel between
contentCache.getExists() and contentCache.getContents().

But since it sounds like you're going to follow up soon with a change to the
content cache where most of the time (always?) it returns either `true` or
`false` I'm a lot less worried.

lgtm.