Typed throws #2219

DougGregor · 2023-09-19T17:38:46Z

Parsing of typed throw specifiers. See swiftlang/swift#68629 for more information.

ahoppen

All of this should be behind an experimental language feature. Let me know if you need help with that as you get closer to actually wanting to merge it.

DougGregor · 2023-09-22T19:11:11Z

All of this should be behind an experimental language feature.

Great point, and it was super-easy to do. Thanks!

ahoppen · 2023-09-25T18:30:00Z

CodeGeneration/Sources/SyntaxSupport/CommonNodes.swift

+    kind: .thrownType,
+    base: .syntax,
+    nameForDiagnostics: "thrown type",


Can you mark this node as experimental? Similar to https://github.com/apple/swift-syntax/blob/7c6d4f729f37f5e5a206b39ffa7bc1fac9479c24/CodeGeneration/Sources/SyntaxSupport/StmtNodes.swift#L590

ahoppen · 2023-09-25T18:30:28Z

CodeGeneration/Sources/SyntaxSupport/CommonNodes.swift

+      Child(
+        name: "thrownType",
+        kind: .node(kind: .thrownType),
+        isOptional: true
+      ),


We need to mark this child as experimental. There is no infrastructure for it yet, though. Could you write it?

Same for all the other thrownType children.

Could you add documentation for these children?

ahoppen · 2023-09-25T18:34:28Z

CodeGeneration/Sources/SyntaxSupport/Traits.swift

@@ -56,7 +56,13 @@ public let TRAITS: [Trait] = [
      Child(name: "asyncSpecifier", kind: .token(choices: [.keyword(.async), .keyword(.reasync)]), isOptional: true),
      Child(name: "unexpectedBetweenAsyncSpecifierAndThrowsSpecifier", kind: .node(kind: .unexpectedNodes), isOptional: true),
      Child(name: "throwsSpecifier", kind: .token(choices: [.keyword(.throws), .keyword(.rethrows)]), isOptional: true),
-      Child(name: "unexpectedAfterThrowsSpecifier", kind: .node(kind: .unexpectedNodes), isOptional: true),
+      Child(name: "unexpectedBetweenThrowsSpecifierAndThrownType", kind: .node(kind: .unexpectedNodes), isOptional: true),


This is an API-breaking change that we should mention in the API-Incompatible Changes section of the release notes. I think it’s fine to break the API here since it’s unlikely that this property is in considerable use.

ahoppen · 2023-09-25T18:36:20Z

CodeGeneration/Sources/SyntaxSupport/CommonNodes.swift

@@ -79,6 +79,27 @@ public let COMMON_NODES: [Node] = [
    ]
  ),

+  Node(
+    kind: .thrownType,
+    base: .syntax,


Could you add a doc comments for this node and its children?

ahoppen · 2023-09-25T18:37:50Z

CodeGeneration/Sources/SyntaxSupport/CommonNodes.swift

@@ -79,6 +79,27 @@ public let COMMON_NODES: [Node] = [
    ]
  ),

+  Node(
+    kind: .thrownType,


The more consistent naming of this node would be thrownTypeClause in my opinion since it’s more than just the type (it also has parentheses).

ahoppen · 2023-09-25T18:41:42Z

Sources/SwiftParser/Specifiers.swift

+    if throwsKeyword != nil && self.at(.leftParen) && experimentalFeatures.contains(.typedThrows) {
+      thrownType = parseThrownType()
+    }


IIUC this would also try to parse a thrown type for try async(MyError), which I think is confusing. IMO we should parse the thrown type whenever we set the throwsKeyword

Sources/SwiftParser/Specifiers.swift

ahoppen · 2023-09-25T18:44:45Z

Sources/SwiftParser/Types.swift

+      if effect.spec.isThrowsSpecifier && self.peek().rawTokenKind == .leftParen {
+        var backtrack = self.lookahead()
+        backtrack.consumeAnyToken()
+        backtrack.skipSingle()
+        return backtrack.atFunctionTypeArrow()
+      }


I think this doesn’t handle async throws(MyError) because the current token is async here, which means that we end up in the if below, which just consumes throws but not MyError.

The consumeAnyToken() consumes the throws, and the skipSingle consumes a parenthesized expression.

No, it doesn’t. The following test fails to parse

assertParse( "[() async throws(MyError) -> Void]()" )

ahoppen · 2023-09-25T18:47:43Z

Sources/SwiftParserDiagnostics/ParseDiagnosticsGenerator.swift

@@ -279,9 +279,9 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor {
      }
    }

