Skip to content

Renames are allowed to change the meaning of the program, due to shadowing. #10713

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

Closed
umanwizard opened this issue Nov 7, 2021 · 6 comments · Fixed by #19079
Closed

Renames are allowed to change the meaning of the program, due to shadowing. #10713

umanwizard opened this issue Nov 7, 2021 · 6 comments · Fixed by #19079
Assignees
Labels
A-ide general IDE features C-feature Category: feature request S-actionable Someone could pick this issue up and work on it right now

Comments

@umanwizard
Copy link
Contributor

For example:

fn f() {
    let x = 5;
    let y = x + 3;
    println!("{}", x + y);
}

if x is renamed to y, this becomes

fn f() {
    let y = 5;
    let y = y + 3;
    println!("{}", y + y);
}

which prints a different number. Rust-analyzer should probably refuse to rename x in this case.

@samlh
Copy link

samlh commented Nov 7, 2021

Hm, if this was blocked I'd want a way to force it to happen anyway as sometimes I really do want semantic changes. Visual Studio's C# rename preview highlights name conflicts in red, but lets you do it anyway, which seems the best of both worlds.

@umanwizard
Copy link
Contributor Author

Can you give an example of a situation where this behavior would be desirable?

@samlh
Copy link

samlh commented Nov 8, 2021

Sure! It happens pretty often to me that one is a typo of the other, and renaming the two variables to be the same is the whole point.

@umanwizard
Copy link
Contributor Author

That` is a good point; perhaps the best behavior would be to warn, or to offer different commands for "safe rename" vs. "force rename" (I'm not sure whether this is possible in the LSP protocol, though)

@Veykril
Copy link
Member

Veykril commented Nov 8, 2021

The LSP spec(3.16.0+) supports user confirmation for specified renames with a description as to why confirmation is requested, that could be used in this case, that is if the rename creates name collisions we could add a confirmation via a ChangeAnnotation if supported.

@Veykril Veykril added A-ide general IDE features C-feature Category: feature request S-actionable Someone could pick this issue up and work on it right now labels Nov 8, 2021
@vigna
Copy link

vigna commented Feb 1, 2025

Has this been solved? I just stumbled into the same problem in a less evident context: imagine something like

fn main() {
    let mut v = vec![0; 5];
    let node = 0;
    let mut iter = (0..5).into_iter();
    while let Some(curr) = iter.next() {
        v[curr] = node;
    }
    println!("{:?}", v);
}

but more complicated. If you rename the loop variable code to node the program semantics changes completely. I happened to do this together with a number of renamings and it took some time to find the broken one.

One vote for default behavior being to warn the user. Refactorings should not, by definition, change the program semantics.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ide general IDE features C-feature Category: feature request S-actionable Someone could pick this issue up and work on it right now
Projects
None yet
5 participants