Warn the user when a rename will change the meaning of the program

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.

Author: ChayimFriedman2

crates/hir-def/src/path.rs

@@ -57,7 +57,7 @@ pub enum Path {
     /// or type anchor, it is `Path::Normal` with the generics filled with `None` even if there are none (practically
     /// this is not a problem since many more paths have generics than a type anchor).
     BarePath(Interned<ModPath>),
-    /// `Path::Normal` may have empty generics and type anchor (but generic args will be filled with `None`).
+    /// `Path::Normal` will always have either generics or type anchor.
     Normal(NormalPath),
     /// A link to a lang item. It is used in desugaring of things like `it?`. We can show these
     /// links via a normal path since they might be private and not accessible in the usage place.
@@ -211,11 +211,15 @@ impl Path {
                     mod_path.segments()[..mod_path.segments().len() - 1].iter().cloned(),
                 ));
                 let qualifier_generic_args = &generic_args[..generic_args.len() - 1];
-                Some(Path::Normal(NormalPath::new(
-                    type_anchor,
-                    qualifier_mod_path,
-                    qualifier_generic_args.iter().cloned(),
-                )))
+                if type_anchor.is_none() && qualifier_generic_args.iter().all(|it| it.is_none()) {
+                    Some(Path::BarePath(qualifier_mod_path))
+                } else {
+                    Some(Path::Normal(NormalPath::new(
+                        type_anchor,
+                        qualifier_mod_path,
+                        qualifier_generic_args.iter().cloned(),
+                    )))
+                }
             }
             Path::LangItem(..) => None,
         }

crates/hir-def/src/resolver.rs

@@ -3,10 +3,11 @@ use std::{fmt, iter, mem};
 
 use base_db::CrateId;
 use hir_expand::{name::Name, MacroDefId};
-use intern::sym;
+use intern::{sym, Symbol};
 use itertools::Itertools as _;
 use rustc_hash::FxHashSet;
 use smallvec::{smallvec, SmallVec};
