Emit diagnostics for missing nodes #739

ahoppen · 2022-09-08T14:49:29Z

Re-opening #657 without dependencies

Previously, we weren’t emitting any errors for the missing layout nodes (RawMissingExprSyntax etc.)

ahoppen · 2022-09-20T20:34:58Z

bnbarham · 2022-09-09T00:09:43Z

Sources/SwiftParser/Declarations.swift

@@ -13,7 +13,7 @@
 @_spi(RawSyntax) import SwiftSyntax

 extension TokenConsumer {
-  func atStartOfDeclaration(isAtTopLevel: Bool = false, allowRecovery: Bool = false) -> Bool {
+  func atStartOfDeclaration(isAtTopLevel: Bool = false, allowRecovery: Bool = false, allowCase: Bool = false) -> Bool {


bnbarham · 2022-09-09T00:23:40Z

Sources/SwiftParser/Diagnostics/ParserDiagnosticMessages.swift

+    if let parent = missingNode.parent,
+        let childName = parent.childNameForDiagnostics(missingNode.index) {
+      message = "Expected \(childName)"
+      if let parent = missingNode.parent,


Already have parent

bnbarham · 2022-09-09T00:27:48Z

Sources/SwiftParser/Expressions.swift

+          let element = RawArrayElementSyntax(
+            expression: el, trailingComma: comma, arena: self.arena
+          )
+          if element.raw.byteLength == 0 {


Could add a .isEmpty?

bnbarham · 2022-09-09T00:40:40Z

Tests/SwiftParserTest/Attributes.swift

@@ -11,6 +11,7 @@ final class AttributeTests: XCTestCase {
      }
      """,
      diagnostics: [
+        DiagnosticSpec(locationMarker: "DIAG_2", message: "Expected declaration"),
        DiagnosticSpec(locationMarker: "DIAG_1", message: "Expected 'for' in attribute argument"),
        DiagnosticSpec(locationMarker: "DIAG_1", message: "Expected ':' in attribute argument"),
        DiagnosticSpec(locationMarker: "DIAG_2", message: "Expected ')' to end attribute"),


Is there an issue already for not eating func here?

Now we do: #821

bnbarham · 2022-09-09T00:46:32Z

Tests/SwiftParserTest/Attributes.swift

+        DiagnosticSpec(message: "Expected declaration"),
+        DiagnosticSpec(message: "Expected ':' in attribute argument"),
+        DiagnosticSpec(message: "Expected ')' to end attribute"),


What do you think about special-casing trailing missing nodes? Ie. If a parent has multiple sequential missing nodes, instead use Expected complete <parent>. Open to ideas for "complete" there. Ideally pretty printing (which we don't have yet) could then output an example/grammar/something else as well 🤔.

I'd also expect an after attribute for Expected declaration, ie. Expected declaration after attribute + for it to be after the attribute diagnostics.

What do you think about special-casing trailing missing nodes? Ie. If a parent has multiple sequential missing nodes, instead use Expected complete <parent>. Open to ideas for "complete" there. Ideally pretty printing (which we don't have yet) could then output an example/grammar/something else as well 🤔.

Let’s do this in a follow-up PR because it might be non-trivial. #822

I'd also expect an after attribute for Expected declaration, ie. Expected declaration after attribute +

Done

for it to be after the attribute diagnostics.

This is non-trivial because we can’t just order the diagnostics by their indexInTree anymore. Let’s consider do it in a follow-up PR. #823

bnbarham · 2022-09-09T00:51:27Z

Sources/SwiftParser/Diagnostics/ParserDiagnosticMessages.swift

+      if let lastChild = missingNode.lastToken(viewMode: .fixedUp), lastChild.presence == .present {
+        message += " after '\(lastChild.text)'"
+      } else if let previousToken = missingNode.previousToken(viewMode: .fixedUp), previousToken.presence == .present {
+        message += " after '\(previousToken.text)'"


If previousToken.parent != missingNode.parent, could we use the parent of the previous token rather than the token itself? That would give both the attribute tests Expected declaration after attribute rather than empty/')', but still keep after <token> for cases where it makes sense (eg. a ? b :).

Actually speaking of the ternary case, shouldn't it have in ternary expression at the end of it?

If previousToken.parent != missingNode.parent, could we use the parent of the previous token rather than the token itself? That would give both the attribute tests Expected declaration after attribute rather than empty/')', but still keep after <token> for cases where it makes sense (eg. a ? b :).

after attribute should now be added based.

Actually speaking of the ternary case, shouldn't it have in ternary expression at the end of it?

It doesn’t have in ternary expression at the end of it because the expression is not missing inside the ternary expression but in the sequence expression. For a ? b : the syntax tree looks like

IdentifierExpr: a

UnresolvedTernaryExpr: ? b :

MissingExpr

Filed #824 so we can take care of it later.

bnbarham · 2022-09-09T01:02:01Z

Tests/SwiftParserTest/Expressions.swift

@@ -396,27 +415,35 @@ final class ExpressionTests: XCTestCase {
    AssertParse(
      "foo ? 1#^DIAG^#",
      diagnostics: [
-        DiagnosticSpec(message: "Expected ':' after '? ...' in ternary expression")
+        DiagnosticSpec(message: "Expected ':' after '? ...' in ternary expression"),
+        DiagnosticSpec(message: "Expected expression"),


Expanding on merging the missing trailing nodes, maybe we could list the missing nodes as well? Expected complete ternary expression, missing ':' and expression. I admit that sounds a little weird though 🤔

Let’s take care of this as part of merging diagnostics: #822

bnbarham · 2022-09-09T01:06:09Z

Tests/SwiftParserTest/Expressions.swift

+        DiagnosticSpec(locationMarker: "KEY_PATH_2", message: "Expected expression of key path"),
+        DiagnosticSpec(locationMarker: "END", message: #"Expected '"' in string literal"#),
+        DiagnosticSpec(locationMarker: "END", message: "Expected '}' to end 'if' statement"),
+        DiagnosticSpec(locationMarker: "END", message: "Expected '}' to end function"),


Unrelated to the PR - we need to add notes and test those as well at some point (eg. I'd expect a function starts here note).

Filed #825 so we can do it later.

bnbarham · 2022-09-20T20:19:33Z

Sources/SwiftSyntax/gyb_generated/SyntaxEnum.swift

@@ -486,7 +486,7 @@ public enum SyntaxEnum {
    case .parameterClause:
      return "parameter clause"
    case .returnClause:
-      return "return clause"


What diagnostic did you not want to have return clause? IMO diagnostics with "after return" do make sense.

struct Foo { subscript(x: String) {} }

If we keep return clause as a node name we emit

Expected return type in return clause

instead of

Expected return type in subscript

I liked the second better than the first.

Oh sorry, it's the return clause within a declaration. Sure, definitely agree with you there 👍

bnbarham · 2022-09-20T20:41:35Z

Sources/generate-swift-syntax-builder/gyb_syntax_support/ExprNodes.py

@@ -618,13 +618,13 @@
    Node('KeyPathExpr', name_for_diagnostics='key path', kind='Expr',
         children=[
             Child('Backslash', kind='BackslashToken'),
-             Child('RootExpr', kind='Expr', is_optional=True,
+             Child('RootExpr', kind='Expr', name_for_diagnostics='root', is_optional=True,


I haven't gone through all of these (do you want me to?) but should we be careful to use the swift.org language here? Ie. type name and key path components?

Also, are child names for optional nodes useful? Would they ever actually be used?

I haven't gone through all of these (do you want me to?) but should we be careful to use the swift.org language here? Ie. type name and key path components?

Honestly, I didn’t put too much effort into this (still took me 30-60 minutes to create these annotations). If you’ve got an idea how to create better names in a principled way, I’d be open to suggestions. Also, we can always update them later, so I don’t think it has to be blocking this PR.

My idea was that we update these as we discover diagnostics that we don’t like…

Also, are child names for optional nodes useful? Would they ever actually be used?

They aren’t being used right now but they don’t hurt either. And maybe we’ll come up with a use case later.

Fair enough.

… an expression

ahoppen · 2022-09-21T16:57:05Z

@swift-ci Please test

ahoppen requested review from rintaro, CodaFi and bnbarham September 8, 2022 14:49

ahoppen marked this pull request as ready for review September 8, 2022 14:49

ahoppen force-pushed the ahoppen/diagnose-missing-nodes branch 2 times, most recently from cfa069b to 6422f11 Compare September 9, 2022 05:00

ahoppen mentioned this pull request Sep 20, 2022

Fix loop breaking condition for dictionary literal parsing. #812

Closed

ahoppen force-pushed the ahoppen/diagnose-missing-nodes branch from 6422f11 to e9cd304 Compare September 20, 2022 13:55

ahoppen added 6 commits September 20, 2022 22:31

Remove FIXMEs that have already been handled

12ed1bd

Allow specifying a custom position for diagnostics

08c99fd

If the location of a diagnostic didn't match, emit its content

ae00785

Don’t add entirely missing elements to collection literals

5731968

Add name for diagnostics for syntax node child

b8ed226

Emit diagnostics for missing nodes

ce30583

ahoppen force-pushed the ahoppen/diagnose-missing-nodes branch from e9cd304 to bf5e9e8 Compare September 20, 2022 20:34

bnbarham reviewed Sep 20, 2022

View reviewed changes

Resolve issue where standalone return statemente incorrectly expected…

509f005

… an expression

Address review comments

cecf603

ahoppen force-pushed the ahoppen/diagnose-missing-nodes branch from bf5e9e8 to cecf603 Compare September 21, 2022 09:32

bnbarham approved these changes Sep 21, 2022

View reviewed changes

ahoppen merged commit acd3067 into swiftlang:main Sep 21, 2022

ahoppen deleted the ahoppen/diagnose-missing-nodes branch September 21, 2022 19:21

Emit diagnostics for missing nodes #739

Emit diagnostics for missing nodes #739

Uh oh!

Conversation

ahoppen commented Sep 8, 2022

Uh oh!

ahoppen commented Sep 20, 2022

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 Sep 21, 2022 • edited 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!

ahoppen Sep 21, 2022 • edited 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!

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 21, 2022

Uh oh!

Uh oh!

ahoppen Sep 21, 2022 •

edited

Loading

ahoppen Sep 21, 2022 •

edited

Loading