-    if let throwsSpecifier = node.throwsSpecifier {
+    if let throwsSpecifier = node.throwsSpecifier, node.thrownType == nil {


Why do we need to check that thrownType is nil here? We should be able to handle throws(MyError) async as well.

We don't need to do the check

ahoppen · 2023-09-25T18:55:46Z

Tests/SwiftParserTest/TypeTests.swift

+      "[() throws(PosixError) -> Void]()",
+      experimentalFeatures: [.typedThrows]
+    )
+  }


I think the following test cases would be interesting as well. I have not filled in the expected diagnostics here but we should make sure that they make sense

Function parsing.

assertParse( "func test() throws(MyError) async {}" ) assertParse( "func test() throws async(MyError) {}" ) assertParse( "func test() try(MyError) async {}" ) assertParse( "func test() try async(MyError) {}" ) assertParse( "func test() throws(MyError) await {}" ) assertParse( "func test() throws await(MyError) {}" ) assertParse( "func test() try(MyError) await {}" ) assertParse( "func test() try await(MyError) {}" ) assertParse( "func test() async(MyError) {}" ) assertParse( "func test() await(MyError) {}" ) assertParse( "func test() try(MyError) {}" ) assertParse( "func test() throws(MyError) {}" ) assertParse( "func test() throws(MyError)async {}" // no space between closing parenthesis and `async` )

isAtFunctionTypeArrow disambiguation

assertParse( "[() throws(MyError) async -> Void]()" ) assertParse( "[() throws async(MyError) -> Void]()" ) assertParse( "[() try(MyError) async -> Void]()" ) assertParse( "[() try async(MyError) -> Void]()" ) assertParse( "[() throws(MyError) await -> Void]()" ) assertParse( "[() throws await(MyError) -> Void]()" ) assertParse( "[() try(MyError) await -> Void]()" ) assertParse( "[() try await(MyError) -> Void]()" ) assertParse( "[() async(MyError) -> Void]()" ) assertParse( "[() await(MyError) -> Void]()" ) assertParse( "[() try(MyError) -> Void]()" ) assertParse( "[() throws(MyError) -> Void]()" )

Great test ideas, thanks!

DougGregor · 2023-09-29T21:22:02Z

swiftlang/swift#68629

@swift-ci please test

bnbarham · 2023-09-29T21:30:24Z

Tests/SwiftParserTest/DeclarationTests.swift

+        DiagnosticSpec(locationMarker: "1️⃣", message: "'async' must precede 'throws'", fixIts: ["move 'async' in front of 'throws'"]),
+        DiagnosticSpec(locationMarker: "2️⃣", message: "unexpected code '(MyError)' in function"),
+      ],
+      fixedSource: "func test() async throws (MyError) {}",


@ahoppen are we missing removing (MyError) here because they end up intersecting?

I think it's grabbing both the async and the unexpected nodes after the async and moving them forward.

That’s intentional and behaves correctly. (MyError) gets parsed as unexpected tokens (which is fine, I think). And we don’t provide a Fix-It to remove unexpected tokens because most likely you wrote them for a reason and just removing something you wrote should not be a default fixing strategy.

bnbarham · 2023-09-29T21:32:58Z

Tests/SwiftParserTest/DeclarationTests.swift

