Clear the element for imports and exports when the target doesn't exist.

Clear the element for imports and exports when the target doesn't exist.

This fixes a bug where the unused import hint doesn't go away after
removing the file being imported.

(Also, I suspect it fixes a memory leak.)

BUG=
R=brianwilkerson@google.com

Committed: https://github.com/dart-lang/sdk/commit/44301b8001137c16de5735f433cb6bf39d88a798

[skybrian, 2016-02-27 06:38:51]

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

[Brian Wilkerson, 2016-02-27 18:05:21]

https://codereview.chromium.org/1743523003/diff/1/pkg/analyzer/lib/src/dart/e...
File pkg/analyzer/lib/src/dart/element/builder.dart (right):

https://codereview.chromium.org/1743523003/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/builder.dart:164: node.element = null;
I don't understand why this would be necessary. DirectiveElementBuilder is only
used in the BuildDirectiveElementsTask, and at the point at which that task is
called the (Import|Export)Directive should not have an element associated with
it.

If we're getting a (partially) resolved AST when that task is being performed,
then we have probably failed much earlier when we ought to have invalidated the
AST.

My concern is that this change would only mask a different bug.

[skybrian, 2016-02-27 23:29:04]

My suspicion is that when the user changes the target of an import statement, it
invalidates the elements but not the AST (since the Dart file containing the
import statement didn't actually change). In that case, it seems like
BuildDirectiveElementsTask would get called again with the same AST as last
time, and therefore the AST contains old Elements in it that need to be cleared
away and replaced with the new ones.

[Brian Wilkerson, 2016-02-29 01:12:58]

Interesting. We need to confirm or deny your suspicion, but yes, the scenario
you postulate makes sense.

But if that's the case, then it means that we're missing a dependency between
the version of the AST in which the directives have been resolved and some
result associated with the target of the directive (hereafter T). (I'm not sure
what the most appropriate result is; we might even need to introduce a new
result such as EXISTS. The question is "what kinds of changes to T cause the AST
to be invalid?".)

If we introduce the appropriate dependency, then when T is changed (in the
appropriate way) we will invalid all results associated with T, which will
trigger the invalidation of the AST (and transitively anything that depends on
it).

If we don't, then your changes will fix one place in which we can observe the
invalid state, but leave others. For example, if navigation is requested for the
file containing the directive, it would get back the invalid AST and produce
invalid navigation information.

[skybrian, 2016-02-29 06:48:02]

As you say, this is a special case of a more general problem. But I'm a bit
surprised that this is surprising; it seems like it's inherent in the design of
the task model, so I think it must have come up before? There are other tasks
that update mutable fields. Whenever a field-mutating task gets invalidated, it
means there are some fields on some mutable objects somewhere that are
temporarily stale until the task is redone. The difference in this case is that
the new result after the task is redone should be a null instead of a pointer to
an object as is usually the case.

If we're concerned someone might accidentally read stale data during analysis,
we could treat invalidation as an undo operation. Since
BuildDirectiveElementsTask sets element pointers in the AST, maybe invalidating
LIBRARY_ELEMENT2 should undo it by setting the pointers in the AST back to null?
But that seems expensive, so perhaps documenting that the element pointer in the
AST is only valid when LIBRARY_ELEMENT2 (in this case) is valid might be enough?

[Brian Wilkerson, 2016-02-29 16:53:07]

> As you say, this is a special case of a more general problem. But I'm a bit
> surprised that this is surprising; it seems like it's inherent in the design
of
> the task model, so I think it must have come up before?

Yes, but I didn't immediately see that that was the cause of the issue. Thanks
for walking me through. I understand what's going on now.

As you say, the issue is with mutable state and the fact that this method was
assuming that we hadn't previously set the state. We have previously recognized
the principle that tasks must put mutable objects into the same state whether
they had been run before or not, but this code violated that in a subtle way.

LGTM

[skybrian, 2016-02-29 18:06:16]

Description was changed from

==========
Clear the element for imports and exports when the target doesn't exist.

This fixes a bug where the unused import hint doesn't go away after
removing the file being imported.

(Also, I suspect it fixes a memory leak.)

BUG=
==========

to

==========
Clear the element for imports and exports when the target doesn't exist.

This fixes a bug where the unused import hint doesn't go away after
removing the file being imported.

(Also, I suspect it fixes a memory leak.)

BUG=
R=brianwilkerson@google.com

Committed:
https://github.com/dart-lang/sdk/commit/44301b8001137c16de5735f433cb6bf39d88a798
==========

[skybrian, 2016-02-29 18:06:18]

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