use CompletionTarget for replacement offset/length

pkg/analysis_server/lib/src/services/completion/dart_completion_manager.dart

Index: pkg/analysis_server/lib/src/services/completion/dart_completion_manager.dart
diff --git a/pkg/analysis_server/lib/src/services/completion/dart_completion_manager.dart b/pkg/analysis_server/lib/src/services/completion/dart_completion_manager.dart
index 65e3a46187c4f916e23437caf99d0bd0fe9506da..1c19a17d3ed133889deac8159febf438716e7cbf 100644
--- a/pkg/analysis_server/lib/src/services/completion/dart_completion_manager.dart
+++ b/pkg/analysis_server/lib/src/services/completion/dart_completion_manager.dart
@@ -21,6 +21,7 @@ import 'package:analysis_server/src/services/search/search_engine.dart';
 import 'package:analyzer/src/generated/ast.dart';
 import 'package:analyzer/src/generated/engine.dart';
 import 'package:analyzer/src/generated/source.dart';
+import 'package:analyzer/src/generated/scanner.dart';
 
 // TODO (danrubel) these are temporary constants as we transition completion
 // relevance from CompletionRelevance.LOW/DEFAULT/HIGH to int.
@@ -61,15 +62,18 @@ class DartCompletionManager extends CompletionManager {
   List<DartCompletionComputer> computers;
 
   DartCompletionManager(AnalysisContext context, this.searchEngine,
-      Source source, this.cache)
-      : super(context, source),
-        computers = [
+      Source source, this.cache, [this.computers])
+      : super(context, source) {
+    if (computers == null) {
+      computers = [
           new KeywordComputer(),
           new LocalComputer(),
           new ArgListComputer(),
           new CombinatorComputer(),
           new ImportedComputer(),
           new InvocationComputer()];
+    }
+  }
 
   /**
    * Create a new initialized Dart source completion manager
@@ -104,8 +108,25 @@ class DartCompletionManager extends CompletionManager {
       CompilationUnit unit = context.parseCompilationUnit(source);
       request.unit = unit;
       request.node = new NodeLocator.con1(request.offset).searchWithin(unit);
-      request.node.accept(new _ReplacementOffsetBuilder(request));
       request.target = new CompletionTarget.forOffset(unit, request.offset);
+
+      request.replacementOffset = request.offset;

[Paul Berry, 2015/01/28 16:32:21]

Some issues with this code:
- It doesn't handle the case where entity is a keyword token but not an AstNode.
 (This would happen when the cursor is on an "is" or "as" keyword).
- The "if" and "else if" clauses below are redundant, since SimpleIdentifier is
a subclass of AstNode (and the logic is equivalent).
- The test "token.offset <= request.offset" is redundant, since the
CompletionTarget logic will never return a completion target that is after the
requested offset position.

Suggested alternative:

request.replacementOffset = request.offset;
request.replacementLength = 0;
var entity = request.target.entity;
Token targetToken = entity is AstNode ? entity.beginToken : entity;
if (targetToken != null &&
    (targetToken.type == TokenType.KEYWORD ||
     targetToken.type == TokenType.IDENTIFIER)) {
  request.replacementOffset = targetToken.offset;
  request.replacementLength = targetToken.length;
}

[danrubel, 2015/01/28 20:10:43]

Good eyes. I like that code better. Done with a slight tweak.

+      request.replacementLength = 0;
+      var entity = request.target.entity;
+      if (entity is SimpleIdentifier) {
+        if (entity.offset <= request.offset) {
+          request.replacementOffset = entity.offset;
+          request.replacementLength = entity.length;
+        }
+      } else if (entity is AstNode) {
+        Token token = entity.beginToken;
+        if (token.offset <= request.offset &&
+            (token.type == TokenType.KEYWORD || token.type == TokenType.IDENTIFIER)) {
+          request.replacementOffset = token.offset;
+          request.replacementLength = token.length;
+        }
+      }
+
       List<DartCompletionComputer> todo = new List.from(computers);
       todo.removeWhere((DartCompletionComputer c) {
         return request.performance.logElapseTime(
@@ -122,14 +143,12 @@ class DartCompletionManager extends CompletionManager {
   /**
    * If there is remaining work to be done, then wait for the unit to be
    * resolved and request that each remaining computer finish their work.
+   * Return a [Future] that completes when the last notification has been sent.
    */
-  void computeFull(DartCompletionRequest request,
+  Future computeFull(DartCompletionRequest request,
       List<DartCompletionComputer> todo) {
     request.performance.logStartTime('waitForAnalysis');
-    waitForAnalysis().then((CompilationUnit unit) {
-      if (controller.isClosed) {

[Paul Berry, 2015/01/28 16:32:21]

Nit: can we keep this?  Granted, it's unnecessary because of your change to
sendResults() below, but it shouldn't cause any harm, and it will save us from
doing unnecessary work in the case where the completion results aren't needed.

[danrubel, 2015/01/28 20:10:43]

Makes sense. I'll put this back in. I've made the test code use this class's
computeFast method rather than rolling it into the test infrastructure. I want
to do the same for this computeFull method which is why I removed the
controller.isClosed() statement, but based on your suggestion I'll find a
different modification that will satisfy testing and keeping this statement for
efficiency.

-        return;
-      }
+    return waitForAnalysis().then((CompilationUnit unit) {
       request.performance.logElapseTime('waitForAnalysis');
       if (unit == null) {
         sendResults(request, true);
@@ -181,7 +200,7 @@ class DartCompletionManager extends CompletionManager {
    * Send the current list of suggestions to the client.
    */
   void sendResults(DartCompletionRequest request, bool last) {
-    if (controller.isClosed) {
+    if (controller == null || controller.isClosed) {
       return;
     }
     controller.add(
@@ -315,21 +334,3 @@ class DartCompletionRequest extends CompletionRequest {
     return _optype;
   }
 }
-
-/**
- * Visitor used to determine the replacement offset and length
- * based upon the cursor location.
- */
-class _ReplacementOffsetBuilder extends SimpleAstVisitor {
-  final DartCompletionRequest request;
-
-  _ReplacementOffsetBuilder(this.request) {
-    request.replacementOffset = request.offset;
-    request.replacementLength = 0;
-  }
-
-  visitSimpleIdentifier(SimpleIdentifier node) {
-    request.replacementOffset = node.offset;
-    request.replacementLength = node.length;
-  }
-}