+      "func test() 1️⃣try2️⃣(MyError) async {}",
+      diagnostics: [
+        DiagnosticSpec(locationMarker: "1️⃣", message: "expected throwing specifier; did you mean 'throws'?", fixIts: ["replace 'try' with 'throws'"]),
+        DiagnosticSpec(locationMarker: "2️⃣", message: "unexpected code '(MyError) async' in function"),


I'm surprised this isn't handled as it is above when try is the proper throws or below where try is before async, ie. that async is unexpected here.

It's because I am not parsing the thrown type clause along a recovery path, I think.

We should parse the thrown type clause along the recovery path.

bnbarham · 2023-09-29T21:34:59Z

Tests/SwiftParserTest/DeclarationTests.swift

+    assertParse(
+      "func test() throws(MyError) 1️⃣await {}",
+      diagnostics: [
+        DiagnosticSpec(locationMarker: "1️⃣", message: "'await' must precede 'throws'", fixIts: ["move 'await' in front of 'throws'"])


No expected async specifier; did you mean 'async'? 🤔? The fixed source is correct though so... that's good. This is the case for a few of the tests.

There's some comments in the diagnostics generation about how the later errors subsume the earlier ones.

This should be relatively easy to fix by changing the creation of missingToken for misspelled tokens in parseEffectsSpecifier like

asyncKeyword = missingToken(misspelledAsync.tokenKind, text: misspelledAsync.tokenText)

bnbarham · 2023-09-29T21:37:20Z

Tests/SwiftParserTest/DeclarationTests.swift

+      diagnostics: [
+        DiagnosticSpec(message: "'async' must precede 'throws'", fixIts: ["move 'async' in front of 'throws'"])
+      ],
+      fixedSource: "func test() async throws(MyError){}",


Very nitpicky, but it would be good to keep the space before the {.

DougGregor · 2023-09-30T00:30:51Z

swiftlang/swift#68629

@swift-ci please test

DougGregor · 2023-09-30T00:42:37Z

swiftlang/swift#68629

@swift-ci please test Windows

DougGregor · 2023-09-30T05:42:16Z

swiftlang/swift#68629

@swift-ci please test

DougGregor · 2023-09-30T05:43:06Z

swiftlang/swift#68629

@swift-ci please test

DougGregor · 2023-09-30T06:05:56Z

swiftlang/swift#68629

@swift-ci please test

DougGregor · 2023-09-30T06:06:29Z

swiftlang/swift#68629

@swift-ci please test Windows

DougGregor · 2023-09-30T15:43:51Z

swiftlang/swift#68629

@swift-ci please test

DougGregor · 2023-09-30T15:44:09Z

swiftlang/swift#68629

@swift-ci please test Windows

DougGregor · 2023-09-30T15:55:47Z

swiftlang/swift#68629

@swift-ci please test Linux

ahoppen · 2023-10-02T18:32:08Z

CodeGeneration/Sources/SyntaxSupport/CommonNodes.swift

@@ -79,6 +79,34 @@ public let COMMON_NODES: [Node] = [
    ]
  ),

+  Node(
+    kind: .thrownTypeClause,


You should mark this node as experimental.

ahoppen · 2023-10-02T18:37:12Z

Sources/SwiftParser/Types.swift

+      if effect.spec.isThrowsSpecifier && self.peek().rawTokenKind == .leftParen {
+        var backtrack = self.lookahead()
+        backtrack.consumeAnyToken()
+        backtrack.skipSingle()
+        return backtrack.atFunctionTypeArrow()
+      }


No, it doesn’t. The following test fails to parse

assertParse( "[() async throws(MyError) -> Void]()" )

ahoppen · 2023-10-02T18:37:46Z

Sources/SwiftParserDiagnostics/ParseDiagnosticsGenerator.swift

+      node.unexpectedBeforeAsyncSpecifier, node.unexpectedBetweenAsyncSpecifierAndThrowsSpecifier, node.unexpectedBetweenThrowsSpecifierAndThrownError,
+      node.unexpectedAfterThrownError,


Could you wrap this array literal so that there’s one element per line. It just reads nicer that way.

ahoppen · 2023-10-02T18:39:34Z

Sources/SwiftSyntax/SyntaxProtocol.swift

+
+extension EffectSpecifiersSyntax {
+  // Default implementation for the experimental thrownError child, which must
+  // be provided due to the use of @_spi.
+  @_spi(ExperimentalLanguageFeatures)
+  public var thrownError: ThrownTypeClauseSyntax? {
+    get { return nil }
+    set {}
+  }
+}


SyntaxProtocol.swift really doesn’t seem like the right place to put this. Also, AFAICT everything compiles fine even if I remove this.

ahoppen · 2023-10-02T20:44:47Z

Tests/SwiftParserTest/DeclarationTests.swift

+        DiagnosticSpec(locationMarker: "1️⃣", message: "'async' must precede 'throws'", fixIts: ["move 'async' in front of 'throws'"]),
+        DiagnosticSpec(locationMarker: "2️⃣", message: "unexpected code '(MyError)' in function"),
+      ],
+      fixedSource: "func test() async throws (MyError) {}",


That’s intentional and behaves correctly. (MyError) gets parsed as unexpected tokens (which is fine, I think). And we don’t provide a Fix-It to remove unexpected tokens because most likely you wrote them for a reason and just removing something you wrote should not be a default fixing strategy.

ahoppen · 2023-10-02T20:52:03Z

Tests/SwiftParserTest/DeclarationTests.swift

+    assertParse(
+      "func test() throws(MyError) 1️⃣await {}",
+      diagnostics: [
+        DiagnosticSpec(locationMarker: "1️⃣", message: "'await' must precede 'throws'", fixIts: ["move 'await' in front of 'throws'"])


This should be relatively easy to fix by changing the creation of missingToken for misspelled tokens in parseEffectsSpecifier like

asyncKeyword = missingToken(misspelledAsync.tokenKind, text: misspelledAsync.tokenText)

ahoppen · 2023-10-02T20:53:02Z

Tests/SwiftParserTest/DeclarationTests.swift

+      "func test() 1️⃣try2️⃣(MyError) await {}",
+      diagnostics: [
+        DiagnosticSpec(locationMarker: "1️⃣", message: "expected throwing specifier; did you mean 'throws'?", fixIts: ["replace 'try' with 'throws'"]),
+        DiagnosticSpec(locationMarker: "2️⃣", message: "unexpected code '(MyError) await' in function"),


We should be parsing (MyError) as a thrown type clause here or at least recover to parse await as a misspelled async keyword. No matter how we implement it, the Fix-It that should be produced, is to move await in front of try.

ahoppen · 2023-10-02T20:56:26Z

Tests/SwiftParserTest/DeclarationTests.swift

+        DiagnosticSpec(locationMarker: "1️⃣", message: "expected throwing specifier; did you mean 'throws'?", fixIts: ["replace 'try' with 'throws'"]),
+        DiagnosticSpec(locationMarker: "2️⃣", message: "unexpected code '(MyError)' in function"),
+      ],
+      fixedSource: "func test() throws (MyError) {}",


We should change BasicFormat to not insert a space here. BasicFormat.requiresWhitespace is then entry point where this is decided.

ahoppen · 2023-10-02T20:57:11Z

Tests/SwiftParserTest/DeclarationTests.swift

+      fixedSource: "func test() async throws(MyError) {}",
+      experimentalFeatures: [.typedThrows]
+    )
+    assertParse(


Could you add empty lines between the assertParse statements to make it a little easier to read?

ahoppen · 2023-10-02T21:01:28Z

Tests/SwiftParserTest/ExpressionTests.swift

+      experimentalFeatures: .typedThrows
+    )
+    assertParse(
+      "[() try(MyError) async -> Void]()",


This should not parse. This seems to be an existing issue in the new parser. I filed #2255

ahoppen · 2023-10-02T21:06:42Z

I’ve got a couple more comments in-line. On a higher level, the following still needs to be done.

The following should be SPI
- The ThrownTypeClauseSyntax type
- SyntaxAnyVisitor.visit(ThrownTypeClauseSyntax)
- SyntaxAnyVisitor.visitPost(ThrownTypeClauseSyntax)
- SyntaxEnum.thrownTypeClause
- SyntaxKind.thrownTypeClause
- SyntaxRewriter.visit(ThrownTypeClauseSyntax)
- SyntaxVisitor.visit(ThrownTypeClauseSyntax)
- SyntaxVisitor.visitPost(ThrownTypeClauseSyntax)
- The initializers of AccessorEffectSpecifiersSyntax etc. that take ThrownTypeClauseSyntax
- unexpectedBetweenThrowsSpecifierAndThrownError and unexpectedAfterThrownError
Provide unexpectedAfterThrowsSpecifier that concatenates unexpectedBetweenThrowsSpecifierAndThrownError, thrownError, and unexpectedAfterThrownError
Ensure that all the expression tests like [() try(MyError) async -> Void]() don’t parse once #2255 is fixed

@DougGregor Could you address my review comments in a follow-up PR?

ahoppen · 2023-10-04T19:55:33Z

We just discussed the design of the syntax tree and I think the tree should be structured as follows:

The effect specifier nodes contain a throwsClasue: ThrowsClauseSyntax child, instead of throwsSpecifier and thrownType
The ThrowsClauseSyntax has 4 children
- throwsSpecifier: TokenSyntax (which can be either throws or rethrows)
- leftParent: TokenSyntax?
- thrownType: TypeSyntax?
- rightParen: TokenSyntax?

ahoppen reviewed Sep 19, 2023

View reviewed changes

DougGregor force-pushed the typed-throws branch from 9c18353 to 7f39859 Compare September 22, 2023 19:10

DougGregor mentioned this pull request Sep 22, 2023

Typed throws swiftlang/swift#68629

Merged

21 tasks

ahoppen reviewed Sep 25, 2023

View reviewed changes

DougGregor added 10 commits September 29, 2023 10:15

[Typed throws] Add thrown type to effect specifiers

8e75584

Regenerate syntax tree

a602fdd

[Typed throws] Parse typed throws in effect specifiers

d6e4880

Introduce an experimental feature for typed throws

7557530

Add documentation and improve names for typed throws syntax nodes

93f2a15

Add support for experimental child nodes

fd475cb

Mark thrown type clauses as experimental

9e7cb5e

Regenerate

c595980

Update for new names

7af0beb

Improve parser recovery and diagnostics for typed throws

f3168d0

DougGregor force-pushed the typed-throws branch from 7f39859 to f3168d0 Compare September 29, 2023 17:15

Support building resilient libraries with @_spi default implementation

9d7be37

DougGregor marked this pull request as ready for review September 29, 2023 21:21

DougGregor requested a review from bnbarham as a code owner September 29, 2023 21:21

bnbarham reviewed Sep 29, 2023

View reviewed changes

DougGregor added 3 commits September 29, 2023 17:19

Address test failures be renaming child nodes and adding a trait

1df2e44

Regenerate

204162a

Adjust to renames

011ddd9

Regenerate

15ae54e

Finish fixup after regeneration

a3c2de2

Renames for consistency in EffectSpecifiers

b243260

DougGregor merged commit f974452 into swiftlang:main Sep 30, 2023

DougGregor deleted the typed-throws branch September 30, 2023 18:18

ahoppen reviewed Oct 2, 2023

View reviewed changes

		node.unexpectedBeforeAsyncSpecifier, node.unexpectedBetweenAsyncSpecifierAndThrowsSpecifier, node.unexpectedBetweenThrowsSpecifierAndThrownError,
		node.unexpectedAfterThrownError,

Typed throws #2219

Typed throws #2219

Uh oh!

Conversation

DougGregor commented Sep 19, 2023

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

DougGregor commented Sep 22, 2023

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!

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

Function parsing.

isAtFunctionTypeArrow disambiguation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DougGregor commented Sep 29, 2023

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!

DougGregor commented Sep 30, 2023

Uh oh!

DougGregor commented Sep 30, 2023

Uh oh!

DougGregor commented Sep 30, 2023

Uh oh!

DougGregor commented Sep 30, 2023

Uh oh!

DougGregor commented Sep 30, 2023

Uh oh!

DougGregor commented Sep 30, 2023

Uh oh!

DougGregor commented Sep 30, 2023

Uh oh!

DougGregor commented Sep 30, 2023

Uh oh!

DougGregor commented Sep 30, 2023

Uh oh!

`isAtFunctionTypeArrow` disambiguation

ahoppen commented Oct 2, 2023 •

edited

Loading