Add completion support for plugins

pkg/analyzer_plugin/lib/utilities/completion.dart

+// Copyright (c) 2017, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'dart:async';
+
+import 'package:analyzer/dart/analysis/results.dart';
+import 'package:analyzer/file_system/file_system.dart';
+import 'package:analyzer_plugin/protocol/protocol.dart';
+import 'package:analyzer_plugin/protocol/protocol_common.dart';
+import 'package:analyzer_plugin/protocol/protocol_generated.dart';
+import 'package:analyzer_plugin/src/utilities/completion.dart';
+import 'package:analyzer_plugin/utilities/generator.dart';
+
+/**
+ * An exception that is thrown when the current completion request should be
+ * aborted because either the source changed since the request was made, or
+ * a new completion request was received.
+ */
+class AbortCompletion {}
+
+/**
+ * An object that [CompletionContributor]s use to record completion suggestions.
+ *
+ * Clients may not extend, implement or mix-in this class.
+ */
+abstract class CompletionCollector {
+  /**
+   * Set the length of the region of text that should be replaced by the
+   * selected completion suggestion.
+   */
+  void set length(int length);
+
+  /**
+   * Set the offset of the region of text that should be replaced by the
+   * selected completion suggestion.
+   */
+  void set offset(int offset);

[scheglov, 2017/05/31 19:26:03]

Should we throw an exception if a different value is set?
So that client's code does not expect that it can set one offset/length,
addSuggestion(s), then set another offset/length, add more suggestions.

[Brian Wilkerson, 2017/05/31 19:33:20]

Maybe. I definitely considered that.

It would require the angular plugin to change some. It's currently setting
default values, then sometimes changing those later.

But it would be safer. Minimally I should probably add some clarifying text to
the doc comment.

[mfairhurst, 2017/05/31 21:49:34]

We could definitely make this change if necessary. We calculate offsets and
regions with one algorithm in one place, so I don't think the defaults are
strictly necessary.

[Brian Wilkerson, 2017/06/03 16:44:37]

Done

+
+  /**
+   * Record the given completion [suggestion].
+   */
+  void addSuggestion(CompletionSuggestion suggestion);
+}
+
+/**
+ * An object used to produce completion suggestions.
+ *
+ * Clients may implement this class when implementing plugins.
+ */
+abstract class CompletionContributor {
+  /**
+   * Contribute completion suggestions for the completion location specified by
+   * the given [request] into the given [collector].
+   */
+  Future<Null> computeSuggestions(
+      CompletionRequest request, CompletionCollector collector);
+}
+
+/**
+ * A generator that will generate a 'completion.getSuggestions' response.
+ *
+ * Clients may not extend, implement or mix-in this class.
+ */
+class CompletionGenerator {
+  /**
+   * The contributors to be used to generate the completion suggestions.
+   */
+  final List<CompletionContributor> contributors;
+
+  /**
+   * Initialize a newly created completion generator.
+   */
+  CompletionGenerator(this.contributors);
+
+  /**
+   * Create a 'completion.getSuggestions' response for the file with the given
+   * [path]. If any of the contributors throws an exception, also create a
+   * non-fatal 'plugin.error' notification.
+   */
+  Future<GeneratorResult> generateCompletionResponse(
+      CompletionRequest request) async {
+    List<Notification> notifications = <Notification>[];
+    CompletionCollectorImpl collector = new CompletionCollectorImpl();
+    try {
+      for (CompletionContributor contributor in contributors) {
+        request.checkAborted();

[mfairhurst, 2017/05/31 21:49:35]

should this be checked at the end of the contributors running too?

[Brian Wilkerson, 2017/05/31 22:51:47]

I assume that the time it takes to get the next contributor from the iterator is
small enough that the end of the loop is essentially the same as the beginning
of the loop, so I don't think that would be useful.

[mfairhurst, 2017/05/31 23:17:36]

I mostly mean that the unrolled loop looks like:

checkAbort();
runContributor1();
checkAbort();
runContributor2();
sendNotifications();

rather than

checkAbort();
runContributor1();
checkAbort();
runContributor2();
checkAbort();
sendNotifications();

because runContributor2() may take a nontrivial amount of time. But depending on
what checkAbort is trying to accomplish it may not matter -- if its only trying
to avoid running contributors that don't need to be run, the difference between
the two doesn't matter.

[Brian Wilkerson, 2017/06/03 16:44:37]

Yes, that's the purpose. The spec doesn't say what we do if a completion request
is received before the last response is sent, but we know that all of our
existing clients will discard any results for anything other than the most
recent request. We're just avoiding unnecessary work.

+        try {
+          await contributor.computeSuggestions(request, collector);

[maxkim, 2017/05/31 17:44:35]

Really liking the new structure.

Let me see if I'm following the new design pattern: 
every plugin will be responsible for providing a 'Contributor' which is added
into the Generator's list of known contributors. In this sense, every plugin and
its contributor is given the collector object to load additions into the
suggestions and override the offset and length as necessary? 

OR,

Is the intended strategy for every plugin to have their own generator?

[Brian Wilkerson, 2017/05/31 18:39:43]

Every plugin is running in a separate isolate, and as such cannot share anything
with any other plugin (or with server), so each plugin will create it's own
generator and pass in one or more contributors. (The angular plugin is currently
using three contributors, two of which are from server: TypeMemberContributor
and InheritedReferenceContributor. Hence my comment about needing to get those
ported over.)

[maxkim, 2017/05/31 19:04:03]

Understood. Just wanted to make sure it was clear - but each plugin creating its
own generator and needed contributors (created or imported) sounds great.

+        } catch (exception, stackTrace) {

[mfairhurst, 2017/05/31 21:49:34]

Maybe this shouldn't be caught here, but at the layer above, so that
`ServerPlugin` can send the notification for this, and this can return a
CompletionSuggestionsResult directly without the wrapper class

[Brian Wilkerson, 2017/05/31 22:51:47]

If we catch this in an outer context, then this method wouldn't be able to
return any suggestions if one of the contributors throws an exception. I was
thinking that it would be better to return partial results than to return no
results.

[mfairhurst, 2017/05/31 23:17:36]

Interesting. Yeah, that makes sense.

I guess my main concern is that it will be easy to forget to send the
notifications inside the implementation of ServerPlugin. Is there any other way
we can make that easier to remember? Like passing a Channel reference in instead
of getting a List out?

[Brian Wilkerson, 2017/06/03 16:44:37]

Your suggestion elsewhere, which I like, of defining a "completion mixin" would
solve this problem nicely.

+          notifications.add(new PluginErrorParams(
+                  false, exception.toString(), stackTrace.toString())
+              .toNotification());
+        }
+      }
+    } on AbortCompletion {
+      return new GeneratorResult(null, notifications);
+    }
+    CompletionGetSuggestionsResult result = new CompletionGetSuggestionsResult(
+        collector.offset, collector.length, collector.suggestions);
+    return new GeneratorResult(result, notifications);
+  }
+}
+
+/**
+ * The information about a requested list of completions.
+ *
+ * Clients may not extend, implement or mix-in this class.
+ */
+abstract class CompletionRequest {
+  /**
+   * Return the offset within the source at which the completion is being
+   * requested.
+   */
+  int get offset;
+
+  /**
+   * Return the resource provider associated with this request.
+   */
+  ResourceProvider get resourceProvider;

[mfairhurst, 2017/05/31 21:49:35]

What is the purpose of the resource provider?

[Brian Wilkerson, 2017/05/31 22:51:47]

It's needed by the completion contributor that works inside (import, export and
part) directives. We use the resource provider for all file system access. It
has the advantage that many of our tests can get data from memory rather than a
real file system, which decreases how long it takes to run them.

[mfairhurst, 2017/05/31 23:17:36]

I would say then that it makes more sense for the contributor to hold a
reference to the resourceProvider than for the request to hold a reference to
it. But then contributor instances are coupled to their
resourceProviders....Maybe, since drivers are already coupled to resource
providers, that drivers should produce contributors. But then contributors would
have to be known by package:analyzer....hmmm....

[Brian Wilkerson, 2017/06/03 16:44:37]

If the request object holds on to the resource provider, then we don't require
every implementation of a contributor to have a field for it and to take a
provider as an argument to the constructor. So there's less boiler plate for
plugin contributors to duplicate.

+
+  /**
+   * The analysis result for the file in which the completion is being
+   * requested.
+   */
+  ResolveResult get result;
+
+  /**
+   * Throw an [AbortCompletion] if the completion request has been aborted.
+   */
+  void checkAborted();
+}