Add documentation to completion metrics and clean up a bit of code

Change-Id: I612b5f5953bc0f8d94ad9976e55590097389c62e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/210740
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>

pkg/analysis_server/tool/code_completion/completion_metrics.dart

@@ -58,6 +58,32 @@ import 'output_utilities.dart';
 import 'relevance_table_generator.dart';
 import 'visitors.dart';
 
+/// Completion metrics are computed by taking a single package and iterating
+/// over the compilation units in the package. For each unit we visit the AST
+/// structure to find all of the places where completion suggestions should be
+/// offered (essentially, everywhere that there's a keyword or identifier). At
+/// each location we compute the completion suggestions using the same code path
+/// used by the analysis server. We then compare the suggestions against the
+/// token that was actually at that location in the file.
+///
+/// This approach has several drawbacks:
+///
+/// - The AST is always complete and correct, and that's rarely the case for
+///   real completion requests. Usually the tree is incomplete and often has a
+///   completely different structure because of the way recovery works. We
+///   currently have no way of measuring completions under realistic conditions.
+///
+/// - We can't measure completions for several keywords because the presence of
+///   the keyword in the AST causes it to not be suggested.
+///
+/// - The time it takes to compute the suggestions doesn't include the time
+///   required to finish analyzing the file if the analysis hasn't been
+///   completed before suggestions are requested. While the times are accurate
+///   (within the accuracy of the `Stopwatch` class) they are the minimum
+///   possible time. This doesn't give us a measure of how completion will
+///   perform in production, but does give us an optimistic approximation.
+///
+/// The first is arguably the worst of the limitations of our current approach.
 Future<void> main(List<String> args) async {
   var parser = createArgParser();
   var result = parser.parse(args);
@@ -1318,7 +1344,7 @@ class CompletionMetricsComputer {
       var filePath = result.path;
       // Use the ExpectedCompletionsVisitor to compute the set of expected
       // completions for this CompilationUnit.
-      final visitor = ExpectedCompletionsVisitor(filePath);
+      final visitor = ExpectedCompletionsVisitor(result);
       _resolvedUnitResult.unit.accept(visitor);
 
       for (var expectedCompletion in visitor.expectedCompletions) {

pkg/analysis_server/tool/code_completion/visitors.dart

@@ -5,11 +5,13 @@
 import 'package:analysis_server/src/protocol/protocol_internal.dart';
 import 'package:analysis_server/src/protocol_server.dart' as protocol;
 import 'package:analysis_server/src/services/completion/dart/keyword_contributor.dart';
+import 'package:analyzer/dart/analysis/results.dart';
 import 'package:analyzer/dart/ast/ast.dart';
 import 'package:analyzer/dart/ast/syntactic_entity.dart';
 import 'package:analyzer/dart/ast/token.dart';
 import 'package:analyzer/dart/ast/visitor.dart';
 import 'package:analyzer/dart/element/element.dart' as element;
+import 'package:analyzer/source/line_info.dart';
 
 class ExpectedCompletion {
   final String _filePath;
@@ -122,11 +124,12 @@ class ExpectedCompletion {
 }
 
 class ExpectedCompletionsVisitor extends RecursiveAstVisitor<void> {
-  final List<ExpectedCompletion> expectedCompletions;
+  /// The result of resolving the file being visited.
+  final ResolvedUnitResult result;
 
-  final String filePath;
+  /// The completions that are expected to be produced in the file being
-
+  /// visited.
-  late CompilationUnit _enclosingCompilationUnit;
+  final List<ExpectedCompletion> expectedCompletions = [];
 
   /// This boolean is set to enable whether or not we should assert that some
   /// found keyword in Dart syntax should be in the completion set returned from
@@ -141,22 +144,22 @@ class ExpectedCompletionsVisitor extends RecursiveAstVisitor<void> {
   /// comment don't yield an error like Dart syntax mistakes would yield.
   final bool _doExpectCommentRefs = false;
 
-  ExpectedCompletionsVisitor(this.filePath)
+  ExpectedCompletionsVisitor(this.result);
-      : expectedCompletions = <ExpectedCompletion>[];
+
+  /// Return the path of the file that is being visited.
+  String get filePath => result.path;
+
+  /// Return the line info for the file that is being visited.
+  LineInfo get lineInfo => result.lineInfo;
 
   void safelyRecordEntity(SyntacticEntity? entity,
       {protocol.CompletionSuggestionKind? kind,
       protocol.ElementKind? elementKind}) {
     // Only record if this entity is not null, has a length, etc.
-    if (entity != null && entity.offset > 0 && entity.length > 0) {
+    if (entity != null && entity.offset >= 0 && entity.length > 0) {
-      // Compute the line number at this offset
+      var location = lineInfo.getLocation(entity.offset);
-      var lineNumber = _enclosingCompilationUnit.lineInfo!
+      var lineNumber = location.lineNumber;
-          .getLocation(entity.offset)
+      var columnNumber = location.columnNumber;
-          .lineNumber;
-
-      var columnNumber = _enclosingCompilationUnit.lineInfo!
-          .getLocation(entity.offset)
-          .columnNumber;
 
       bool isKeyword() => kind == protocol.CompletionSuggestionKind.KEYWORD;
 
@@ -314,12 +317,6 @@ class ExpectedCompletionsVisitor extends RecursiveAstVisitor<void> {
     super.visitClassTypeAlias(node);
   }
 
-  @override
-  void visitCompilationUnit(CompilationUnit node) {
-    _enclosingCompilationUnit = node;
-    super.visitCompilationUnit(node);
-  }
-
   @override
   void visitConfiguration(Configuration node) {
     safelyRecordKeywordCompletion(node.ifKeyword);