feat: Warn the user when a rename will change the meaning of the program #19079
Conversation
rustbot
added
the
S-waiting-on-review
| }, | ||
| Indel { | ||
| insert: "\n\nfn fun_name() -> i32 {\n 5\n}", | ||
| delete: 110..110, | ||
| annotation: None, |
There was a problem hiding this comment.
nit but might be nice to omit rendering annotation in the debug impl if its None (given we use it in a bunch of expect tests that reduces some noise)
There was a problem hiding this comment.
I prefer to update the tests, like what if we will need it in some of them eventually.
| // Create a dummy edit to warn the user. | ||
| file_id = Some(file_id2); | ||
| let text = sema.db.file_text(file_id2.file_id())[range].to_owned(); | ||
| edit.indel(Indel::replace(range, text).with_annotation(annotation)); |
There was a problem hiding this comment.
Do we actually need annotation support on an Indel basis? That seems way too fine grained, imo it should totally suffice to attach an annotation on a per TextEdit basis instead. That would simplify things as well (just because the LSP allows us to have this level of control doesn't mean we need to do so).
That way we also don't need a dummy edit here I believe and instead just attach the annotation to the edit and then replicate that back to all of its indels within the proto conversion
There was a problem hiding this comment.
The dummy edit is important because it shows which variables will conflict. As such I think we do need them on Indel.
There was a problem hiding this comment.
I find that rather confusing though. We are abusing the LSP here, showing the user a no-op edit (that in VSCode they can set a checkbox for that doesn't do anything).
There was a problem hiding this comment.
I don't like that either, but I do think pointing to the affected variables is important. Perhaps we can create a protocol extension? Does VSCode have some way to point at them during rename?
There was a problem hiding this comment.
I don't think there is. VSCode is generally very inflexible with its builtin features.
There was a problem hiding this comment.
If so I don't know what is best. I still think we should show the conflicting variables, but I also agree the noop edit is confusing. So I'll do what you prefer. Do you prefer to show one confirmation?
Specifically, when a rename of a local will change some code that refers it to refer another local, or some code that refer another local to refer to it. We do it by introducing a dummy edit with an annotation. I'm not a fond of this approach, but I don't think LSP has a better way.
ChayimFriedman2
force-pushed
the
rename-conflict
branch
from
f87a7b8 to
0c7c63e
Compare
Specifically, when a rename of a local will change some code that refers it to refer another local, or some code that refer another local to refer to it.
We do it by introducing a dummy edit with an annotation. I'm not a fond of this approach, but I don't think LSP has a better way.
Demo:
Screen.Recording.2025-02-02.015127.mp4
Closes #10713.