Explicitly model all supported contextual keywords in the `contextualKeyword` `RawTokenKind` #1173

ahoppen · 2022-12-28T18:37:47Z

The main change here is that the contextualKeyword TokenKind and RawTokenKind no longer have a string as an associated value but an enum that enumerates all known contextual keywords.

In follow-up commits, this allows us to rename contextualKeyword to keyword and merge all the other known keywords into the Keyword enum that currently enumerates all contextual keywords. That way we can remove the distinction between contextual keywords and keywords we currently have.

I’m also thinking that we can use this additional structure to guarantee a little more type safety in the parser, for example by generating RawTokenKindSubsets for children that have text_choices set, but I still need to look into that.

bnbarham

Cool stuff, the simplification of consume* is really nice! I'm not a huge fan of the huge switch for SyntaxText and then the conversion to String in TokenKind, but given we need it for defaultText in places ... 🤔.

Sources/SwiftSyntax/SyntaxNodes.swift.gyb.template

bnbarham · 2022-12-28T19:07:18Z

Sources/SwiftParser/Modifiers.swift

-        (.__setter_access, _)?,
-        (.reasync, _)?:
-        elements.append(self.parseSimpleModifier())
+      case (.final, let handle)?,


There's a few places where this is the case, but IMO it'd be better to assign kind/handle first and then switch over kind rather than having to do (.kind, let handle) or (.kind, _) everywhere. Not really relevant to this, I just noticed it while going through the changes.

The nice thing about this approach is that it guarantees we have a handle if we match a kind without having to force-unwrap it (remember there’s a default case at the bottom that doesn’t have a handle).

There's no reason that has to be in the switch though:

guard let (kind, handle) = self.at(anyIn: DeclarationModifier.self) else { break MODIFIER_LOOP } switch kind { ... }

Or am I missing something obvious?

No, that would also work. I think that’s just a matter of style now. If there were fewer cases in the RawTokenKindSubset, I would prefer a single switch instead of the guard + switch combination. And then I think it’s nice to always use the same style when you’re switching over RawTokenKindSubsets.

Sure, it's definitely style. I'll win you over eventually!

Sources/SwiftParser/Types.swift

bnbarham · 2022-12-28T19:15:42Z

Sources/SwiftParser/Types.swift

