[analysis_server] Prevent LSP format-on-type from formatting in unwanted cases

We register `}` and `;` as trigger characters for format-on-type, but some instances of these characters should not trigger formatting (for example in strings and comments).

With this change, we'll look at the AST and only format for specific cases we've opted in to, instead of all.

Fixes Dart-Code/Dart-Code#5832

Change-Id: I94d348520649ea2e072f13350863f27a2e07699d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/466100
Reviewed-by: Brian Wilkerson <[email protected]>
Reviewed-by: Samuel Rawlins <[email protected]>
Commit-Queue: Brian Wilkerson <[email protected]>

Author: DanTup

pkg/analysis_server/lib/src/lsp/constants.dart

@@ -2,6 +2,9 @@
 // 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.
 
+/// @docImport 'package:analysis_server/src/lsp/handlers/handler_format_on_type.dart';
+library;
+
 import 'package:analysis_server/lsp_protocol/protocol.dart';
 import 'package:analysis_server/src/services/refactoring/framework/refactoring_processor.dart';
 
@@ -43,6 +46,9 @@ const dartSignatureHelpRetriggerCharacters = <String>[','];
 const dartSignatureHelpTriggerCharacters = <String>['('];
 
 /// Characters to trigger formatting when format-on-type is enabled.
+///
+/// [FormatOnTypeHandler._shouldTriggerFormatting] contains the logic for when
+/// each of these characters will allow formatting.
 const dartTypeFormattingCharacters = ['}', ';'];
 
 /// A [TextDocumentFilterScheme] for Analysis Options files.

pkg/analysis_server/lib/src/lsp/handlers/handler_format_on_type.dart

@@ -9,6 +9,9 @@ import 'package:analysis_server/src/lsp/handlers/handlers.dart';
 import 'package:analysis_server/src/lsp/mapping.dart';
 import 'package:analysis_server/src/lsp/registration/feature_registration.dart';
 import 'package:analysis_server/src/lsp/source_edits.dart';
+import 'package:analyzer/dart/analysis/results.dart';
+import 'package:analyzer/dart/ast/ast.dart';
+import 'package:analyzer/dart/ast/token.dart';
 
 typedef StaticOptions = DocumentOnTypeFormattingOptions?;
 
@@ -27,7 +30,11 @@ class FormatOnTypeHandler
   @override
   bool get requiresTrustedCaller => false;
 
-  Future<ErrorOr<List<TextEdit>?>> formatFile(String path) async {
+  Future<ErrorOr<List<TextEdit>?>> formatFile(
+    String path,
+    Position typedCharacterPosition,
+    String typedCharacter,
+  ) async {
     var file = server.resourceProvider.getFile(path);
     if (!file.exists) {
       return error(
@@ -42,6 +49,18 @@ class FormatOnTypeHandler
       return success(null);
     }
 
+    // The client will always send a request when a trigger character is typed
+    // even if it's in a comment or something else that shouldn't trigger
+    // formatting so we need to check the character/position to see if this is
+    // really something that should be allowed to trigger formatting.
+    if (!_shouldTriggerFormatting(
+      result,
+      typedCharacterPosition,
+      typedCharacter,
+    )) {
+      return success(null);
+    }
+
     var lineLength = server.lspClientConfiguration.forResource(path).lineLength;
     return generateEditsForFormatting(result, defaultPageWidth: lineLength);
   }
@@ -64,9 +83,80 @@ class FormatOnTypeHandler
         return success(null);
       }
 
-      return await formatFile(path);
+      return await formatFile(path, params.position, params.ch);
     });
   }
