Make CodeGeneration a lot less string-based #1672
Merged
+2,257
−1,783
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kimdv
approved these changes
May 16, 2023
| @@ -123,19 +118,26 @@ public class Child { | |||
|
|
|||
| /// Whether this child has syntax kind `UnexpectedNodes`. | |||
| public var isUnexpectedNodes: Bool { | |||
| syntaxKind == "UnexpectedNodes" | |||
| switch kind { | |||
| case .collection(kind: .unexpectedNodes, collectionElementName: _): | |||
There was a problem hiding this comment.
Would it make sense to just omit collectionElementName
| nameForDiagnostics: nil, | ||
| description: "A CodeBlockItem is any Syntax node that appears on its own line inside a CodeBlock.", | ||
| kind: "Syntax", | ||
| documentation: "A CodeBlockItem is any Syntax node that appears on its own line inside a CodeBlock.", |
There was a problem hiding this comment.
Not part of this, but we could add ticks around CodeBlockItem and CodeBlock (and properly also many other places) to add doc linking
bnbarham
approved these changes
May 16, 2023
| /// `AccessorList` represents a collection of `AccessorDeclSyntax` | ||
| /// `AccessorListSyntax` represents a collection of `AccessorDeclSyntax` |
There was a problem hiding this comment.
These docs don't seem particularly useful to me. I'd expect them on the type itself rather than the extension (and even there... fairly dubious about their value).
There was a problem hiding this comment.
I won’t argue with you about that. I hope that we’ll write proper documentation soon and then these will just automatically go away.
Because of its Python legacy the CodeGeneration tool has been heavily string-based. This PR fixes that at least to some degree.
The main motivating factors here were:
- Instead of using strings to specify the node type of a child node, define a `SyntaxNodeKind` enum that contains a case for each syntax node. This way you can be sure that you’re not referring to a non-existent syntax node (like we did for e.g. `InOutToken`, which we forgot to change to `.token("inout")`)
- Refactor `Node` so that it contains two initializers: One for collections and one for layout nodes instead of having one initializer that has a bunch of optional and defaulted arguments.
- Change a bunch of properties from returning a `String` to returning a `TypeSyntax` or `TokenSyntax` so we don‘t need to use the `raw:` interpolation style for them.
In general, the API of `Node` and `SyntaxNodeKind` is what I’m happy with now. The other files can still do with some cleanup.
All the changes resulted in nearly no functionaly changes of the generated code.
ahoppen
force-pushed
the
ahoppen/code-gen-not-string-based
branch
from
e708871 to
cf6ff40
Compare
|
@swift-ci Please test |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Because of its Python legacy the CodeGeneration tool has been heavily string-based. This PR fixes that at least to some degree.
The main motivating factors here were:
SyntaxNodeKindenum that contains a case for each syntax node. This way you can be sure that you’re not referring to a non-existent syntax node (like we did for e.g.InOutToken, which we forgot to change to.token("inout"))Nodeso that it contains two initializers: One for collections and one for layout nodes instead of having one initializer that has a bunch of optional and defaulted arguments.Stringto returning aTypeSyntaxorTokenSyntaxso we don‘t need to use theraw:interpolation style for them.In general, the API of
NodeandSyntaxNodeKindis what I’m happy with now. The other files can still do with some cleanup.All the changes resulted in nearly no functionality changes of the generated code, so I think this PR doesn’t need a super thorough review, just skimming over it should be sufficient. It’s really repetitive after all.