update completion sorter to new API

update completion sorter to new API
first step updating to the new completion API

R=brianwilkerson@google.com

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

[danrubel, 2015-11-13 20:48:16]

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

[danrubel, 2015-11-13 20:48:16]

This is a first step towards the new completion API that we discussed. The
completion sorter now references CompletionRequest rather than
DartCompletionRequest and returns AnalysisRequest.

[danrubel, 2015-11-13 21:03:46]

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

https://codereview.chromium.org/1447503002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/contribution_sorter.dart:25:
AnalysisRequest sort(
Although the default completion sorter needs no additional analysis, Luke's
dart-mind based completion sorter does, thus the sort method can return an
AnalysisRequest.

[Brian Wilkerson, 2015-11-14 16:59:24]

LGTM after comments are addressed

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

https://codereview.chromium.org/1447503002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/provisional/completion/completion_core.dart:44: *
Clients may extend this class when implementing plugins.
Out of curiosity, why `extend` rather than `implement`?

https://codereview.chromium.org/1447503002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/provisional/completion/completion_core.dart:51:
List<CompletionSuggestion> computeSuggestions(CompletionRequest request);
Am I misremembering, or did your previous CL introduce `AnalysisCallback` and 
`AnalysisRequest` and change this interface?

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

https://codereview.chromium.org/1447503002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/common_usage_computer.dart:48:
var libElem = request.context.getResult(request.source, LIBRARY_ELEMENT1);
If `request.source` is a part file, then `libElem` will always be `null`. You
need to ask for the `CONTAINING_LIBRARIES` and deal with the possibility that
the part might be in multiple libraries.

https://codereview.chromium.org/1447503002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/common_usage_computer.dart:49:
if (libElem is LibraryElement) {
The value of `libElem` can only be a LibraryElement or `null`. Personally, I'd
find the code to be cleaner if we gave `libElem` a type of `LibraryElement` and
made the null check explicit here. (Of course, this will all change anyway as a
result of my previous comment...)

In general, it's better to include type annotations everywhere so that we get
extra checking when we run our tests.

https://codereview.chromium.org/1447503002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/common_usage_computer.dart:53:
if (unit is CompilationUnit){
"){"? Did the formatter leave the code this way?

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

https://codereview.chromium.org/1447503002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/contribution_sorter.dart:25:
AnalysisRequest sort(
Doesn't that violate the requirement that "This method should execute quickly
and not block"?

Should "should" be replaced by "must"? What's the effect of implementing a
method that is slow or that has to wait for some analysis to complete?

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

https://codereview.chromium.org/1447503002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart_completion_manager.dart:187:
contributionSorter.sort(request, request.suggestions);
Given that this one line will turn into multiple lines, it would be best to
extract it into its own method now.

Also, the comment is a little vague. I'm not sure I'll remember in 3 months what
needed to be done here. Assuming I understand now, I think the eventual code
needs to be

AnalysisRequest request = contributionSorter.sort(request, request.suggestions);
while (request != null) {
    Object result = ...;
    request = request.callback(request, result);
}

And I assume the "..." is implemented somewhere already as part of the previous
changes. We should either implement it now or add this code as a concrete
reminder.

[danrubel, 2015-11-17 06:58:45]

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

https://codereview.chromium.org/1447503002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/provisional/completion/completion_core.dart:44: *
Clients may extend this class when implementing plugins.
On 2015/11/14 16:59:23, Brian Wilkerson wrote:
> Out of curiosity, why `extend` rather than `implement`?

Ack. Should not have been changed. Fixed.

https://codereview.chromium.org/1447503002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/provisional/completion/completion_core.dart:51:
List<CompletionSuggestion> computeSuggestions(CompletionRequest request);
On 2015/11/14 16:59:24, Brian Wilkerson wrote:
> Am I misremembering, or did your previous CL introduce `AnalysisCallback` and 
> `AnalysisRequest` and change this interface?

My prior CL was a snapshot for you to review the overall direction of this new
API migration, but I had no intention of committing. This is the first of
several smaller CLs that I intend to land. Sorry I did not make that clear in
our discussions.

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

https://codereview.chromium.org/1447503002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/common_usage_computer.dart:48:
var libElem = request.context.getResult(request.source, LIBRARY_ELEMENT1);
On 2015/11/14 16:59:24, Brian Wilkerson wrote:
> If `request.source` is a part file, then `libElem` will always be `null`. You
> need to ask for the `CONTAINING_LIBRARIES` and deal with the possibility that
> the part might be in multiple libraries.

Good point. Fixed.

https://codereview.chromium.org/1447503002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/common_usage_computer.dart:53:
if (unit is CompilationUnit){
On 2015/11/14 16:59:24, Brian Wilkerson wrote:
> "){"? Did the formatter leave the code this way?

Oops. Must forgotten to run the formatter. Done.

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

https://codereview.chromium.org/1447503002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/contribution_sorter.dart:25:
AnalysisRequest sort(
On 2015/11/14 16:59:24, Brian Wilkerson wrote:
> Doesn't that violate the requirement that "This method should execute quickly
> and not block"?
> 
> Should "should" be replaced by "must"? What's the effect of implementing a
> method that is slow or that has to wait for some analysis to complete?

The current default sorter requires no additional analysis so I'm not worried in
the near to mid term.  Obviously any sorter that requests additional analysis
runs the risk of making completion non-performant. Allowing the sorter to
request additional analysis allows experimentation with the sorter
implementation (e.g. dart-mind), but we must vet any implementation before we
build it into the system.

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

https://codereview.chromium.org/1447503002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart_completion_manager.dart:187:
contributionSorter.sort(request, request.suggestions);
On 2015/11/14 16:59:24, Brian Wilkerson wrote:
> Given that this one line will turn into multiple lines, it would be best to
> extract it into its own method now.
> 
> Also, the comment is a little vague. I'm not sure I'll remember in 3 months
what
> needed to be done here. Assuming I understand now, I think the eventual code
> needs to be
> 
> AnalysisRequest request = contributionSorter.sort(request,
request.suggestions);
> while (request != null) {
>     Object result = ...;
>     request = request.callback(request, result);
> }
> 
> And I assume the "..." is implemented somewhere already as part of the
previous
> changes. We should either implement it now or add this code as a concrete
> reminder.

Good point. I'll address that in a subsequent CL.

[danrubel, 2015-11-17 07:01:46]

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

[danrubel, 2015-11-17 23:12:54]

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

https://codereview.chromium.org/1447503002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart_completion_manager.dart:187:
contributionSorter.sort(request, request.suggestions);
On 2015/11/17 06:58:45, danrubel wrote:
> On 2015/11/14 16:59:24, Brian Wilkerson wrote:
> > Given that this one line will turn into multiple lines, it would be best to
> > extract it into its own method now.
> > 
> > Also, the comment is a little vague. I'm not sure I'll remember in 3 months
> what
> > needed to be done here. Assuming I understand now, I think the eventual code
> > needs to be
> > 
> > AnalysisRequest request = contributionSorter.sort(request,
> request.suggestions);
> > while (request != null) {
> >     Object result = ...;
> >     request = request.callback(request, result);
> > }
> > 
> > And I assume the "..." is implemented somewhere already as part of the
> previous
> > changes. We should either implement it now or add this code as a concrete
> > reminder.
> 
> Good point. I'll address that in a subsequent CL.

https://codereview.chromium.org/1449333002/