+
+  /// Returns whether [character] at [position] should trigger formatting.
+  ///
+  /// Generally we only want to trigger formatting for things like a `;` at the
+  /// end of a statement, and not inside something like a string or comment.
+  ///
+  /// Trigger characters are declared in [dartTypeFormattingCharacters].
+  bool _shouldTriggerFormatting(
+    ParsedUnitResult result,
+    Position position,
+    String character,
+  ) {
+    var offset =
+        result.lineInfo.getOffsetOfLine(position.line) + position.character;
+    var node = result.unit.nodeCovering(offset: offset);
+
+    /// Helper to check if a token is at the current offset.
+    ///
+    /// Check both offset/end because the LSP spec says
+    /// >> This is not necessarily the exact position where the character
+    /// >> denoted by the property `ch` got typed.
+    /// and in testing with VS Code it's the end.
+    bool isAtOffset(Token token) =>
+        token.offset == offset || token.end == offset;
+
+    return switch (character) {
+      // Only consider semicolons that are the end of statements/declarations
+      ';' => switch (node) {
+        // Statements
+        AssertStatement(:var semicolon) ||
+        BreakStatement(:var semicolon) ||
+        ContinueStatement(:var semicolon) ||
+        DoStatement(:var semicolon) ||
+        EmptyStatement(:var semicolon) ||
+        ExpressionStatement(:var semicolon?) ||
+        PatternVariableDeclarationStatement(:var semicolon) ||
+        ReturnStatement(:var semicolon) ||
+        VariableDeclarationStatement(:var semicolon) ||
+        YieldStatement(:var semicolon) ||
+        // Bodies
+        EmptyClassBody(:var semicolon) ||
+        EmptyFunctionBody(:var semicolon) ||
+        EnumBody(:var semicolon?) ||
+        ExpressionFunctionBody(:var semicolon?) ||
+        NativeFunctionBody(:var semicolon) ||
+        // Declarations
+        FieldDeclaration(:var semicolon) ||
+        TopLevelVariableDeclaration(:var semicolon) ||
+        TypeAlias(:var semicolon) ||
+        // Directives
+        LibraryDirective(:var semicolon) ||
+        NamespaceDirective(:var semicolon) ||
+        PartDirective(:var semicolon) ||
+        PartOfDirective(:var semicolon) => isAtOffset(semicolon),
+        _ => false,
+      },
+      // Only consider closing braces that are the end of "blocks" but not
+      // things like patterns that might usually be inline.
+      '}' => switch (node) {
+        Block(:var rightBracket) ||
+        BlockClassBody(:var rightBracket) ||
+        EnumBody(:var rightBracket) ||
+        ListLiteral(:var rightBracket) ||
+        SetOrMapLiteral(:var rightBracket) ||
+        SwitchExpression(:var rightBracket) ||
+        SwitchStatement(:var rightBracket) => isAtOffset(rightBracket),
+        _ => false,
+      },
+      _ => false,
+    };
+  }
 }
 
 class FormatOnTypeRegistrations extends FeatureRegistration

pkg/analysis_server/test/lsp/format_test.dart

