Share implementation of local rename and related identifiers + implement NameMatcher in Swift
#70008
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Local rename and related identifiers were sufficiently similar that we can implement related identifiers in terms of local rename.
…nges instead of putting them in an array first After implementing related identifiers in terms of rename, this step seems to no longer be necessary.
We should (and do) have one new name for the entire rename operation, not a separate new name for different positions at which the renamed symbol occurs.
The parameter was never checked anywhere.
AFAICT we can just infer whether to resolve arguments based on the presence of arguments. We don’t need the information to be passed in.
`UnresolvedLoc` was just a wrapper around `SourceLoc` and isn’t needed anymore.
We can’t represent the parent node when rewriting `NameMatcher` using swift-syntax. Replace it by two boolean fields `IsComment` and `IsInStringLiteral` that can be bridged.
Local refactoring should go through the `find-local-rename-ranges` request.
With the local refactoring being removed, all production refactoring paths return a set of ranges to rename and don’t apply edits to the file. Thus, the `syntactic-rename` action also doesn’t make sense anymore.
With `TextReplacementsRenamer` gone, `RenameRangeDetailCollector` was the only remaining subclass of `Renamer`.
…tail The usage becomes a lot clearer if there’s just a top-level function that returns the details for the syntactic rename ranges.
… consumers This makes it a lot easier to follow the code.
This type will get exposed to Swift and the members should be lowercase so that they read nicely in Swift.
…re_swift_host_library`
This reduces the configure time and reduces the pieces that may check for dependencies as we are growing the dependencies on Swift.
…cVector` This was causing build issues on Linux with Swift 5.8. Instead, wrap the `std::vector` in a `BridgedResolvedLocVector` that has a pointer to a heap-allocated `std::vector`
… on Windows Without this, header fail to compile when they are used from Swift with C++ interop.
ahoppen
requested review from
bnbarham,
rintaro,
hamishknight,
zoecarver and
CodaFi
as code owners
This was referenced Nov 23, 2023
|
swiftlang/llvm-project#7818 @swift-ci Please smoke test |
|
swiftlang/llvm-project#7818 @swift-ci Please SourceKit stress test |
|
swiftlang/llvm-project#7818 @swift-ci Please smoke test Windows |
|
swiftlang/llvm-project#7818 @swift-ci Please SourceKit stress test |
ahoppen
commented
Nov 27, 2023
Comment on lines
27
to
35
| #else // USED_IN_CPP_SOURCE | ||
|
|
||
| #ifdef SWIFT_SIL_SILVALUE_H | ||
| #error "should not include swift headers into bridging header" | ||
| #endif | ||
| #ifdef LLVM_SUPPORT_COMPILER_H | ||
| #error "should not include llvm headers into bridging header" | ||
| #endif | ||
|
|
There was a problem hiding this comment.
@eeckstein Why were these checks needed? AFAICT everything builds if I add the following two includes, which hit the check.
There was a problem hiding this comment.
Because the BridgingHeaders should not include any llvm/swift/C++ headers in pure mode. See #69039.
So please don't remove those checks!
|
Note to self: Stress tester did not find any issues. |
ahoppen
force-pushed
the
ahoppen/name-matcher-in-swift
branch
from
e84212f to
3650cf2
Compare
|
@swift-ci Please smoke test |
ahoppen
force-pushed
the
ahoppen/name-matcher-in-swift
branch
from
3650cf2 to
47b079b
Compare
|
@swift-ci Please smoke test |
ahoppen
force-pushed
the
ahoppen/name-matcher-in-swift
branch
from
47b079b to
f0ca5f3
Compare
ahoppen
force-pushed
the
ahoppen/name-matcher-in-swift
branch
from
f0ca5f3 to
b313982
Compare
|
swiftlang/llvm-project#7818 @swift-ci Please smoke test |
eeckstein
reviewed
Nov 28, 2023
include/swift/Basic/BasicBridging.h
Outdated
| @@ -25,6 +25,7 @@ | |||
|
|
|||
| #include <stddef.h> | |||
| #include <stdint.h> | |||
| #include <vector> | |||
There was a problem hiding this comment.
Please don't add any C++ headers in BasicBridging.h
There was a problem hiding this comment.
Is it even needed to bridge a std::vector of BridgedCharSourceRange?
If this vector is constructed on the swift side and then passed to C++, you can just use a swift Array and pass its buffer to C++, like done here: https://github.com/apple/swift/blob/b90b3261164f149030243b856f5dcfce12a34662/SwiftCompilerSources/Sources/SIL/Utils.swift#L224
If it's constructed on the C++ side and used on the swift side, you can bridge an ArrayRef like struct.
|
@swift-ci Please smoke test |
1 similar comment
|
@swift-ci Please smoke test |
eeckstein
reviewed
Nov 29, 2023
| /// ensure that `takeUnbridged` is called to free the memory. | ||
| class BridgedCharSourceRangeVector { | ||
| /// Opaque pointer to `std::vector<CharSourceRange>`. | ||
| void *_Nonnull vector; |
hamishknight
approved these changes
Nov 29, 2023
|
This broke the community Android CI- one of the only CI that doesn't have a host Swift compiler so it doesn't build the early swift-syntax- because I think this can be fixed by making |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The primary goal of this PR is to share the same implementation between local rename and related identifiers. Doing so uncovered issues in
NameMatcher, which assumed that it was iterating the AST nodes in source order, but wasn’t in some cases. To fix that issue, this the linked swift-syntax PR re-implementsNameMatcheron top of swift-syntax trees and this PR start using that.I tried using C++ to Swift interop to achieve this but ultimately failed because
The last couple of commits revert the C++ to Swift interop. Instead, Swift exposes a C function that can be called from C++.
It should be possible to review the PR commit by commit. Most commits should be very straightforward.