Update SOURCE_KIND when a missing source file appears

Update SOURCE_KIND when a missing source file appears

If the source kind isn't updated, we get a bogus error:
The imported library ... must not have a part-of directive.
This error is calculated in BuildDirectiveElementsTask.
based on SOURCE_KIND.

SOURCE_KIND is based on whether MODIFICATION_DATE is -1,
so make sure we invalidate MODIFICATION_DATE.

Also, fix an issue where _sourceChanged would have
no effect if incremental resolution isn't run, because
the content has already been updated and is therefore
equal.

Also, remove unused RelativeFileUriResolver and
clarify comments around missing Source handling.

BUG=https://github.com/dart-lang/sdk/issues/25808
R=brianwilkerson@google.com

Committed: https://github.com/dart-lang/sdk/commit/41ce555c2e1c8239ed460f2471f7eb2082f6fd48

[skybrian, 2016-02-26 06:44:45]

skybrian@google.com changed reviewers:
+ brianwilkerson@google.com, scheglov@google.com

[skybrian, 2016-02-26 06:44:46]

This change can't be committed yet because four tests in context_test.dart fail.
However, the tests are complicated and I don't understand what broke. Any ideas?

[Brian Wilkerson, 2016-02-26 15:07:32]

> This change can't be committed yet because four tests in
> context_test.dart fail. However, the tests are complicated
> and I don't understand what broke. Any ideas?

Which tests are broken?

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

https://codereview.chromium.org/1737693004/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/context.dart:1021: // Disable it since we know the
file changed.
Why is the new content in the cache?

I'm guessing that _tryPoorMansIncrementalResolution must do that as a
side-effect, but it seems like there shouldn't be any side-effect if it returns
false. If that's the case, then we should fix _tryPoorMansIncrementalResolution,
which would negate the need for the extra argument to _sourceChanged.

https://codereview.chromium.org/1737693004/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/context.dart:1823:
entry.setState(MODIFICATION_TIME, CacheState.INVALID);
If we're trying to ensure that SOURCE_KIND is recalculated, shouldn't we set its
state to INVALID?

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

https://codereview.chromium.org/1737693004/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/source.dart:264: * Returns null if no resolver can
handle the URI. If the source file is
nit: "Returns null" --> "Return `null`"

https://codereview.chromium.org/1737693004/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/source.dart:265: * missing, returns a non-null
Source that will have a negative
nit: "returns" --> "return"

https://codereview.chromium.org/1737693004/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/source_io.dart (left):

