Introduce CompletionTarget to take the place of the Node being completed.

Introduce CompletionTarget to take the place of the Node being completed.

Instead of locating the node containing the cursor offset using a
NodeLocator, identify the edge in the parse tree which will be
displaced or modified by the completion proposal.  This reduces the
number of cases that need to be handled in the _OpTypeAstVisitor, and
avoids the need for the _OpTypeAstVisitor to consider character
offsets in nearly all cases.

R=danrubel@google.com

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

[Paul Berry, 2014-12-19 22:17:47]

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

[Paul Berry, 2014-12-19 22:17:47]

This is not quite as polished (or as thoroughly tested) as I would normally
like, but all the tests pass and I'd like to avoid accumulating merge conflicts
over the holiday break, so I'm publishing it now.  Dan, if you're planning on
working on completion in the next two weeks, I'd appreciate if you could review
this code first so I can land it and avoid having to rebase atop your changes
when I get back.

Here's what remains to be done:

1. Test the new CompletionTarget algorithm, and the childEntities getters,
directly.  (So far they are only indirectly tested via completions).

2. For AST nodes that contain separators (e.g. ArgumentList), have childEntities
include the separators.  Until we do this, we can't distinguish between
completions like "foo(bar ^, baz)" and "foo(bar, ^baz)".

[danrubel, 2014-12-20 05:22:57]

LGTM. Nice work.

https://codereview.chromium.org/820773002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/services/completion/completion_target.dart
(right):

https://codereview.chromium.org/820773002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/services/completion/completion_target.dart:40: *
target will be the edge marked "b", so the [context] is the token "as".
Should the end of this sentence be "... so the [entity] is the token "as" ?

https://codereview.chromium.org/820773002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/services/completion/completion_target.dart:65: final
AstNode context;
Since "context" already has a strong connotation and this object contains the
entity, could we call it a container? entityContainer? containingNode?

https://codereview.chromium.org/820773002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/services/completion/optype.dart (right):

https://codereview.chromium.org/820773002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/services/completion/optype.dart:419: void
visitTypeName(TypeName node) {
Should never happen? assert(false); ?
Or includeTypeNameSuggestions = true ?

https://codereview.chromium.org/820773002/diff/1/pkg/analysis_server/test/ser...
File pkg/analysis_server/test/services/completion/optype_test.dart (right):

https://codereview.chromium.org/820773002/diff/1/pkg/analysis_server/test/ser...
pkg/analysis_server/test/services/completion/optype_test.dart:604:
assertOpType();
Shouldn't type names be suggested here?

https://codereview.chromium.org/820773002/diff/1/pkg/analysis_server/test/ser...
pkg/analysis_server/test/services/completion/optype_test.dart:610:
assertOpType();
And here as well?

[Paul Berry, 2014-12-20 15:43:25]

https://codereview.chromium.org/820773002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/services/completion/completion_target.dart
(right):

https://codereview.chromium.org/820773002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/services/completion/completion_target.dart:40: *
target will be the edge marked "b", so the [context] is the token "as".
On 2014/12/20 05:22:57, danrubel wrote:
> Should the end of this sentence be "... so the [entity] is the token "as" ?

Yes, thanks.  Fixed.

https://codereview.chromium.org/820773002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/services/completion/completion_target.dart:65: final
AstNode context;
On 2014/12/20 05:22:57, danrubel wrote:
> Since "context" already has a strong connotation and this object contains the
> entity, could we call it a container? entityContainer? containingNode?

Good idea.  Renamed to "containingNode".

https://codereview.chromium.org/820773002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/services/completion/optype.dart (right):

https://codereview.chromium.org/820773002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/services/completion/optype.dart:419: void
visitTypeName(TypeName node) {
On 2014/12/20 05:22:57, danrubel wrote:
> Should never happen? assert(false); ?
> Or includeTypeNameSuggestions = true ?

Actually the only way this code can be reached is if entity is
node.typeArguments, meaning the offset is between the type name and the "<" that
starts the type arguments (e.g. "Foo ^<Bar>").  In which case there are no
reasonable completions to offer.

I've added a comment and an assertion to clarify this.

https://codereview.chromium.org/820773002/diff/1/pkg/analysis_server/test/ser...
File pkg/analysis_server/test/services/completion/optype_test.dart (right):

https://codereview.chromium.org/820773002/diff/1/pkg/analysis_server/test/ser...
pkg/analysis_server/test/services/completion/optype_test.dart:604:
assertOpType();
On 2014/12/20 05:22:57, danrubel wrote:
> Shouldn't type names be suggested here?

No, they shouldn't, because these identifiers *declare* new type parameters;
they don't refer to types declared elsewhere.  It makes sense to suggest the
names of existing types when instantiating a generic type, but not when
declaring one, because at the declaration site the user wants to use identifiers
that aren't already used elsewhere.

Perhaps the test you meant to write was something more like this?

  class tezetst<T, U> {}
  main() {
    new tezetst<String, ^List>();
  }

[Paul Berry, 2014-12-20 15:47:36]

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