Skip to content

FixItApplier: Misc improvements #3121

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 7 commits into from
Jul 4, 2025
Merged

FixItApplier: Misc improvements #3121

merged 7 commits into from
Jul 4, 2025

Conversation

AnthonyLatsis
Copy link
Contributor

@AnthonyLatsis AnthonyLatsis commented Jul 2, 2025

  • Tests for the overload that takes source edits.
  • Optimizations.
  • Bug fixes.
  • Control over whether to skip duplicate insertions. In swift package migrate, this will enable us to prevent repeated fix-it application in various scenarios where multiple elements in a protocol composition type are diagnosed with the same insertion fix-it.

@AnthonyLatsis
Copy link
Contributor Author

@swift-ci please test


// Drop any subsequent edits that conflict with one we just applied, and
// adjust the range of the rest.
while true {
Copy link
Member

Choose a reason for hiding this comment

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

Am I missing something or could this be a for loop like

for adjustmentEditIndex in editIndex..<edits.endIndex

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for adjustmentEditIndex in (editIndex + 1)..<edits.endIndex

, yeah. Sounds good.

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 just remembered. I did this because it felt less safe to have 2 index variables in scope in the inner loop.

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 crammed the increment into the while condition but I don’t think you’re going to like this 😅

@AnthonyLatsis AnthonyLatsis force-pushed the jepa2 branch 2 times, most recently from 6d1fd86 to bc79daf Compare July 4, 2025 09:55
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

LGTM, could you add a short RFC post for the new API as described in https://github.com/swiftlang/swift-syntax/blob/main/Contributor%20Documentation/RFC%20Process.md?

@AnthonyLatsis
Copy link
Contributor Author

Thank you, loved the feedback!

Declarations marked with @_spi are not considered part of the public API and don’t require an RFC

For the new parameter you mean? Even though FixItApplier is SPI?

@ahoppen
Copy link
Member

ahoppen commented Jul 4, 2025

Sorry, didn’t realize that FixItApplier is SPI, just saw the public methods inside. In that case we don’t need an RFC. Good to go 🚀

@AnthonyLatsis
Copy link
Contributor Author

Ah, to be fair, I did add a Strideable conformance to AbsolutePosition. Let me know if that warrants an RFC 🙂

@AnthonyLatsis
Copy link
Contributor Author

@swift-ci please test

@AnthonyLatsis AnthonyLatsis enabled auto-merge July 4, 2025 17:40
@AnthonyLatsis
Copy link
Contributor Author

@swift-ci please test Windows

@AnthonyLatsis AnthonyLatsis merged commit 638901a into main Jul 4, 2025
27 checks passed
@AnthonyLatsis AnthonyLatsis deleted the jepa2 branch July 4, 2025 22:36
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