[RelatedIdents] Fix an assertion failure if two invalid declarations have the same base name #73468
Conversation
|
@swift-ci Please smoke test |
lib/Refactoring/LocalRename.cpp
Outdated
| @@ -195,6 +195,7 @@ swift::ide::getRenameInfo(ResolvedCursorInfoPtr cursorInfo) { | |||
|
|
|||
| class RenameRangeCollector : public IndexDataConsumer { | |||
| StringRef usr; | |||
| SmallString<64> displayName; | |||
There was a problem hiding this comment.
Any reason not to treat this the same as usr? ie. local smallstring and copy into string storage? I haven't looked at where usr is used and if there's a good reason we're doing that anyway, so this really is a genuine question and not just rhetorically asking you to change it 😅
There was a problem hiding this comment.
I was writing a whole paragraph about how usr is returned and thus needs to be in stringStorage anyway and how name is only used locally so no need to waste space in stringStorage for it, only that I now realized that name is returned and usr is not. 😅
Changed both to use SmallString<64> as a member of RenameRangeCollector because it simplifies the code.
lib/Refactoring/LocalRename.cpp
Outdated
| usr = stringStorage->copyString(usrStorage.str()); | ||
|
|
||
| llvm::raw_svector_ostream nameOS(displayName); | ||
| index::printDisplayName(D, nameOS); |
There was a problem hiding this comment.
Do we really have to expose printDisplayName here? It just handles getters/setters, which presumably we don't care about here anyway (ie. can this just be D->getName()?)
There was a problem hiding this comment.
I would prefer to, yes. Less because we need it right now and more to be future-proof. If we modify printDisplayName in the future without remembering to change the call here, we would be missing results from related idents.
ahoppen
force-pushed
the
related-idents-of-invalid-decls-with-same-base-name
branch
from
ade4b2a to
df035eb
Compare
|
@swift-ci Please smoke test |
…have the same base name
For example, the following declarations have the same USR with a single ERROR_TYPE parameter despite being distinct declarations.
```swift
func bar(body: Invalid) {}
func bar(ignoreCase: Bool, body: Invalid) {}
```
We originally intended to check the USR so that local rename behaves more like global rename, which also looks symbols up by USR. But the above example proves that assumption wrong.
rdar://126803702
ahoppen
force-pushed
the
related-idents-of-invalid-decls-with-same-base-name
branch
from
df035eb to
1a67c61
Compare
|
@swift-ci Please smoke test |
|
@swift-ci Please smoke test |
For example, the following declarations have the same USR with a single ERROR_TYPE parameter despite being distinct declarations.
We originally intended to check the USR so that local rename behaves more like global rename, which also looks symbols up by USR. But the above example proves that assumption wrong.
rdar://126803702