Skip to content

[ASTGen] Give argument labels to all generate(_:) functions #69876

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.

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

Merged
merged 2 commits into from
Nov 15, 2023

Conversation

rintaro
Copy link
Member

@rintaro rintaro commented Nov 15, 2023

Explicit argument labels are useful for "Open Quickly..." in Xcode, reduce the fear of miss overload resolution, and better crash backtrace as they might not show parameter types.

notes:

  • generate(xxxExpr:) instead of generateXXXExpr(_:) because it's an argument information. It's' possible that we'd need to generate different things from the same argument. I'd like to reserve the base name for the return value. For example:
      func generateExpr(tupleExpr: TupleExprSyntax) -> BridgedTupleExpr
      func generatePattern(tupleExpr: TupleExprSyntax) -> BridgedTuplePattern
  • I haven't decided what todo with generate(_: Syntax) -> ASTNode maybe generateASTNode(syntax:)?
  • All generate(_: T?) are now generate(optional:) It's still overloaded, but probably enough for disambiguation

Explicit argument labels are useful for "Open Quickly..." in Xcode,
reduce the fear of miss overload resolution, and better crash
backtrace as they might not show parameter types.
@rintaro
Copy link
Member Author

rintaro commented Nov 15, 2023

@swift-ci Please smoke test

@rintaro
Copy link
Member Author

rintaro commented Nov 15, 2023

@swift-ci Please smoke test

@ahoppen
Copy link
Member

ahoppen commented Nov 15, 2023

  • I haven't decided what todo with generate(_: Syntax) -> ASTNode maybe generateASTNode(syntax:)?

Since we also have generate(choices:) -> ASTNode, why not have generate(syntax:) -> ASTNode?

  • All generate(_: T?) are now generate(optional:) It's still overloaded, but probably enough for disambiguation

I would have overloaded the generate(xxxExpr:) with the optional variant instead of overloading generate(optional:) but don’t have very strong feelings.

@rintaro rintaro merged commit 553e55f into swiftlang:main Nov 15, 2023
@rintaro
Copy link
Member Author

rintaro commented Nov 15, 2023

I haven't decided what todo with generate(_: Syntax) -> ASTNode maybe generateASTNode(syntax:)?

Since we also have generate(choices:) -> ASTNode, why not have generate(syntax:) -> ASTNode?

I decided to remove generate(_: Syntax) -> ASTNode. It's just unsafe (i.e. not all node kind can generate ASTNode), and I believe it's actually not needed.

All generate(_: T?) are now generate(optional:) It's still overloaded, but probably enough for disambiguation

I would have overloaded the generate(xxxExpr:) with the optional variant instead of overloading generate(optional:) but don’t have very strong feelings.

I'm going to generalize optional node handling in followups. Defining this for each optional syntax kind is too much boilerplate.

@ahoppen
Copy link
Member

ahoppen commented Nov 15, 2023

I'm going to generalize optional node handling in followups. Defining this for each optional syntax kind is too much boilerplate.

If you have ideas for that, I’m happy to see them.

@rintaro
Copy link
Member Author

rintaro commented Nov 15, 2023

#69894

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants