Skip to content
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

Determine if ASTGen is responsible for checking that optional tokens exist in the SwiftSyntax tree #68355

Open
ahoppen opened this issue Sep 6, 2023 · 4 comments
Assignees
Labels
ASTGen Area → compiler: The ASTGen module, which translates SwiftSyntax trees to the C++ AST compiler The Swift compiler in itself task

Comments

@ahoppen
Copy link
Contributor

ahoppen commented Sep 6, 2023

https://github.com/apple/swift/pull/67111/files#diff-de110680caa5f6af695d85ee44bc75ce4693b169e6e537ca176ec3fda17705a8R22-R29 checks that a FunctionCallExprSyntax has a left parenthesis and a right parenthesis if there are arguments. We need to figure out if it’s ASTGen’s responsibility to performs such checks.

Options are:

  1. We change the SwiftSyntax tree to make sure that the parenthesis are always present if one of the following conditions is satisfied (which might be tricky)
    a. There is a single argument in arguments or
    b. There is no trailing closure
  2. ASTGen performs these checks like it does today
  3. Since the SwiftSyntax tree is structurally valid without the left and right parenthesis, ASTGen does not need to check these invariants unless it requires the parentheses e.g. to get their locations.

I’m favoring option (3) at the moment

@ahoppen ahoppen added the ASTGen Area → compiler: The ASTGen module, which translates SwiftSyntax trees to the C++ AST label Sep 6, 2023
@ahoppen ahoppen changed the title Determine if ASTGen is responsible for checking that Determine if ASTGen is responsible for checking that optional tokens exist in the SwiftSyntax tree Sep 6, 2023
@AnthonyLatsis AnthonyLatsis added compiler The Swift compiler in itself task labels Sep 8, 2023
@saehejkang
Copy link
Contributor

If ASTGen does not need to make the check for the other invariants, is this issue just a simple deletion of that check? The left paren and right paren are passed to the visit, so only a check there would be appropriate?

@bnbarham
Copy link
Contributor

bnbarham commented Oct 2, 2023

is this issue just a simple deletion of that check

The short answer here is really that we haven't decided yet.

The currently-marked ASTGen issues are mostly from tasks that we identified in #67111 as needing to happen before any further work in this area. In some cases (like this one), we haven't necessarily decided on a direction yet. Other than the issues marked good first issue, they're generally fairly involved and were mostly opened so that we didn't lose the discussion in that PR.

@rintaro and @hamishknight will be working in this area, so I'll go through and assign them appropriately. If you're interested in following the work, we'll make sure to mark any related PRs with ASTGen :)

@saehejkang
Copy link
Contributor

are there any other good first issues or other issues in general that will be good to work on for gaining a better understaniding Looking at the good first issues that are open, a lot of them are really old or being worked on.

@bnbarham
Copy link
Contributor

Sorry @saehejkang, missed this comment. I don't think there's any in ASTGen at the moment (pending some foundational work), but it's likely we'll have more in the future and we'll file/mark them then :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ASTGen Area → compiler: The ASTGen module, which translates SwiftSyntax trees to the C++ AST compiler The Swift compiler in itself task
Projects
None yet
Development

No branches or pull requests

5 participants