+use span::SyntaxContextId;
 use triomphe::Arc;
 
 use crate::{
@@ -342,15 +343,7 @@ impl Resolver {
         }
 
         if n_segments <= 1 {
-            let mut hygiene_info = if !hygiene_id.is_root() {
-                let ctx = hygiene_id.lookup(db);
-                ctx.outer_expn.map(|expansion| {
-                    let expansion = db.lookup_intern_macro_call(expansion);
-                    (ctx.parent, expansion.def)
-                })
-            } else {
-                None
-            };
+            let mut hygiene_info = hygiene_info(db, hygiene_id);
             for scope in self.scopes() {
                 match scope {
                     Scope::ExprScope(scope) => {
@@ -370,19 +363,7 @@ impl Resolver {
                         }
                     }
                     Scope::MacroDefScope(macro_id) => {
-                        if let Some((parent_ctx, label_macro_id)) = hygiene_info {
-                            if label_macro_id == **macro_id {
-                                // A macro is allowed to refer to variables from before its declaration.
-                                // Therefore, if we got to the rib of its declaration, give up its hygiene
-                                // and use its parent expansion.
-                                let parent_ctx = db.lookup_intern_syntax_context(parent_ctx);
-                                hygiene_id = HygieneId::new(parent_ctx.opaque_and_semitransparent);
-                                hygiene_info = parent_ctx.outer_expn.map(|expansion| {
-                                    let expansion = db.lookup_intern_macro_call(expansion);
-                                    (parent_ctx.parent, expansion.def)
-                                });
-                            }
-                        }
+                        handle_macro_def(db, &mut hygiene_id, &mut hygiene_info, macro_id)
                     }
                     Scope::GenericParams { params, def } => {
                         if let Some(id) = params.find_const_by_name(first_name, *def) {
@@ -728,6 +709,107 @@ impl Resolver {
         })
     }
 
+    /// Checks if we rename `renamed` (currently named `current_name`) to `new_name`, will the meaning of this reference
+    /// (that contains `current_name` path) change from `renamed` to some another variable (returned as `Some`).
+    pub fn rename_will_conflict_to_another_variable(
+        &self,
+        db: &dyn DefDatabase,
+        current_name: &Name,
+        current_name_as_path: &ModPath,
+        mut hygiene_id: HygieneId,
+        new_name: &Symbol,
+        renamed: BindingId,
+    ) -> Option<BindingId> {
+        let mut hygiene_info = hygiene_info(db, hygiene_id);
+        let mut will_be_resolved_to = None;
+        for scope in self.scopes() {
+            match scope {
+                Scope::ExprScope(scope) => {
+                    for entry in scope.expr_scopes.entries(scope.scope_id) {
+                        if entry.hygiene() == hygiene_id {
+                            if entry.binding() == renamed {
+                                // This currently resolves to our renamed variable, now `will_be_resolved_to`
+                                // contains `Some` if the meaning will change or `None` if not.
+                                return will_be_resolved_to;
+                            } else if entry.name().symbol() == new_name {
+                                will_be_resolved_to = Some(entry.binding());
+                            }
+                        }
+                    }
+                }
+                Scope::MacroDefScope(macro_id) => {
+                    handle_macro_def(db, &mut hygiene_id, &mut hygiene_info, macro_id)
+                }
+                Scope::GenericParams { params, def } => {
+                    if params.find_const_by_name(current_name, *def).is_some() {
+                        // It does not resolve to our renamed variable.
+                        return None;
+                    }
+                }
+                Scope::AdtScope(_) | Scope::ImplDefScope(_) => continue,
+                Scope::BlockScope(m) => {
+                    if m.resolve_path_in_value_ns(db, current_name_as_path).is_some() {
+                        // It does not resolve to our renamed variable.
+                        return None;
+                    }
+                }
+            }
+        }
+        // It does not resolve to our renamed variable.
+        None
+    }
+
+    /// Checks if we rename `renamed` to `name`, will the meaning of this reference (that contains `name` path) change
+    /// from some other variable (returned as `Some`) to `renamed`.
+    pub fn rename_will_conflict_to_renamed(
+        &self,
+        db: &dyn DefDatabase,
+        name: &Name,
+        name_as_path: &ModPath,
+        mut hygiene_id: HygieneId,
+        renamed: BindingId,
+    ) -> Option<BindingId> {
+        let mut hygiene_info = hygiene_info(db, hygiene_id);
+        let mut will_resolve_to_renamed = false;
+        for scope in self.scopes() {
+            match scope {
+                Scope::ExprScope(scope) => {
+                    for entry in scope.expr_scopes.entries(scope.scope_id) {
+                        if entry.binding() == renamed {
+                            will_resolve_to_renamed = true;
+                        } else if entry.hygiene() == hygiene_id && entry.name() == name {
+                            if will_resolve_to_renamed {
+                                // This will resolve to the renamed variable before it resolves to the original variable.
+                                return Some(entry.binding());
+                            } else {
+                                // This will resolve to the original variable.
+                                return None;
+                            }
+                        }
+                    }
+                }
+                Scope::MacroDefScope(macro_id) => {
+                    handle_macro_def(db, &mut hygiene_id, &mut hygiene_info, macro_id)
+                }
+                Scope::GenericParams { params, def } => {
+                    if params.find_const_by_name(name, *def).is_some() {
+                        // Here and below, it might actually resolve to our renamed variable - in which case it'll
+                        // hide the generic parameter or some other thing (not a variable). We don't check for that
+                        // because due to naming conventions, it is rare that variable will shadow a non-variable.
+                        return None;
+                    }
+                }
+                Scope::AdtScope(_) | Scope::ImplDefScope(_) => continue,
+                Scope::BlockScope(m) => {
+                    if m.resolve_path_in_value_ns(db, name_as_path).is_some() {
+                        return None;
+                    }
+                }
+            }
+        }
+        None
+    }
+
     /// `expr_id` is required to be an expression id that comes after the top level expression scope in the given resolver
     #[must_use]
     pub fn update_to_inner_scope(
@@ -793,6 +875,44 @@ impl Resolver {
     }
 }
 
+#[inline]
+fn handle_macro_def(
+    db: &dyn DefDatabase,
+    hygiene_id: &mut HygieneId,
+    hygiene_info: &mut Option<(SyntaxContextId, MacroDefId)>,
+    macro_id: &MacroDefId,
+) {
+    if let Some((parent_ctx, label_macro_id)) = hygiene_info {
+        if label_macro_id == macro_id {
+            // A macro is allowed to refer to variables from before its declaration.
+            // Therefore, if we got to the rib of its declaration, give up its hygiene
+            // and use its parent expansion.
+            let parent_ctx = db.lookup_intern_syntax_context(*parent_ctx);
+            *hygiene_id = HygieneId::new(parent_ctx.opaque_and_semitransparent);
+            *hygiene_info = parent_ctx.outer_expn.map(|expansion| {
+                let expansion = db.lookup_intern_macro_call(expansion);
+                (parent_ctx.parent, expansion.def)
+            });
+        }
+    }
+}
+
+#[inline]
+fn hygiene_info(
+    db: &dyn DefDatabase,
+    hygiene_id: HygieneId,
+) -> Option<(SyntaxContextId, MacroDefId)> {
+    if !hygiene_id.is_root() {
+        let ctx = hygiene_id.lookup(db);
+        ctx.outer_expn.map(|expansion| {
+            let expansion = db.lookup_intern_macro_call(expansion);
+            (ctx.parent, expansion.def)
+        })
+    } else {
+        None
+    }
+}
+
 pub struct UpdateGuard(usize);
 
 impl Resolver {

crates/hir/src/semantics.rs

@@ -12,7 +12,8 @@ use std::{
 
 use either::Either;
 use hir_def::{
-    hir::{Expr, ExprOrPatId},
+    expr_store::Body,
+    hir::{BindingId, Expr, ExprId, ExprOrPatId, Pat},
     lower::LowerCtx,
     nameres::{MacroSubNs, ModuleOrigin},
     path::ModPath,
@@ -631,6 +632,31 @@ impl<'db> SemanticsImpl<'db> {
         )
     }
 
+    /// Checks if renaming `renamed` to `new_name` may introduce conflicts with other locals,
+    /// and returns the conflicting locals.
+    pub fn rename_conflicts(&self, renamed: &Local, new_name: &str) -> Vec<Local> {
+        let body = self.db.body(renamed.parent);
+        let resolver = renamed.parent.resolver(self.db.upcast());
+        let starting_expr =
+            body.binding_owners.get(&renamed.binding_id).copied().unwrap_or(body.body_expr);
+        let mut visitor = RenameConflictsVisitor {
+            body: &body,
+            conflicts: FxHashSet::default(),
+            db: self.db,
+            new_name: Symbol::intern(new_name),
+            old_name: renamed.name(self.db).symbol().clone(),
+            owner: renamed.parent,
+            renamed: renamed.binding_id,
+            resolver,
+        };
+        visitor.rename_conflicts(starting_expr);
+        visitor
+            .conflicts
+            .into_iter()
+            .map(|binding_id| Local { parent: renamed.parent, binding_id })
+            .collect()
+    }
+
     /// Retrieves all the formatting parts of the format_args! (or `asm!`) template string.
     pub fn as_format_args_parts(
         &self,
@@ -2153,3 +2179,69 @@ impl ops::Deref for VisibleTraits {
         &self.0
     }
 }
+
+struct RenameConflictsVisitor<'a> {
+    db: &'a dyn HirDatabase,
+    owner: DefWithBodyId,
+    resolver: Resolver,
+    body: &'a Body,
+    renamed: BindingId,
+    new_name: Symbol,
+    old_name: Symbol,
+    conflicts: FxHashSet<BindingId>,
+}
+
+impl RenameConflictsVisitor<'_> {
+    fn resolve_path(&mut self, node: ExprOrPatId, path: &Path) {
+        if let Path::BarePath(path) = path {
+            if let Some(name) = path.as_ident() {
+                if *name.symbol() == self.new_name {
+                    if let Some(conflicting) = self.resolver.rename_will_conflict_to_renamed(
+                        self.db.upcast(),
+                        name,
+                        path,
+                        self.body.expr_or_pat_path_hygiene(node),
+                        self.renamed,
+                    ) {
+                        self.conflicts.insert(conflicting);
+                    }
+                } else if *name.symbol() == self.old_name {
+                    if let Some(conflicting) =
+                        self.resolver.rename_will_conflict_to_another_variable(
+                            self.db.upcast(),
+                            name,
+                            path,
+                            self.body.expr_or_pat_path_hygiene(node),
+                            &self.new_name,
+                            self.renamed,
+                        )
+                    {
+                        self.conflicts.insert(conflicting);
+                    }
+                }
+            }
+        }
+    }
+
+    fn rename_conflicts(&mut self, expr: ExprId) {
+        match &self.body[expr] {
+            Expr::Path(path) => {
+                let guard = self.resolver.update_to_inner_scope(self.db.upcast(), self.owner, expr);
+                self.resolve_path(expr.into(), path);
+                self.resolver.reset_to_guard(guard);
+            }
+            &Expr::Assignment { target, .. } => {
+                let guard = self.resolver.update_to_inner_scope(self.db.upcast(), self.owner, expr);
+                self.body.walk_pats(target, &mut |pat| {
+                    if let Pat::Path(path) = &self.body[pat] {
+                        self.resolve_path(pat.into(), path);
+                    }
+                });
+                self.resolver.reset_to_guard(guard);
+            }
+            _ => {}
+        }
+
+        self.body.walk_child_exprs(expr, |expr| self.rename_conflicts(expr));
+    }
+}