Some rename bugs

[Bben01]

rust-analyzer version: 0.3.2237-standalone (59bc7b4 2024-12-29)
rustc 1.83.0 (90b35a623 2024-11-26)
VSCode version: 1.95.3 (macOS)

Hi,
I worked lately on a renaming tool using r-a, I ran into some edge cases.
I've opened a repo that list what I could minimally reproduce, with one bug per module.

You can find it here: https://github.com/Bben01/rust-analyser-rename-bugs

Here is a summary of the bugs:

• Renaming an item that have a cfg-ed out counterpart won't get renamed

// here we have two items, one will be disabled and one enabled,
// if I rename the one that is enabled, the other one won't get renamed

#[cfg(miri)]
const CONST: usize = 6;

#[cfg(not(miri))]
const CONST: usize = 8;

• Renaming insinde the offset_of! macro is not working #18891

pub struct Offset {
    // renaming inner here won't rename inner in the offset_of! macro
    inner: Inner,
}

pub struct Inner {
    // renaming `a` here won't rename `a` in the offset_of! macro
    a: u8,
}

#[test]
fn rename_offset_of() {
    // Trying to rename inner or `a` from here results in a "No reference found at position"
    std::mem::offset_of!(Offset, inner.a);
}

• Renaming a function used in serde(with = "") won't get renamed

use std::fmt::Display;
use serde::{Serialize, Serializer};

#[derive(Serialize)]
pub struct SomeStruct {
    #[serde(serialize_with = "serialize_as_display")]
    field: usize,
}

// renaming this function won't rename the usage in `serialize_with`
pub fn serialize_as_display<T, S>(value: &T, serializer: S) -> Result<S::Ok, S::Error>
where
    T: Display,
    S: Serializer,
{
    serializer.serialize_str(&value.to_string())
}

• Renaming a struct that has a function with the same name will break code: #18892

// This currently export both the function and the struct
pub use aligned::Aligned;

mod aligned {
    // Renaming this struct will rename the `pub use` above, breaking the use in the test
    pub struct Aligned {
        size: usize,
    }

    #[allow(non_snake_case)]
    pub fn Aligned(size: usize) -> Aligned {
        Aligned {
            size
        }
    }
}

#[test]
fn aligned_rename_failure() {
    Aligned(5);
}

• Renaming a field used in thiserror won't rename the usage in the attribute of the macro (I think it may be related to the serde(with)) #18893

use std::io;
use thiserror::Error;

#[derive(Error, Debug)]
#[error("Rename failed at {}", .source)]
pub struct Error {
    // renaming this field won't rename the usage above
    source: io::Error,
}

I am also working on reproducing a crash I got with the semantic db, I will update this issue when I get an example.

[ChayimFriedman2]

Renaming an item that have a cfg-ed out counterpart won't get renamed

There isn't really something we can do about this. Right now, we don't analyze cfg'ed out code at all. I want to change this, but in this case we have literally no way to know the two consts are related.

Renaming a function used in serde(with = "") won't get renamed

Probably because it's inside a string. I believe it's doable though.

Renaming a struct that has a function with the same name will break code:

The code is invalid, but it was your request. We can at most warn that the rename will produce invalid code, and even that I don't believe we can do with the current infrastructure. For cases like that there is Undo. See below.

[Bben01]

The code is invalid, but it was your request. We can at most warn that the rename will produce invalid code, and even that I don't believe we can do with the current infrastructure. For cases like that there is Undo.

In theory, couldn't we add another export with the new name (so that the previous behavior - exporting the struct and the function - is what we get at the end)?

[ChayimFriedman2]

Sorry, I misunderstood your meaning in that bullet. That is indeed possible I believe.

[Veykril]

Renaming a function used in serde(with = "") won't get renamed

(as was said) This is due to the string literal. This won't ever be supported, serde should instead allow using proper paths (without the string literals) there. (the reason for why it uses string literals is that literals were the only allowed t okens in that position back in the day)

Renaming a field used in thiserror won't rename the usage in the attribute of the macro (I think it may be related to the serde(with))

This on the other hand I would expect to work, though it depends on what thiserror is doing. It needs to attach the span to the field in its expansion which I believe it might not be doing in which case we can't do anything about this either.

[Bben01]

I've uploaded a minimal reproduction of the panic in Semantic db (you can find it here: https://github.com/Bben01/rust-analyser-rename-bugs/blob/bdc4c6127262974e554bbb147258d053d7949f8d/src/proc_macro_static.rs)

The backtrace

thread 'Worker' panicked at crates/hir/src/semantics.rs:1728:13:


Failed to lookup TRAIT@0..14 in this Semantics.
Make sure to use only query nodes, derived from this instance of Semantics.
root node:   MACRO_ITEMS@0..101
known nodes: SOURCE_FILE@0..116, MACRO_ITEMS@0..6480, MACRO_ITEMS@0..279, MACRO_ITEMS@0..814, MACRO_ITEMS@0..72


stack backtrace:
   0: _rust_begin_unwind
   1: core::panicking::panic_fmt
   2: hir::semantics::SemanticsImpl::find_file
   3: ide_db::defs::NameClass::classify
   4: ide_db::search::FindUsages::search
   5: hashbrown::raw::RawIterRange<T>::fold_impl
   6: <core::iter::adapters::map::Map<I,F> as core::iter::traits::iterator::Iterator>::fold
   7: ide::highlight_related::highlight_related
   8: ra_salsa::Cancelled::catch
   9: rust_analyzer::handlers::request::handle_document_highlight
  10: core::ops::function::FnOnce::call_once{{vtable.shim}}
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
[Error - 19:00:16] Request textDocument/documentHighlight failed.
  Message: request handler panicked: 

Failed to lookup TRAIT@0..14 in this Semantics.
Make sure to use only query nodes, derived from this instance of Semantics.
root node:   MACRO_ITEMS@0..101
known nodes: SOURCE_FILE@0..116, MACRO_ITEMS@0..6480, MACRO_ITEMS@0..279, MACRO_ITEMS@0..814, MACRO_ITEMS@0..72

[Veykril]

Having looked at the thiserror thing a bit, the spans seem correct, the ident is used within a destruring pattern once, and within a condition. We seem to only pick the one in the condition though, so we lose the actual field association.

[Veykril]

I broke out the workable issues into their own to better track them #18893 #18892 #18891