+        } else if self.at(.contextualKeyword(.Type)) || self.at(.contextualKeyword(.Protocol)) {
+          let typeOrProtocol = self.consume(if: .contextualKeyword(.Type)) ?? self.consume(if: .contextualKeyword(.Protocol))!


IMO we should add a 2 and 3 arg consume to avoid the array creation and then use that here. The only non-2/3 kind calls should probably be subsets anyway.

I agree. I’ll re-consider it after I have had some time to play around automatic generation of RawTokenKindSubset from text_choices tomorrow.

Sources/SwiftSyntax/Raw/RawSyntax.swift

Sources/SwiftSyntax/gyb_generated/TokenKind.swift

CodeGeneration/Sources/generate-swiftparser/templates/DeclarationModifierFile.swift

kimdv

Added some inline questions and notes to my self

CodeGeneration/Sources/generate-swiftparser/templates/DeclarationModifierFile.swift

Sources/SwiftSyntax/SyntaxFactory.swift.gyb

gyb_syntax_support/Token.py

gyb_syntax_support/Child.py

Sources/SwiftSyntax/Tokens.swift.gyb

kimdv · 2022-12-29T11:14:13Z

Sources/SwiftSyntax/TokenKind.swift.gyb

@@ -26,7 +26,9 @@ public enum TokenKind: Hashable {
 %
 %   # Tokens that don't have a set text have an associated value that
 %   # contains their text.
-%   if not token.text:


Note to self: Remember in #1158

Sources/SwiftSyntax/SyntaxTraits.swift.gyb

ahoppen · 2022-12-29T14:29:07Z

@swift-ci Please test

ahoppen · 2022-12-30T09:52:18Z

@swift-ci Please test

ahoppen · 2022-12-30T12:18:23Z

@swift-ci Please test

ahoppen · 2023-01-02T16:30:40Z

@swift-ci Please test

ahoppen · 2023-01-03T10:43:14Z

@swift-ci Please test

ahoppen · 2023-01-03T13:46:36Z

swiftlang/swift-stress-tester#218
swiftlang/swift-format#464

@swift-ci Please test

ahoppen · 2023-01-03T16:39:26Z

swiftlang/swift-stress-tester#218
swiftlang/swift-format#464

@swift-ci Please test

…able

…Keyword` `RawTokenKind` The main change here is that the `contextualKeyword` `TokenKind` and `RawTokenKind` no longer have a string as an associated value but an enum that enumerates all known contextual keywords. In follow-up commits, this allows us to rename `contextualKeyword` to `keyword` and merge all the other known keywords into the `Keyword` enum that currently enumerates all contextual keywords. That way we can remove the distinction between contextual keywords and keywords we currently have. I’m also thinking that we can use this additional structure to guarantee a little more type safety in the parser, for example by generating `RawTokenKindSubset`s for children that have `text_choices` set, but I still need to look into that.

ahoppen · 2023-01-11T06:55:20Z

swiftlang/swift-stress-tester#218
swiftlang/swift-format#464

@swift-ci Please test

ahoppen requested review from rintaro and bnbarham December 28, 2022 18:37

bnbarham reviewed Dec 28, 2022

View reviewed changes

kimdv reviewed Dec 28, 2022

View reviewed changes

CodeGeneration/Sources/generate-swiftparser/templates/DeclarationModifierFile.swift Outdated Show resolved Hide resolved

kimdv reviewed Dec 29, 2022

View reviewed changes

ahoppen force-pushed the ahoppen/enumerate-contextual-keywords branch 2 times, most recently from b7b9576 to f275db8 Compare December 29, 2022 14:24

ahoppen mentioned this pull request Dec 29, 2022

Handle contextual and non-contextual keywords in the same way #1174

Merged

ahoppen force-pushed the ahoppen/enumerate-contextual-keywords branch from f275db8 to 4c78347 Compare December 30, 2022 12:12

ahoppen mentioned this pull request Dec 30, 2022

[DO NOT MERGE] Dummy PR for CI testing swiftlang/swift#37710

Draft

ahoppen mentioned this pull request Dec 30, 2022

Replace where parameter in consume(if:) and friends by allowTokenAtStartOfLine #1175

Merged

ahoppen force-pushed the ahoppen/enumerate-contextual-keywords branch from 4c78347 to 7e46439 Compare January 2, 2023 16:30

ahoppen force-pushed the ahoppen/enumerate-contextual-keywords branch from d01b3e0 to 8ef8747 Compare January 3, 2023 10:43

This was referenced Jan 3, 2023

Adjustments for explicit enumeration of contextual keywords in SwiftSyntax swiftlang/swift-stress-tester#218

Merged

Adjustments for explicit enumeration of contextual keywords in SwiftSyntax swiftlang/swift-format#464

Merged

ahoppen force-pushed the ahoppen/enumerate-contextual-keywords branch 2 times, most recently from db1d8a4 to 32e1b39 Compare January 3, 2023 16:39

bnbarham approved these changes Jan 9, 2023

View reviewed changes

ahoppen added 3 commits January 11, 2023 07:32

Rename tokenKind in Lexeme to rawTokenKind

0e5987f

Allow skipping all long-running tests using a single environment vari…

b8a1761

…able

ahoppen force-pushed the ahoppen/enumerate-contextual-keywords branch from 32e1b39 to 2a1597f Compare January 11, 2023 06:55

ahoppen merged commit 8e6ae6e into swiftlang:main Jan 11, 2023

ahoppen deleted the ahoppen/enumerate-contextual-keywords branch January 11, 2023 09:23

ahoppen mentioned this pull request Jan 23, 2023

Fix performance regressions introduced by my recent changes #1257

Merged

		} else if self.at(.contextualKeyword(.Type)) \|\| self.at(.contextualKeyword(.Protocol)) {
		let typeOrProtocol = self.consume(if: .contextualKeyword(.Type)) ?? self.consume(if: .contextualKeyword(.Protocol))!

Explicitly model all supported contextual keywords in the contextualKeyword RawTokenKind #1173

Explicitly model all supported contextual keywords in the contextualKeyword RawTokenKind #1173

Uh oh!

Conversation

ahoppen commented Dec 28, 2022

Uh oh!

bnbarham left a comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kimdv left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ahoppen commented Dec 29, 2022

Uh oh!

ahoppen commented Dec 30, 2022

Uh oh!

ahoppen commented Dec 30, 2022

Uh oh!

ahoppen commented Jan 2, 2023

Uh oh!

ahoppen commented Jan 3, 2023

Uh oh!

ahoppen commented Jan 3, 2023

Uh oh!

ahoppen commented Jan 3, 2023

Uh oh!

ahoppen commented Jan 11, 2023

Uh oh!

Uh oh!

Explicitly model all supported contextual keywords in the `contextualKeyword` `RawTokenKind` #1173

Explicitly model all supported contextual keywords in the `contextualKeyword` `RawTokenKind` #1173

kimdv left a comment •

edited

Loading