Merge missing token diagnostics if they occur at the same source location #838

ahoppen · 2022-09-23T18:12:02Z

No description provided.

bnbarham · 2022-09-23T20:06:27Z

Sources/SwiftParser/Diagnostics/ParseDiagnosticsGenerator.swift

+      fixIts = []
+    }
+
+    // Find the previous sibling, skipping over siblings that don't actually contain any source text.


Can't we just use .sourceAccurate here rather than checking for missing?

Rather than grabbing previous siblings and replacing previous diagnostics, could we instead grab next siblings and just create the one diagnostic at this point? That would avoid looking through all diagnostics each time. IMO we also want to merge the fix-its, which that change would also make easier.

We may want to start splitting these files up as well. Ie. the generation + message for this one could be defined in a MissingSyntaxDiagnostic.swift instead (it's a fair amount of code).

Can't we just use .sourceAccurate here rather than checking for missing?

No, because then we wouldn’t find missing tokens, which are skipped in the .sourceAccurate traversal mode.

Rather than grabbing previous siblings and replacing previous diagnostics, could we instead grab next siblings and just create the one diagnostic at this point? That would avoid looking through all diagnostics each time. IMO we also want to merge the fix-its, which that change would also make easier.

Good idea. I thought that this would be more difficult than looking behind but it turns out that it’s actually simpler.

We may want to start splitting these files up as well. Ie. the generation + message for this one could be defined in a MissingSyntaxDiagnostic.swift instead (it's a fair amount of code).

Good idea.

bnbarham · 2022-09-23T20:16:57Z

Sources/SwiftParser/Diagnostics/ParserDiagnosticMessages.swift

-      }
+private extension String {
+  /// Remove any leading or trailing whitespace
+  func trimmingWhitespaces() -> String {


Avoiding a Foundation import I assume? Crazy that we have drop and trimPrefix but not a trimSuffix even though drop and trimPrefix are basically equivalent. Also note that this isn't "whitespace" but rather "spaces".

Yes, exactly. I wished we had functionality like this in the stdlib but apparently we don’t 😬

Good catch with spaces vs whitespace

bnbarham · 2022-09-23T20:19:14Z

Sources/SwiftParser/Diagnostics/ParserDiagnosticMessages.swift

+  init(missingNodes: [Syntax]) {
+    assert(!missingNodes.isEmpty)
+    self.missingNodes = missingNodes
+    self.commonParent = missingNodes.first!.parent


In the spirit of not-crashing IMO it would be nice to avoid ! here and just return some generic message if they're empty.

bnbarham · 2022-09-23T20:31:37Z

Sources/SwiftParser/Diagnostics/ParserDiagnosticMessages.swift

+        let (rawKind, text) = missingToken.tokenKind.decomposeToRaw()
+        if let text = text, !text.isEmpty {
+          let presentToken = TokenSyntax(missingToken.tokenKind, presence: .present)
+          newPart = .sourceText("\(Syntax(Format().format(syntax: presentToken)))")


Would be nice to have an API where you don't have to instantiate Format first. IMO this could even just be on the syntax node's themselves (ie. presentToken.formatted()).

bnbarham · 2022-09-23T20:32:46Z

Sources/SwiftParser/Diagnostics/ParserDiagnosticMessages.swift

+
+      var description: String {
+        switch self {
+        case .sourceText(let content):


Could take a TokenSyntax instead. Both uses pass the same "\(Syntax(Format().format(syntax: presentToken)))".

It can’t take a TokenSyntax because we are merging these further below. But [TokenSyntax] is possible. I changed it to that.

bnbarham · 2022-09-23T21:14:27Z

Sources/SwiftParser/Diagnostics/ParserDiagnosticMessages.swift

+      switch missingToken.tokenKind {
+      case .rightBrace, .rightAngle, .rightParen, .rightSquareBracket:
+        if commonParent.children(viewMode: .fixedUp).last?.as(TokenSyntax.self) == missingToken {
+          if nodePosition == .toStart {


in is the default, just if nodePosition != .toStart?

I refactored this logic quite a bit to make it simpler so it doesn’t use ContextType anymore.

bnbarham · 2022-09-23T21:15:35Z

Sources/SwiftParser/Diagnostics/ParserDiagnosticMessages.swift

      }
    }
-    return message
+    if let commonParent = commonParent, let parentTypeName = commonParent.nodeTypeNameForDiagnostics(allowSourceFile: false) {


Can remove this check with the guard above (and then all the commonParent checks as well)

bnbarham · 2022-09-23T21:25:41Z

Sources/SwiftParser/Diagnostics/ParserDiagnosticMessages.swift

+  public let attributeName: TokenSyntax
+
+  public var message: String {
+    return "Expected argument for '@\(attributeName)' attribute"


argument or arguments?

I think based on the way we currently model attributes we don’t really know whether a particular attribute requires a single argument or multiple… Might be something to tackle in a follow-up PR.

argument(s) 😅

bnbarham · 2022-09-23T21:38:11Z

Sources/SwiftSyntax/Syntax.swift

+    let nextSiblingIndex = siblings.index(after: self.index)
+    if let previousAbsoluteRawSibling = siblings[nextSiblingIndex...].last {
+      return Syntax(SyntaxData(previousAbsoluteRawSibling, parent: parent))
+    } else {
+      return nil
+    }
+  }


Probably worth adding some quick tests for these - this should be .first.

After making some changes, it turns out that we don’t need previousSibling and nextSibling anymore.

bnbarham · 2022-09-23T21:59:19Z

Tests/SwiftParserTest/Declarations.swift

+        DiagnosticSpec(message: "Expected 'set)' to end modifier"),
        DiagnosticSpec(message: "Unexpected text 'get, didSet' in variable")


Probably worth specializing this one.

What would you expect there?

Ideally 'private' only accepts 'set' with a replacement for get, didSet -> set and then a Expected ')' to end modifier. And I'd only expect that if the unexpected was one of get/didSet/willSet.

This is fine for now, could add an issue for it though if you agree.

bnbarham · 2022-09-23T22:01:25Z

Tests/SwiftParserTest/Declarations.swift

-        DiagnosticSpec(locationMarker: "EXPECTED_DECL", message: "Expected declaration after '{' in struct"),
+        DiagnosticSpec(locationMarker: "EXPECTED_DECL", message: "Expected declaration in struct"),


I think I agree with doing this, especially since the location would be the "after" anyway (though I suppose not all IDEs display the actual column).

bnbarham · 2022-09-23T22:02:08Z

Tests/SwiftParserTest/Expressions.swift

@@ -395,8 +395,8 @@ final class ExpressionTests: XCTestCase {
      \#^AFTER_SLASH^#\(#^AFTER_PAREN^#
      """##,
      diagnostics: [
-        DiagnosticSpec(locationMarker: "AFTER_SLASH", message: #"Expected root and expression after '\' in key path"#),
-        DiagnosticSpec(locationMarker: "AFTER_PAREN", message: "Expected value after '(' in tuple"),
+        DiagnosticSpec(locationMarker: "AFTER_SLASH", message: #"Expected root and expression in key path"#),


Is there an issue up for key paths in general? root isn't required, but it looks like we're calling parsePostfixExpression if there's no ., that returns missing, and thus we get an expected "root" here where that makes no sense (the type isn't required, really I'd expect "Expected '.' and expression in key path" here).

bnbarham · 2022-09-23T22:11:02Z

Tests/SwiftParserTest/Expressions.swift

@@ -467,7 +467,7 @@ final class ExpressionTests: XCTestCase {
                diagnostics: [
                  DiagnosticSpec(message: "Expected identifier in '#keyPath' expression"),
                  DiagnosticSpec(message: "Expected ')' to end '#keyPath' expression"),
-                  DiagnosticSpec(locationMarker: "MISSING_VALUE", message: "Expected value after ':' in function call"),
+                  DiagnosticSpec(locationMarker: "MISSING_VALUE", message: "Expected value in function call"),


Ideally this would be "argument" but that's not particularly easy with the current scheme 🤔.

bnbarham · 2022-09-23T22:17:52Z

Tests/SwiftParserTest/Expressions.swift

@@ -476,7 +476,7 @@ final class ExpressionTests: XCTestCase {
      "[(Int) -> #^DIAG^#throws Int]()",
      diagnostics: [
        // FIXME: We should suggest to move 'throws' in front of '->'
-        DiagnosticSpec(message: "Expected expression after '->' in array element"),
+        DiagnosticSpec(message: "Expected expression in array element"),


Ignoring the throws problem, this not being something like "expected return type" is also an issue.

bnbarham · 2022-09-23T22:18:31Z

Tests/SwiftParserTest/Expressions.swift

        DiagnosticSpec(locationMarker: "DIAG_1", message: "Unexpected text '..' in function type"),
-        DiagnosticSpec(locationMarker: "END", message: "Expected type after '->' in function type"),
+        DiagnosticSpec(locationMarker: "END", message: "Expected type in function type"),


Similar to the above, without after '->' this would really need to be expected return type.

bnbarham · 2022-09-26T16:55:33Z

Tests/SwiftParserTest/Declarations.swift

+        DiagnosticSpec(message: "Expected 'set)' to end modifier"),
        DiagnosticSpec(message: "Unexpected text 'get, didSet' in variable")


Ideally 'private' only accepts 'set' with a replacement for get, didSet -> set and then a Expected ')' to end modifier. And I'd only expect that if the unexpected was one of get/didSet/willSet.

This is fine for now, could add an issue for it though if you agree.

bnbarham · 2022-09-26T17:03:39Z

Sources/SwiftParser/Diagnostics/ParserDiagnosticMessages.swift

+  public let attributeName: TokenSyntax
+
+  public var message: String {
+    return "Expected argument for '@\(attributeName)' attribute"


argument(s) 😅

bnbarham · 2022-09-26T17:14:07Z

Tests/SwiftParserTest/Attributes.swift

@@ -6,16 +6,19 @@ final class AttributeTests: XCTestCase {
  func testMissingArgumentToAttribute() {
    AssertParse(
      """
-      @_dynamicReplacement(#^DIAG_1^#
+      @_dynamicReplacement(#^DIAG^#
      func #^DIAG_2^#test_dynamic_replacement_for2() {


Can remove DIAG_2

bnbarham · 2022-09-26T17:15:39Z

Tests/SwiftParserTest/Attributes.swift

+        DiagnosticSpec(locationMarker: "DIAG_2", message: "Expected ')' to end attribute", fixIts: ["Insert ')'"]),
+      ],
+      fixedSource: """
+        @differentiable(reverse wrt: <#syntax#>,where T = <#type#>)


Shouldn't this be 'parameters'? Not much better than 'syntax` though 🤔

bnbarham · 2022-09-26T17:18:16Z

Tests/SwiftParserTest/Declarations.swift

+        DiagnosticSpec(locationMarker: "DIAG_2", message: "Expected member block in class"),
+      ],
+      fixedSource: """
+        }class C {}


Shouldn't this be removing the first }?

bnbarham · 2022-09-26T17:58:37Z

Sources/SwiftParser/Diagnostics/DiagnosticExtensions.swift

@@ -0,0 +1,38 @@
+//===--- DiagnosticExtensions.swift ---------------------------------------===//


Need to add all the new files to CMakeLists.txt?

…tion

ahoppen · 2022-09-27T07:25:45Z

@swift-ci Please test

ahoppen force-pushed the ahoppen/merge-diags branch from 9a20d2f to 3ad5464 Compare September 23, 2022 18:12

bnbarham reviewed Sep 23, 2022

View reviewed changes

ahoppen force-pushed the ahoppen/merge-diags branch 2 times, most recently from 106d379 to d4bab52 Compare September 26, 2022 13:29

bnbarham approved these changes Sep 26, 2022

View reviewed changes

bnbarham reviewed Sep 26, 2022

View reviewed changes

Merge missing token diagnostics if they occur at the same source loca…

1289d74

…tion

ahoppen force-pushed the ahoppen/merge-diags branch from d4bab52 to 1289d74 Compare September 27, 2022 07:25

ahoppen marked this pull request as ready for review September 27, 2022 07:25

ahoppen merged commit 8a4fa32 into swiftlang:main Sep 27, 2022

ahoppen deleted the ahoppen/merge-diags branch September 27, 2022 08:43

		DiagnosticSpec(message: "Expected 'set)' to end modifier"),
		DiagnosticSpec(message: "Unexpected text 'get, didSet' in variable")

		DiagnosticSpec(locationMarker: "EXPECTED_DECL", message: "Expected declaration after '{' in struct"),
		DiagnosticSpec(locationMarker: "EXPECTED_DECL", message: "Expected declaration in struct"),

		@@ -0,0 +1,38 @@
		//===--- DiagnosticExtensions.swift ---------------------------------------===//

Merge missing token diagnostics if they occur at the same source location #838

Merge missing token diagnostics if they occur at the same source location #838

Uh oh!

Conversation

ahoppen commented Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnbarham Sep 26, 2022 • edited by ahoppen Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnbarham Sep 26, 2022 • edited by ahoppen Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Sep 27, 2022

Uh oh!

Uh oh!

ahoppen commented Sep 23, 2022 •

edited

Loading

bnbarham Sep 26, 2022 •

edited by ahoppen

Loading

bnbarham Sep 26, 2022 •

edited by ahoppen

Loading