step toward new completion API

step toward new completion API
- separate new CompletionContributor and DartCompletionContributor APIs
- revise new APIs and implementations to be Futures based
- revise InheritedContributor to use new Futures based contributor API
- remove unused classes

R=brianwilkerson@google.com, scheglov@google.com

Committed: https://github.com/dart-lang/sdk/commit/38cec67d032ef0fb6dff662fa4e5dcd3863db062

[danrubel, 2015-11-22 02:29:21]

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

[danrubel, 2015-11-22 02:29:21]

Per discussion, this is a step towards Futures based APIs. In a subsequent CL
I'll add extension points for CompletionContributor and
DartCompletionContributor such that a single contribution to
CompletionContributor delegates to multiple DartCompletionContributors.

[scheglov, 2015-11-22 05:19:02]

LGTM

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/provisional/completion/completion_core.dart
(right):

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/provisional/completion/completion_core.dart:50: *
Compute [suggestions] for the given completion [request].
Why do you use [suggestions] here?
I cannot find its declaration.

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/provisional/completion/completion_dart.dart
(right):

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/provisional/completion/completion_dart.dart:21: *
Compute [suggestions] for the given completion [request].
Where is "suggestions" defined?

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/provisional/completion/completion_dart.dart:43: *
Any any information obtained from [target] prior to calling this method
Really any! :-)

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

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/completion_dart.dart:52: :
super(context, resourceProvider, source, offset);
It may be worth to drop this constructor.
It duplicates constructor arguments of CompletionRequestImpl.
If we ever decide to change them, it would be easier to change only
CompletionRequestImpl.
Maybe always call CompletionRequestImpl.from() constructor.

Or use delegation instead of inheritance.
I'm not sure what is better, if any.

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/completion_dart.dart:74: var
resolvedUnit = await new AnalysisFutureHelper<CompilationUnit>(
Please add the type.

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/completion_dart.dart:75:
(context as AnalysisContextImpl),
Drop parenthesis.
Actually I think we don't need even the cast.

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/completion_dart.dart:77:
RESOLVED_UNIT3).computeAsync();
Now, when we have a single implementation of AnalysisContext it may worth to
drop AnalysisFutureHelper at all and replace it with the new
AnalysisContext.computeResultAsync() method. Not sure how to provide the
requested priority - in case of completion it is interactive, do we need any
other?  See WorkOrderPriority.

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

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart/inherited_contributor.dart:39:
// Partially resolve the compilation unit
If we pretend that these are sentences, I'd love to have a period at the end :-)

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart/inherited_contributor.dart:72:
if (suggestion != null) {
It seems that it cannot be null.

[Brian Wilkerson, 2015-11-22 18:02:47]

LGTM after comments are addressed

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/provisional/completion/completion_core.dart
(right):

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/provisional/completion/completion_core.dart:50: *
Compute [suggestions] for the given completion [request].
Interesting documentation question. Thinking out loud...

If it didn't return a future, I would write:

    Return a list of suggestions appropriate to the given completion [request].

Is the presence of the type Future in the return type sufficient to tell the
reader what's actually happening, or do we need to be explicit in every such
method, such as:

    Return a future that will complete with a list of suggestions appropriate to
the given completion [request].

Seems like a lot of boiler plate to me. Unless there's good reason for it, I'd
like to abbreviate in the docs.

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/provisional/completion/completion_dart.dart
(right):

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/provisional/completion/completion_dart.dart:15: * An
object used to produce completions at a specific location within a file.
"file" --> "Dart file"

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/provisional/completion/completion_dart.dart:44: *
should be discarded as it may have changed.
Needs to document that `null` can be returned and the conditions under which
that should be expected.

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

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/completion_dart.dart:77:
RESOLVED_UNIT3).computeAsync();
> Now, when we have a single implementation of AnalysisContext ...