https://codereview.chromium.org/1737693004/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/source_io.dart:501: class RelativeFileUriResolver
extends UriResolver {
Removing this class is technically a breaking change. Please restore it and mark
it as being @deprecated if it should be removed as part of the next set of
breaking changes.

https://codereview.chromium.org/1737693004/diff/1/pkg/analyzer/test/generated...
File pkg/analyzer/test/generated/compile_time_error_code_test.dart (right):

https://codereview.chromium.org/1737693004/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/compile_time_error_code_test.dart:6056:
expect(target.modificationStamp, -1);
I'm not 100% certain, but given that most of our tests never create any files on
disk, this might not test what you want. It might be better to use the public
API and ask the context for the modification stamp.

https://codereview.chromium.org/1737693004/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/compile_time_error_code_test.dart:6057:
changeSource(target, "");
When a file that didn't exist is created on disk, do we really use changeSource?
I would have expected it to use addSource. If so, this might be the underlying
problem.

[skybrian, 2016-02-26 17:22:06]

Test failures:

FAIL: LimitedInvalidateTest | test_noChange_thenChange
  cache state of /b.dart LIBRARY_ELEMENT: wanted INVALID, got: VALID
  package:unittest/src/simple_configuration.dart 128:34 
SimpleConfiguration.onExpectFailure
  package:unittest/src/simple_configuration.dart 24:13  
_ExpectFailureHandler.fail
  package:unittest/src/matcher/expect.dart 103:18        fail
  test/src/context/context_test.dart 2765:7             
LimitedInvalidateTest._assertInvalid
  test/src/context/context_test.dart 2566:5             
LimitedInvalidateTest.test_noChange_thenChange

FAIL: LimitedInvalidateTest | test_usedName_directUser
  cache state of /b.dart LIBRARY_ERRORS_READY: wanted INVALID, got: VALID
  package:unittest/src/simple_configuration.dart 128:34 
SimpleConfiguration.onExpectFailure
  package:unittest/src/simple_configuration.dart 24:13  
_ExpectFailureHandler.fail
  package:unittest/src/matcher/expect.dart 103:18        fail
  test/src/context/context_test.dart 2765:7             
LimitedInvalidateTest._assertInvalid
  test/src/context/context_test.dart 2646:5             
LimitedInvalidateTest.test_usedName_directUser

FAIL: LimitedInvalidateTest | test_usedName_directUser_withIncremental
  cache state of /b.dart LIBRARY_ERRORS_READY: wanted INVALID, got: VALID
  package:unittest/src/simple_configuration.dart 128:34 
SimpleConfiguration.onExpectFailure
  package:unittest/src/simple_configuration.dart 24:13  
_ExpectFailureHandler.fail
  package:unittest/src/matcher/expect.dart 103:18        fail
  test/src/context/context_test.dart 2765:7             
LimitedInvalidateTest._assertInvalid
  test/src/context/context_test.dart 2696:5             
LimitedInvalidateTest.test_usedName_directUser_withIncremental

FAIL: LimitedInvalidateTest | test_usedName_indirectUser
  cache state of /b.dart LIBRARY_ERRORS_READY: wanted INVALID, got: VALID
  package:unittest/src/simple_configuration.dart 128:34 
SimpleConfiguration.onExpectFailure
  package:unittest/src/simple_configuration.dart 24:13  
_ExpectFailureHandler.fail
  package:unittest/src/matcher/expect.dart 103:18        fail
  test/src/context/context_test.dart 2765:7             
LimitedInvalidateTest._assertInvalid
  test/src/context/context_test.dart 2739:5             
LimitedInvalidateTest.test_usedName_indirectUser

[Brian Wilkerson, 2016-02-26 17:59:37]

I can't tell for sure just by looking at the code, but the clue I'd use to start
investigating is this comment in test_noChange_thenChange:

    // Update a.dart ... invalidates b.dart, so
    // we know that the previous update did not damage dependencies.

I suspect that something you changed caused one of two problems: either the
dependencies are now damaged so that b.dart's element didn't get invalidated, or
the method that causes the dependencies to be used to invalidate the library
element is no longer being invoked.

I'd probably start by checking out master, adding a breakpoint in the code where
a cache value is invalidated, and then running one test (using solo) to figure
out how we got to where we're invalidating the element without your changes.
Then, if necessary, switch back to your branch and step through that same call
stack until the behavior diverges.

[skybrian, 2016-02-26 18:09:01]

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

https://codereview.chromium.org/1737693004/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/context.dart:1021: // Disable it since we know the
file changed.
On 2016/02/26 15:07:32, Brian Wilkerson wrote:
> Why is the new content in the cache?

Within the analyzer, the only caller of handleContentsChanged is this method:

  void _contentsChanged(Source source, String contents, bool notify) {
    String originalContents = _contentCache.setContents(source, contents);
    handleContentsChanged(source, originalContents, contents, notify);
  }

The analysis server also calls handleContentsChanged directly. (See below.)

In either case, the content cache has already been updated.

https://codereview.chromium.org/1737693004/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/context.dart:1823:
entry.setState(MODIFICATION_TIME, CacheState.INVALID);
On 2016/02/26 15:07:32, Brian Wilkerson wrote:
> If we're trying to ensure that SOURCE_KIND is recalculated, shouldn't we set
its
> state to INVALID?

I thought it would be less brittle this way. Since MODIFICATION_TIME has
changed, any derived data that depends on it (including SOURCE_KIND but perhaps
other things) should be rebuilt.

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

https://codereview.chromium.org/1737693004/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/source.dart:264: * Returns null if no resolver can
handle the URI. If the source file is
On 2016/02/26 15:07:32, Brian Wilkerson wrote:
> nit: "Returns null" --> "Return `null`"

Okay, but FYI this is the opposite of normal dartdoc style:
https://www.dartlang.org/effective-dart/documentation/#prefer-starting-functi...

https://codereview.chromium.org/1737693004/diff/1/pkg/analyzer/test/generated...
File pkg/analyzer/test/generated/compile_time_error_code_test.dart (right):

https://codereview.chromium.org/1737693004/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/compile_time_error_code_test.dart:6057:
changeSource(target, "");
On 2016/02/26 15:07:32, Brian Wilkerson wrote:
> When a file that didn't exist is created on disk, do we really use
changeSource?
> I would have expected it to use addSource. If so, this might be the underlying
> problem.

I'm testing the case when the file hasn't been saved to disk, but we are
creating an overlay that's the target of a preexisting import statement. I
wanted to do this approximately same way as the analysis server, so it executes
the same code.

In AnalysisServer.updateContent(), first it updates the content cache directly:
String oldContents = overlayState.getContents(source);
overlayState.setContents(source, newContents);

Then for each context it will call:
List<Source> sources = context.getSourcesWithFullName(file);

A Source will exist (possibly with modificationStamp of -1) in that context if
there's any import, export, or part statement that refers to it.

Then for each Source:
context.handleContentsChanged(source, oldContents, newContents, true);

If the file didn't previously exist then I believe oldContents will be null.

[Brian Wilkerson, 2016-02-26 18:35:57]

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

https://codereview.chromium.org/1737693004/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/context.dart:1823:
entry.setState(MODIFICATION_TIME, CacheState.INVALID);
It probably doesn't matter here, but in general I'd opt for invalidating as
little as necessary in order to improve performance. But then it might also be
the case that the modification time needs to be invalidated anyway.

In either case, it's worth being explicit, because then if the dependency
structure between results changes this code won't break. So I'd at least add

  entry.setState(SOURCE_KIND, CacheState.INVALID);

even if we don't stop setting the state of the modification time.

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

https://codereview.chromium.org/1737693004/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/source.dart:264: * Returns null if no resolver can
handle the URI. If the source file is
I'm aware, but it's consistent with most of the rest of the dartdocs in analyzer
and analysis server.

https://codereview.chromium.org/1737693004/diff/1/pkg/analyzer/test/generated...
File pkg/analyzer/test/generated/compile_time_error_code_test.dart (right):

https://codereview.chromium.org/1737693004/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/compile_time_error_code_test.dart:6057:
changeSource(target, "");
Ok. I just didn't realize that we had clients that were creating in-memory
overrides of files that don't exist on disk.

[skybrian, 2016-02-26 19:54:17]

It appears that this call has unexpected side effects:

  entry.setState(CONTENT, CacheState.FLUSHED)

Skipping the flush causes the test to fail, even though later in the same method
we will invalidate it:

  entry.setState(CONTENT, CacheState.INVALID)

[Brian Wilkerson, 2016-02-26 20:30:08]

On 2016/02/26 19:54:17, skybrian wrote:
> It appears that this call has unexpected side effects:
> 
>   entry.setState(CONTENT, CacheState.FLUSHED)
> 
> Skipping the flush causes the test to fail, even though later in the same
method
> we will invalidate it:
> 
>   entry.setState(CONTENT, CacheState.INVALID)

I have no context for this comment. I don't know where these invocations are or
what test you're referring to.

[skybrian, 2016-02-26 22:19:57]

On 2016/02/26 20:30:08, Brian Wilkerson wrote:
> On 2016/02/26 19:54:17, skybrian wrote:
> > It appears that this call has unexpected side effects:
> > 
> >   entry.setState(CONTENT, CacheState.FLUSHED)
> > 
> > Skipping the flush causes the test to fail, even though later in the same
> method
> > we will invalidate it:
> > 
> >   entry.setState(CONTENT, CacheState.INVALID)
> 
> I have no context for this comment. I don't know where these invocations are
or
> what test you're referring to.

Never mind, i figured it out. Please take another look - this is a smaller
change now.

[Brian Wilkerson, 2016-02-26 22:46:54]

LGTM

[skybrian, 2016-02-27 02:08:03]

Description was changed from

==========
Update SOURCE_KIND when a missing source file appears

If the source kind isn't updated, we get a bogus error:
The imported library ... must not have a part-of directive.
This error is calculated in BuildDirectiveElementsTask.
based on SOURCE_KIND.

SOURCE_KIND is based on whether MODIFICATION_DATE is -1,
so make sure we invalidate MODIFICATION_DATE.

Also, fix an issue where _sourceChanged would have
no effect if incremental resolution isn't run, because
the content has already been updated and is therefore
equal.

Also, remove unused RelativeFileUriResolver and
clarify comments around missing Source handling.

BUG=https://github.com/dart-lang/sdk/issues/25808
==========

to

==========
Update SOURCE_KIND when a missing source file appears

If the source kind isn't updated, we get a bogus error:
The imported library ... must not have a part-of directive.
This error is calculated in BuildDirectiveElementsTask.
based on SOURCE_KIND.

SOURCE_KIND is based on whether MODIFICATION_DATE is -1,
so make sure we invalidate MODIFICATION_DATE.

Also, fix an issue where _sourceChanged would have
no effect if incremental resolution isn't run, because
the content has already been updated and is therefore
equal.

Also, remove unused RelativeFileUriResolver and
clarify comments around missing Source handling.

BUG=https://github.com/dart-lang/sdk/issues/25808
R=brianwilkerson@google.com

Committed:
https://github.com/dart-lang/sdk/commit/41ce555c2e1c8239ed460f2471f7eb2082f6fd48
==========

[skybrian, 2016-02-27 02:08:05]

Committed patchset #3 (id:40001) manually as
41ce555c2e1c8239ed460f2471f7eb2082f6fd48 (presubmit successful).