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

Update Swift #523

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

NinjaLikesCheez
Copy link

@NinjaLikesCheez NinjaLikesCheez commented Jan 16, 2025

Checklist

  • Any new parsing code was already published, integrated, and merged into Semgrep. DO NOT MERGE THIS PR BEFORE THE SEMGREP INTEGRATION WORK WAS COMPLETED.
  • Change has no security implications (otherwise, ping the security team)

I've opened this MR to update an aging Swift grammer. Semgrep is unable to scan most Swift files I've tried it against (because of things like freestanding #Preview macros).

I'm not fully up to speed on item one on this list, so I've marked this as a WIP until I can figure that piece out (but I'm away from my computer from Saturday for a week - so it may take a while).

Thanks!

@NinjaLikesCheez NinjaLikesCheez requested a review from a team as a code owner January 16, 2025 15:35
@CLAassistant
Copy link

CLAassistant commented Jan 16, 2025

CLA assistant check
All committers have signed the CLA.

@aryx aryx requested a review from nmote January 16, 2025 15:50
@nmote nmote marked this pull request as draft January 16, 2025 17:02
@nmote
Copy link
Collaborator

nmote commented Jan 16, 2025

Thanks! Marking as draft while you are working on this. Please mark as ready for review once that is the case!

@NinjaLikesCheez
Copy link
Author

@nmote No problem! I've followed the guide (although it's a little confusing in place - especially when I've never touched a lot of these technologies!). I'm unsure how to properly test that this will integrate fine with semgrep. Do you have any advice or documentation I could take a look at?

I've been following this: https://semgrep.dev/docs/contributing/updating-a-grammar but of course I can't do the release part - I wanted to verify nothing has broken by updating this grammar first however.

Thanks!

@nmote
Copy link
Collaborator

nmote commented Jan 29, 2025

Apologies, this notification must have fallen through the cracks!

It's not well-documented, but you can do a dry-run release and then push the changes from the local copy of semgrep-swift that this creates to your Semgrep submodule and then start making the necessary changes in Semgrep.

Although, this looks to be just a straightforward update to the tree sitter grammar. I'd be happy to release semgrep-swift off of your branch when you're ready, and then you can work on integrating the changes with Semgrep, which frankly is the tedious part of a language upgrade. Once that's all done we can merge this.

Please mark this ready for review to put this back in my review queue, or ping me on the community slack if you need more questions answered. I'm buried in GitHub notifications so I'm likely to miss just a comment.

@NinjaLikesCheez
Copy link
Author

Apologies, this notification must have fallen through the cracks!

No problem!

It's not well-documented, but you can do a dry-run release and then push the changes from the local copy of semgrep-swift that this creates to your Semgrep submodule and then start making the necessary changes in Semgrep.

Although, this looks to be just a straightforward update to the tree sitter grammar. I'd be happy to release semgrep-swift off of your branch when you're ready, and then you can work on integrating the changes with Semgrep, which frankly is the tedious part of a language upgrade. Once that's all done we can merge this.

I eventually figured this might be the way to go so I've already started - there's a fair bit to do as Swift has changed a lot and I'm learning OCaml for this so it will need a thorough review when it's ready.

Please mark this ready for review to put this back in my review queue, or ping me on the community slack if you need more questions answered. I'm buried in GitHub notifications so I'm likely to miss just a comment.

Done! Thanks!

@NinjaLikesCheez NinjaLikesCheez marked this pull request as ready for review January 30, 2025 09:56
@NinjaLikesCheez NinjaLikesCheez changed the title WIP: Update Swift Update Swift Jan 30, 2025
@nmote
Copy link
Collaborator

nmote commented Jan 30, 2025

Sounds like you're not currently blocked on this, then. I'll let you continue as-is unless you need a release of semgrep-swift to make progress. Given that there have been a lot of changes, you might actually find it easier to update incrementally with a few intermediate versions of semgrep-swift based on a few intermediate versions of tree-sitter-swift, and given that you've already figured out how to use a local version of the submodule you can freely experiment with that.

When the time comes I'll be happy to help by releasing intermediate versions of semgrep-swift, or just one for this PR.

Copy link
Collaborator

@nmote nmote left a comment

Choose a reason for hiding this comment

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

I've updated semgrep-swift with these changes: semgrep/semgrep-swift@7214b84

You can now incorporate that into your Semgrep PR to make the check-submodule job happy.

As always, please re-request review on this PR if you need something from me!

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.

3 participants