Improvements to analysis server closing labels

pkg/analysis_server/lib/src/computer/computer_closingLabels.dart

@@ -13,7 +13,7 @@ import 'package:analyzer/src/generated/source.dart';
 class DartUnitClosingLabelsComputer {
   final LineInfo _lineInfo;
   final CompilationUnit _unit;
-  final List<ClosingLabel> _closingLabels = <ClosingLabel>[];
+  final Map<int, List<_ClosingLabelWithLineCount>> _closingLabelsByEndLine = {};
 
   DartUnitClosingLabelsComputer(this._lineInfo, this._unit);
 
@@ -22,67 +22,101 @@ class DartUnitClosingLabelsComputer {
    */
   List<ClosingLabel> compute() {
     _unit.accept(new _DartUnitClosingLabelsComputerVisitor(this));
-    return _closingLabels;
+    return _closingLabelsByEndLine.values
+        .where((l) => l.any((cl) => cl.spannedLines >= 2))
+        .expand((cls) => cls)
+        .map((clwlc) => clwlc.label)
+        .toList();
   }
 }
 
+class _ClosingLabelWithLineCount {
+  final ClosingLabel label;
+  final int spannedLines;
+
+  _ClosingLabelWithLineCount(this.label, this.spannedLines);
+}
+
 /**
  * An AST visitor for [DartUnitClosingLabelsComputer].
  */
 class _DartUnitClosingLabelsComputerVisitor
     extends RecursiveAstVisitor<Object> {
   final DartUnitClosingLabelsComputer computer;
 
+  int interpolatedStringsEntered = 0;
+
   _DartUnitClosingLabelsComputerVisitor(this.computer);
 
   @override
   Object visitInstanceCreationExpression(InstanceCreationExpression node) {
-    if (_spansManyLines(node)) {
+    if (node.argumentList != null) {
       var label = node.constructorName.type.name.name;
       if (node.constructorName.name != null)
         label += ".${node.constructorName.name.name}";
-      _addLabel(node, label);
+      // We override the node used for doing line calculations because otherwise constructors
+      // that split over multiple lines (but have parens on same line) would incorrectly
+      // get labels, because node.start on an instance creation expression starts at the start
+      // of the expression.
+      _addLabel(node, label, checkLinesUsing: node.argumentList);
     }
 
     return super.visitInstanceCreationExpression(node);
   }
 
+  @override
+  visitListLiteral(ListLiteral node) {
+    final args = node.typeArguments?.arguments;
+    final typeName = args != null ? args[0]?.toString() : null;
+
+    if (typeName != null) {
+      _addLabel(node, "List<$typeName>");
+    }
+
+    return super.visitListLiteral(node);
+  }
+
   @override
   Object visitMethodInvocation(MethodInvocation node) {
-    if (node.argumentList != null && _spansManyLines(node)) {
+    if (node.argumentList != null) {
       final target = node.target;
       final label = target is Identifier
           ? "${target.name}.${node.methodName.name}"
           : node.methodName.name;
-      _addLabel(node, label);
+      // We override the node used for doing line calculations because otherwise methods
+      // that chain over multiple lines (but have parens on same line) would incorrectly
+      // get labels, because node.start on a methodInvocation starts at the start of the expression.
+      _addLabel(node, label, checkLinesUsing: node.argumentList);
     }
 
     return super.visitMethodInvocation(node);
   }
 
   @override
-  visitListLiteral(ListLiteral node) {
-    if (_spansManyLines(node)) {
-      final args = node.typeArguments?.arguments;
-      final typeName = args != null ? args[0]?.toString() : null;
-
-      if (typeName != null) {
-        _addLabel(node, "List<$typeName>");
-      }
+  visitStringInterpolation(StringInterpolation node) {
+    interpolatedStringsEntered++;
+    try {
+      return super.visitStringInterpolation(node);
+    } finally {
+      interpolatedStringsEntered--;
     }
-
-    return super.visitListLiteral(node);
   }
 
-  bool _spansManyLines(AstNode node) {
-    final start = computer._lineInfo.getLocation(node.offset);
-    final end = computer._lineInfo.getLocation(node.end - 1);
+  void _addLabel(AstNode node, String label, {AstNode checkLinesUsing}) {
+    // Never add labels if we're inside strings.
+    if (interpolatedStringsEntered > 0) {
+      return;
+    }
 
-    return end.lineNumber - start.lineNumber > 1;
-  }
+    checkLinesUsing = checkLinesUsing ?? node;
+    final start = computer._lineInfo.getLocation(checkLinesUsing.offset);
+    final end = computer._lineInfo.getLocation(checkLinesUsing.end - 1);
+    final closingLabel = new ClosingLabel(node.offset, node.length, label);
+    final labelWithSpan = new _ClosingLabelWithLineCount(
+        closingLabel, end.lineNumber - start.lineNumber);
 
-  void _addLabel(AstNode node, String label) {
-    computer._closingLabels
-        .add(new ClosingLabel(node.offset, node.length, label));
+    computer._closingLabelsByEndLine
+        .putIfAbsent(end.lineNumber, () => <_ClosingLabelWithLineCount>[])
+        .add(labelWithSpan);
   }
 }

pkg/analysis_server/test/analysis/notification_closingLabels_test.dart

@@ -20,19 +20,25 @@ main() {
 
 @reflectiveTest
 class _AnalysisNotificationClosingLabelsTest extends AbstractAnalysisTest {
-  List<ClosingLabel> lastLabels;
+  static const sampleCode = '''
+Widget build(BuildContext context) {
+  return /*1*/new Row(
+    children: /*2*/<Widget>[
+      new Text('a'),
+      new Text('b'),
+    ]/*/2*/,
+  )/*/1*/;
+}
+''';
 
-  Completer _labelsReceived;
+  static final expectedResults = [
+    new ClosingLabel(51, 96, "Row"),
+    new ClosingLabel(79, 57, "List<Widget>")
+  ];
 
-  void subscribeForLabels() {
-    addAnalysisSubscription(AnalysisService.CLOSING_LABELS, testFile);
-  }
+  List<ClosingLabel> lastLabels;
 
-  Future waitForLabels(action()) {
-    _labelsReceived = new Completer();
-    action();
-    return _labelsReceived.future;
-  }
+  Completer _labelsReceived;
 
   void processNotification(Notification notification) {
     if (notification.event == ANALYSIS_NOTIFICATION_CLOSING_LABELS) {
@@ -54,21 +60,9 @@ class _AnalysisNotificationClosingLabelsTest extends AbstractAnalysisTest {
     createProject();
   }
 
-  static const sampleCode = '''
-Widget build(BuildContext context) {
-  return /*1*/new Row(
-    children: /*2*/<Widget>[
-      new Text('a'),
-      new Text('b'),
-    ]/*/2*/,
-  )/*/1*/;
-}
-''';
-
-  static final expectedResults = [
-    new ClosingLabel(51, 96, "Row"),
-    new ClosingLabel(79, 57, "List<Widget>")
-  ];
+  void subscribeForLabels() {
+    addAnalysisSubscription(AnalysisService.CLOSING_LABELS, testFile);
+  }
 
   test_afterAnalysis() async {
     addTestFile(sampleCode);
@@ -98,4 +92,10 @@ Widget build(BuildContext context) {
 
     expect(lastLabels, expectedResults);
   }
+
+  Future waitForLabels(action()) {
+    _labelsReceived = new Completer();
+    action();
+    return _labelsReceived.future;
+  }
 }