Skip to content

Add *Syntax.parse methods for the nodes that we support parsing #1076

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Nov 29, 2022
Merged

Add *Syntax.parse methods for the nodes that we support parsing #1076

merged 1 commit into from
Nov 29, 2022

Conversation

bnbarham
Copy link
Contributor

Adds a new SyntaxParseable protocol with a single parse(from:) method. Any nodes with parser_function defined implement this protocol.

Other updates:

  • SyntaxExpressibleByStringInterpolation nodes to use these new methods
  • Parser tests to check against *Syntax rather than Raw*Syntax nodes
  • Builder methods to to take nodes (which are expressible by string literals) rather than String

@bnbarham bnbarham requested a review from ahoppen as a code owner November 14, 2022 20:29
@bnbarham
Copy link
Contributor Author

@swift-ci please test

bnbarham
bnbarham commented Nov 14, 2022
View reviewed changes
@bnbarham bnbarham force-pushed the move-parse-functions branch 2 times, most recently from 21c450b to 7186e5a Compare November 15, 2022 01:33
@bnbarham
Copy link
Contributor Author

@swift-ci please test

@bnbarham bnbarham mentioned this pull request Nov 15, 2022
Merged
rintaro
rintaro reviewed Nov 15, 2022
View reviewed changes
@bnbarham bnbarham force-pushed the move-parse-functions branch 2 times, most recently from 2dd7a11 to d06c953 Compare November 15, 2022 06:13
@bnbarham
Copy link
Contributor Author

@swift-ci please test

ahoppen
ahoppen reviewed Nov 16, 2022
View reviewed changes
Sources/SwiftSyntaxBuilder/SyntaxNodeWithBody.swift
// TODO: Consider removing `HasTrailingCodeBlock` and friends
// It'd be much nicer if we didn't have any builder APIs taking strings. We
// should just make convenvience inits where needed such that we could write:
// `IfStmt("if foo")` -> `IfStmt(conditions: [.expression("foo")])`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what we had before I introduced HasTrailingCodeBlock and it was super verbose and I didn't like it. If you want, you can take a look at old versions of how classes or Strucks were being constructed in CodeGeneration.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take a look, but if it's too verbose we should fix that. IfStmt("if foo") seems really weird to me, we've said it's an if so why are we writing if again 😅? In general I'd just really like to steer away from methods that take a String and parse it, without it being obvious that's what they're doing.

Copy link
Contributor Author

@bnbarham bnbarham Nov 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you're referring to 2c04f34? To take some examples from there -

Old:

SwitchCase(label: SwitchCaseLabel(caseItems: [CaseItem(pattern: ExpressionPattern(expression: MemberAccessExpr(name: "contextualKeyword")))]))

New:

SwitchCase("case .contextualKeyword:")

Suggested:

SwitchCase(pattern: ".contextualKeyword") { }

Old:

ExtensionDecl(
       leadingTrivia: node.documentation.isEmpty
         ? []
         : .docLineComment("/// \(node.documentation)") + .newline,
       extendedType: Type(node.type.shorthandName),
       inheritanceClause: TypeInheritanceClause { InheritedType(typeName: Type("ExpressibleByArrayLiteral")) }
     )

New:

ExtensionDecl("\(docComment)extension \(node.type.shorthandName): ExpressibleByArrayLiteral")

Suggested:

ExtensionDecl(
  leadingTrivia: docComment,
  extendedType: "\(node.type.shorthandName)",
  inheritances: ["ExpressibleByArrayLiteral"])

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the function declarations were one of the worst in the old API but maybe you're right that we can switch if every child node of these is passable from a string literal. Your examples definitely look good to me.

Tests/SwiftParserTest/Assertions.swift
_ markedSource: String,
_ parseSyntax: (inout Parser) -> Node,
_ parse: (String) -> S,
substructure expectedSubstructure: Syntax? = nil,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing this, I'm wondering whether we can get rid of the custom entry point here altogether. It didn't really turn out to be super useful IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which one? I was going to change these methods to a struct + check or similar after this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that parse here could just always be SourceFileSyntax.parse. I'm still not 100% sure about it though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see. We currently expose a few parse methods (eg. decl/statement/expr/pattern/memberdecl), so IMO it probably makes sense to keep these still.

@bnbarham
Copy link
Contributor Author

@swift-ci please test

@bnbarham bnbarham force-pushed the move-parse-functions branch 2 times, most recently from eb8bf94 to 10f5bdd Compare November 18, 2022 18:08
@bnbarham
Copy link
Contributor Author

@swift-ci please test

@bnbarham bnbarham force-pushed the move-parse-functions branch from 10f5bdd to f1751a6 Compare November 19, 2022 03:09
@bnbarham
Copy link
Contributor Author

@swift-ci please test

@bnbarham bnbarham force-pushed the move-parse-functions branch from f1751a6 to 6f462d7 Compare November 19, 2022 04:42
@bnbarham
Copy link
Contributor Author

@swift-ci please test

@bnbarham bnbarham force-pushed the move-parse-functions branch from 6f462d7 to 0d72ebf Compare November 28, 2022 18:58
@bnbarham
Copy link
Contributor Author

@swift-ci please test

@bnbarham
Add *Syntax.parse methods for the nodes that we support parsing
d44fe39 
Adds a new `SyntaxParseable` protocol with a single `parse(from:)`
method. Any nodes with `parser_function` defined implement this
protocol.

Other updates:
  - `SyntaxExpressibleByStringInterpolation` nodes to use these new
    methods
  - Parser tests to check against `*Syntax` rather than `Raw*Syntax`
    nodes
  - Builder methods to to take nodes (which are expressible by string
    literals) rather than `String`
@bnbarham bnbarham force-pushed the move-parse-functions branch from 0d72ebf to d44fe39 Compare November 29, 2022 19:57
@bnbarham
Copy link
Contributor Author

@swift-ci please test

@bnbarham bnbarham merged commit 715f125 into swiftlang:main Nov 29, 2022
@bnbarham bnbarham deleted the move-parse-functions branch November 29, 2022 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahoppen ahoppen ahoppen left review comments

@rintaro rintaro rintaro approved these changes

@DougGregor DougGregor Awaiting requested review from DougGregor

@CodaFi CodaFi Awaiting requested review from CodaFi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bnbarham @rintaro @ahoppen