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

Fix application of fix-its replacing parent instead of first child #16

Merged
merged 3 commits into from
Jan 3, 2024

Conversation

gohanlon
Copy link
Contributor

This fixes #15 by pulling in the latest FixItApplier from swift-syntax main (d647052), which is now String-based.

  • For review, I left an intermediate commit showing how the new test fails before the fix. I’m happy to squash that commit into the fix before merging.
  • I added “Bump deps” opportunistically, but this PR doesn’t depend on it. Let me know if you’d rather do that outside of this PR.

Note:

  • The upstream FixItApplier has undergone a complete rework in swift-syntax main. It’s now String-based rather than SyntaxRewriter-based. (It’s publicly accessible but not exposed as a product within the underscored _SwiftSyntaxTestSupport module, an “internal helper target”.)
  • The upstream FixItApplier.applyFixes function isn’t needed by swift-macro-testing, and omitting it allows these two extensions on FixIt to be omitted.
  • SwiftSyntax/SourceEdit is public but hasn't been released, so eventually it can be removed from swift-macro-testing.

This pulls in the latest `FixItApplier` from swift-syntax `main`
(d647052), which is now String-based.

Fixes pointfreeco#15.
@gohanlon gohanlon force-pushed the fix-it-applier-source-edit branch from 4d550d8 to fdf08ac Compare December 30, 2023 19:52
@gohanlon
Copy link
Contributor Author

Also for consideration, see a disfavored alternative approach: fix-it-applier-compare-kind. While interesting, I think it ultimately reinforces this PR's solution.

In this alternative, I'd independently arrived at the same simple solution that #12 initially had but removed before merging. However, because oldNode.id == node.id is never true (I think because the old and/or new nodes have been re-rooted), I suspect that visit(_: TokenSyntax) -> TokenSyntax needs to also be modified to also work on offsets and node kind. Further, neither swift-macro-testing nor my own projects use or have test coverage for .replaceLeadingTrivia/.replaceTrailingTrivia, so my inclination is to keep better alignment with the upstream. Plus, I expect/hope FixItApplier will eventually become publicly available.

@@ -343,6 +349,57 @@ public func assertMacro(
}
}

// From: https://github.com/apple/swift-syntax/blob/d647052/Sources/SwiftSyntaxMacrosTestSupport/Assertions.swift
Copy link
Member

Choose a reason for hiding this comment

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

Instead of inlining these in the library, what do you think of putting it in a file in the SwiftSyntax directory alongside SourceEdit, etc.?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'm happy to do that. I didn't do so initially because the upstream extensions are fileprivate and kept close to the asserts that use them. Plus, these two extensions are just a small parts of their larger upstream file, and I think files from upstream have so far been pulled in wholesale with minimal alterations.

Here are the upstream extensions' signatures from SwiftSyntax's SwiftSyntaxMacrosTestSupport/Assertions.swift:

extension FixIt.Change {
  fileprivate func edit(
    in expansionContext: BasicMacroExpansionContext
  ) -> SourceEdit {
    
  }
}
extension BasicMacroExpansionContext {
  fileprivate func position(
    of position: AbsolutePosition,
    anchoredAt node: some SyntaxProtocol
  ) -> AbsolutePosition {
    
  }
}

I think it'd be okay to make the extensions internal instead of fileprivate. Here are some options:

  1. Since BasicMacroExpansionContext is common between the two extensions, make the extensions internal and add them to a new file: Sources/SwiftSyntax/SourceEditMacroExpansion.swift.
  2. Make the extensions internal and them directly to Sources/SwiftSyntax/SourceEdit.swift.
  3. If you'd rather leave them in AssertMacro.swift as fileprivate, I can move them to the end of the file.

For me to move forward, feel free to pick one of these ideas or throw in another that you think might work better. Looking forward to your input!

Copy link
Member

@stephencelis stephencelis left a comment

Choose a reason for hiding this comment

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

Looks great! Just one small thing. Wanna tackle it before we merge?

Copy link
Member

@stephencelis stephencelis left a comment

Choose a reason for hiding this comment

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

I think you've convinced me that moving the methods is probably not worth it 😄

@stephencelis stephencelis merged commit 8b031e3 into pointfreeco:main Jan 3, 2024
2 checks passed
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.

Fix-it targeting first child node instead replaces parent syntax collection
2 participants