@@ -27,6 +27,46 @@ class FormatTest extends AbstractLspAnalysisServerTest {
   static const codeThatWrapsAt40 =
       "var a = '        10        20        30        40';";
 
+  /// Verifies the result of a format-on-type request for the character in the
+  /// marked range in [content].
+  ///
+  /// If the marked range has a position, it will be used for the request,
+  /// otherwise the end point of the range will be used.
+  Future<List<TextEdit>?> expectFormatOnTypeResult(
+    String content,
+    Matcher matcher,
+  ) async {
+    var code = TestCode.parseNormalized(content);
+
+    // We use range to make it really explicit which character is typed and not
+    // have to worry about whether it's before or after the ^.
+    expect(code.ranges, hasLength(1));
+    expect(code.positions, hasLength(anyOf(0, 1)));
+
+    var typedCharacter = code.code.substring(
+      code.range.sourceRange.offset,
+      code.range.sourceRange.end,
+    );
+    await initialize();
+    await openFile(mainFileUri, code.code);
+
+    var result = await formatOnType(
+      mainFileUri,
+      code.positions.isNotEmpty ? code.position.position : code.range.range.end,
+      typedCharacter,
+    );
+
+    expect(
+      result,
+      matcher,
+      reason:
+          'Typing the character "$typedCharacter" at the marked location '
+          'should match the supplied matcher',
+    );
+
+    return result;
+  }
+
   Future<List<TextEdit>> expectFormattedContents(
     Uri uri,
     String original,
@@ -193,30 +233,100 @@ ErrorOr<Pair<A, List<B>>> c(String d, List<Either2<E, F>> g, {h = false}) {}
     expect(registration(Method.textDocument_rangeFormatting), isNotNull);
   }
 
-  Future<void> test_formatOnType_simple() async {
+  Future<void> test_formatOnType_bad_brace_inComment() async {
+    await expectFormatOnTypeResult('''
+void f  ()
+{
+    // hello[!}!]
+    print('test');
+}
+    ''', isNull);
+  }
+
+  Future<void> test_formatOnType_bad_brace_inString() async {
+    await expectFormatOnTypeResult('''
+void f  ()
+{
+
+    print('te[!}!]st');
+}
+    ''', isNull);
+  }
+
+  Future<void> test_formatOnType_bad_semicolon_inComment() async {
+    await expectFormatOnTypeResult('''
+void f  ()
+{
+    // hello[!;!]
+    print('test');
+}
+    ''', isNull);
+  }
+
+  Future<void> test_formatOnType_bad_semicolon_inString() async {
+    await expectFormatOnTypeResult('''
+void f  ()
+{
+
+    print('te[!;!]st');
+}
+    ''', isNull);
+  }
+
+  Future<void> test_formatOnType_good_brace_blockEnd() async {
+    await expectFormatOnTypeResult('''
+void f  ()
+{
+    print('test');
+[!}!]
+    ''', isNotNull);
+  }
+
+  Future<void> test_formatOnType_good_formatsCode() async {
     var code = TestCode.parseNormalized('''
 void f  ()
 {
 
     print('test');
-^}
+[!}!]
     ''');
     var expected = '''void f() {
   print('test');
 }
 ''';
-    await initialize();
-    await openFile(mainFileUri, code.code);
-
-    var formatEdits = (await formatOnType(
-      mainFileUri,
-      code.position.position,
-      '}',
+    var formatEdits = (await expectFormatOnTypeResult(
+      code.markedCode,
+      isNotNull,
     ))!;
     var formattedContents = applyTextEdits(code.code, formatEdits);
     expect(formattedContents, equalsNormalized(expected));
   }
 
+  Future<void> test_formatOnType_good_positionAtEnd() async {
+    await expectFormatOnTypeResult('''
+void f  ()
+{
+[!}^!]
+    ''', isNotNull);
+  }
+
+  Future<void> test_formatOnType_good_positionAtOffset() async {
+    await expectFormatOnTypeResult('''
+void f  ()
+{
+[!^}!]
+    ''', isNotNull);
+  }
+
+  Future<void> test_formatOnType_good_semicolon_statementEnd() async {
+    await expectFormatOnTypeResult('''
+void f  ()
+{
+    print('test')[!;!]
+}
+    ''', isNotNull);
+  }
+
   Future<void> test_formatRange_editsOverlapRange() async {
     // Only ranges that are fully contained by the range should be applied,
     // not those that intersect the start/end.

pkg/analysis_server/test/lsp_over_legacy/format_test.dart

@@ -32,7 +32,8 @@ class FormatTest extends LspOverLegacyTest {
     newFile(testFilePath, content);
     await waitForTasksFinished();
 
-    var edits = await formatOnType(testFileUri, startOfDocPos, '}');
+    var position = positionFromOffset(content.indexOf('}'), content);
+    var edits = await formatOnType(testFileUri, position, '}');
     var formattedContents = applyTextEdits(content, edits!);
     expect(formattedContents.trimRight(), equals(expectedContent));
   }