[SourceKit] Merge local rename and related identifiers #68554
Conversation
ahoppen
requested review from
bnbarham,
rintaro and
hamishknight
as code owners
|
@swift-ci Please smoke test |
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.
ahoppen
force-pushed
the
ahoppen/merge-local-rename-and-related-idents
branch
from
13cb467 to
53a6bca
Compare
|
@swift-ci Please smoke test |
|
@swift-ci Please SourceKit stress test |
| /// The new name that should be given to this symbol. | ||
| /// | ||
| /// This may not be known if the rename locations are specified by the client | ||
| /// using the a rename locations dicationary in syntactic rename. |
There was a problem hiding this comment.
| /// using the a rename locations dicationary in syntactic rename. | |
| /// using the a rename locations dictionary in syntactic rename. |
Is NewName ever not empty then?
|
|
||
| //--- dummy.swift | ||
|
|
||
| // RUN: %sourcekitd-test -req=related-idents -pos=3:10 %t/a.swift -- %t/a.swift %t/b.swift -o implicit_vis.o | %FileCheck -check-prefix=CHECK1 %s |
There was a problem hiding this comment.
Personally I'd put turn on --leading-lines and then put the RUN + CHECKs above using the various line variables. Then also use CHECK-NEXT and remove the -check-prefix=CHECK1 since there's only a single check here.
(I realize you are just changing the test to use split-file here, but may as well clean it up more too!)
| func foo(x: Int) { | ||
| #if true | ||
| print(x) | ||
| #else { |
| // Ignore any errors produced by `resolveRenameLocations` since, if some | ||
| // symbol failed to resolve, we still want to return all the other | ||
| // symbols. This makes related idents more fault-tolerant. | ||
| DiagnosticEngine Diags(SrcMgr); |
There was a problem hiding this comment.
Could just make the diagnostic engine optional (eg. nullptr) in this case
| std::vector<ResolvedLoc> ResolvedLocs = | ||
| resolveRenameLocations(Locs.getLocations(), *SrcFile, Diags); |
There was a problem hiding this comment.
It's unfortunate we go from source locations -> line/col (in the index to produce a symbol) -> back to range here. But re-using the index/rename seems better than implementing related idents separately so 🤷.
| std::unique_ptr<StringScratchSpace> stringStorage; | ||
| std::vector<RenameLoc> locations; |
There was a problem hiding this comment.
Is the collector actually used again with newName, or do we always end up doing the syntactic rename after we have locations?
|
While looking at open issues regarding related idents and local rename, we found that there are still a few issues here, one of them is that The proper solution here is to write The next steps to do then are:
|
|
Superseded by #70008 |
The related identifiers request was sufficiently similar to local rename that we can implement it in terms of local rename.