We effectively only have one implementation now, and we will definitely have a
single implementation within a very short period of time. Not sure we need to
wait to do as Konstantin suggested.

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

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart/inherited_contributor.dart:36:
return null;
The documentation for this method doesn't mention allowing `null` as an
equivalent for an empty list, and I think it's better to require an empty list.
(If CompletionSuggestion doesn't have an EMPTY_LIST constant we can add one.)

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart/inherited_contributor.dart:68:
// Gracefully degrade if the overridden element has not been resolved.
Is that actually necessary? We already have the context because we have the
resolved compilation unit in which completion is being requested. (In fact, if
the element is from the core library, we could get the wrong context by asking
it.)

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart/inherited_contributor.dart:106:
CompilationUnit unit, Element element) {
"Element" --> "ExecutableElement"

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart/inherited_contributor.dart:124:
SimpleIdentifier targetId, CompilationUnit unit, Element element) {
"Element" --> "ExecutableElement"

[danrubel, 2015-11-23 17:47:32]

Comments addressed and per discussion I have commented out usage of
DartChangeBuilder.

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/provisional/completion/completion_core.dart
(right):

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/provisional/completion/completion_core.dart:50: *
Compute [suggestions] for the given completion [request].
On 2015/11/22 18:02:46, Brian Wilkerson wrote:
> Interesting documentation question. Thinking out loud...
> 
> If it didn't return a future, I would write:
> 
>     Return a list of suggestions appropriate to the given completion
[request].
> 
> Is the presence of the type Future in the return type sufficient to tell the
> reader what's actually happening, or do we need to be explicit in every such
> method, such as:
> 
>     Return a future that will complete with a list of suggestions appropriate
to
> the given completion [request].
> 
> Seems like a lot of boiler plate to me. Unless there's good reason for it, I'd
> like to abbreviate in the docs.

Updated dartdoc.

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/provisional/completion/completion_dart.dart
(right):

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/provisional/completion/completion_dart.dart:15: * An
object used to produce completions at a specific location within a file.
On 2015/11/22 18:02:46, Brian Wilkerson wrote:
> "file" --> "Dart file"

Done.

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/provisional/completion/completion_dart.dart:21: *
Compute [suggestions] for the given completion [request].
On 2015/11/22 05:19:01, scheglov wrote:
> Where is "suggestions" defined?

Revised similar to CompletionContributor.

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/provisional/completion/completion_dart.dart:43: *
Any any information obtained from [target] prior to calling this method
On 2015/11/22 05:19:01, scheglov wrote:
> Really any! :-)

Fixed :)

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/provisional/completion/completion_dart.dart:44: *
should be discarded as it may have changed.
On 2015/11/22 18:02:46, Brian Wilkerson wrote:
> Needs to document that `null` can be returned and the conditions under which
> that should be expected.

Done.

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

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/completion_dart.dart:52: :
super(context, resourceProvider, source, offset);
On 2015/11/22 05:19:02, scheglov wrote:
> It may be worth to drop this constructor.
> It duplicates constructor arguments of CompletionRequestImpl.
> If we ever decide to change them, it would be easier to change only
> CompletionRequestImpl.
> Maybe always call CompletionRequestImpl.from() constructor.
> 
> Or use delegation instead of inheritance.
> I'm not sure what is better, if any.

Its easier for the tests to use this constructor, but your point is good. I'll
modify the tests to construct a CompletionRequestImpl and then use
DartCompletionRequestImpl.forRequest(CompletionRequest request).

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/completion_dart.dart:74: var
resolvedUnit = await new AnalysisFutureHelper<CompilationUnit>(
On 2015/11/22 05:19:02, scheglov wrote:
> Please add the type.

Done.

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/completion_dart.dart:75:
(context as AnalysisContextImpl),
On 2015/11/22 05:19:02, scheglov wrote:
> Drop parenthesis.
> Actually I think we don't need even the cast.

Done.

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/completion_dart.dart:77:
RESOLVED_UNIT3).computeAsync();
On 2015/11/22 18:02:46, Brian Wilkerson wrote:
> > Now, when we have a single implementation of AnalysisContext ...
> 
> We effectively only have one implementation now, and we will definitely have a
> single implementation within a very short period of time. Not sure we need to
> wait to do as Konstantin suggested.

Its not clear to me if I need to change anything here. Happy to address in a
subsequent CL.

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

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart/inherited_contributor.dart:36:
return null;
On 2015/11/22 18:02:46, Brian Wilkerson wrote:
> The documentation for this method doesn't mention allowing `null` as an
> equivalent for an empty list, and I think it's better to require an empty
list.
> (If CompletionSuggestion doesn't have an EMPTY_LIST constant we can add one.)

Done.

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart/inherited_contributor.dart:39:
// Partially resolve the compilation unit
On 2015/11/22 05:19:02, scheglov wrote:
> If we pretend that these are sentences, I'd love to have a period at the end
:-)

Acknowledged.

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart/inherited_contributor.dart:68:
// Gracefully degrade if the overridden element has not been resolved.
On 2015/11/22 18:02:47, Brian Wilkerson wrote:
> Is that actually necessary? We already have the context because we have the
> resolved compilation unit in which completion is being requested. (In fact, if
> the element is from the core library, we could get the wrong context by asking
> it.)

I have found situations where the element has not been resolved, I'd like to
keep this guard code.

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart/inherited_contributor.dart:72:
if (suggestion != null) {
On 2015/11/22 05:19:02, scheglov wrote:
> It seems that it cannot be null.

It can now. :-)

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart/inherited_contributor.dart:106:
CompilationUnit unit, Element element) {
On 2015/11/22 18:02:47, Brian Wilkerson wrote:
> "Element" --> "ExecutableElement"

Done.

https://codereview.chromium.org/1467023002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart/inherited_contributor.dart:124:
SimpleIdentifier targetId, CompilationUnit unit, Element element) {
On 2015/11/22 18:02:47, Brian Wilkerson wrote:
> "Element" --> "ExecutableElement"

Done.

[danrubel, 2015-11-23 17:50:16]

Committed patchset #2 (id:20001) manually as
38cec67d032ef0fb6dff662fa4e5dcd3863db062 (presubmit successful).