[clangd] Detect rename conflicits within enclosing scope

[kbobyrev]

Summary

This patch allows detecting conflicts with variables defined in the current
CompoundStmt or If/While/For variable init statements.

[hokein]

Thanks, this looks right to me.

clang-tools-extra/clangd/refactor/Rename.cpp

340

I found that the current structure (we have two places of this If cases, and they are separated) is a bit hard to follow, I'd suggest union them, just like the code below, it seems straight-forward to me, and easy to extend.

if (const auto* CompoundStmtEnclosing = Parents.begin()->get<CompoundStmt>()) {
   CheckCompoundStmt(CompoundStmtEnclosing);
   // below we detect conflicts in conditions or others (if the CompoundStmt is a function body, we could detect the parameter decls as well)
   getParents(CompoundStmtEnclosing);
   if ( ...<IfStmt>()) {
      CheckConditionVariable();
   } else if (...) {
   }
} else if (const auto* IfEnclosing = ... <IfStmt>()) {
   CheckCompoundStmt(If->getThen());
  // possibly CheckCompoundStmt(If->getElse)
} else {
  ...
}

341

thinking more about the if case, I think else should be included as well? no need to address in this patch.

like

if (int a = 0) {
} else {
  int s; // rename s=>a will cause a compiling error.
}

354

nit: VarDecl => NamedDecl

[kbobyrev]

Resolve review comments.

clang-tools-extra/clangd/refactor/Rename.cpp

341

This case is already supported: else's CompoundStmt is attached to the same IfStmt with variable declaration and is checked just like the "main" branch.

The other one wasn't (renaming a into s) but I've added this and also added tests for this.

[kbobyrev]

Save few LOCs by checking for nullptr in CheckDeclStmt.

[kbobyrev]

Don't spell out DynTypedNodeList and don't include ParentMapContext.h

[kbobyrev]

Revert last change: leads to incomplete types.

[hokein]

thanks, looks better now, just some nits to improve the code readability.

clang-tools-extra/clangd/refactor/Rename.cpp

332

since we repeat this code multiple times, I think we can use wrap it with a lambda (e.g. getSingleParent etc).

338

maybe name it LookupInDeclStmt and pass the Name as a parameter, the same below.

345

the same lookupInCompoundStmt.
nit: I would pass Stmt directly, and do the "compoundStmt" cast internally, to save some boilerplate cast in call sides :)

350

nit: can be just return CheckDeclStmt(DS);

355

nit: explicit spell out the auto type?

359

nit: the same here, return CheckDeclStmt (Scope->getConditionVariableDeclStmt()), just adding the nullptr handling in CheckDeclStmt.

364

I understand all cases of these if branches at the moment, but I think we'd better to add some comments, or examples here -- help us understand this code in case that we forget the context in the future :)

382

I think we need a return null here (or use if-else pattern), otherwise, the following if-stmt will be examined as well, which seems no necessary.

[kbobyrev]

Resolve most comments but one: some comments & examples in the code incoming in
the next diff.

[kbobyrev]

clang-tools-extra/clangd/refactor/Rename.cpp

355

That's the idea behind this helper: it's used both on IfStmt and WhileStmt, so the point here is that getConditionVariableDeclStmt() is available in both but not via interface/inheritance.

[kbobyrev]

Add comments.

[hokein]

clang-tools-extra/clangd/refactor/Rename.cpp

333

If the intention is for storage, maybe call it Storage.

334

const DynTypedNode &

362

I think we need to iterate *all* children, rather than the first one. looks like our testcase doesn't cover this, maybe add one.

372

CompoundStmt is the most common enclosing scope for function-local symbols.

378

maybe just GetSingleParent(Parent), rather than creating a new DynTypeNode.

390

be careful about nullptr here, the init-statement could be a nullptr, which will trigger a crash in dyn_cast, thinking about the case below:

for (; ; ;) {
  int var; 
}

use dyn_cast_or_null.

[kbobyrev]

Fix a couple problems pointed out during the review.

clang-tools-extra/clangd/refactor/Rename.cpp

334

Sorry, I'm a bit confused: this can return nullptr when there are _no parents_. Is there any way I can return the reference safely?

[kbobyrev]

Pass const ref to DynTypedNode in helper.

[hokein]

Looks good, thanks!

[hokein]

clang-tools-extra/clangd/refactor/Rename.cpp

411

another case we could add is FunctionDecl, if we rename a parameter decl, we could check collisions in the function body. this can be addressed in a follow-up.

clang-tools-extra/clangd/refactor/Rename.cpp

 #include "AST.h"
 #include "FindTarget.h"
 #include "ParsedAST.h"
 #include "Selection.h"
 #include "SourceCode.h"
 #include "index/SymbolCollector.h"
 #include "support/Logger.h"
 #include "support/Trace.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/ASTTypeTraits.h"
+#include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/AST/ParentMapContext.h"
+#include "clang/AST/Stmt.h"
+#include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
 #include <algorithm>
findExplicitReferences(TopLevelDecl, [&](ReferenceLoc Ref) {
         }
       }
     });
   }
 
   return Results;
 }
 
+// Detect name conflict with othter DeclStmts in the same enclosing scope.
+const NamedDecl *lookupSiblingWithinEnclosingScope(ASTContext &Ctx,
+                                                   const NamedDecl &RenamedDecl,
+                                                   StringRef NewName) {
+  // Store Parents list outside of GetSingleParent, so that returned pointer is
+  // not invalidated.
+  DynTypedNodeList Storage(DynTypedNode::create(RenamedDecl));
+  auto GetSingleParent = [&](const DynTypedNode &Node) -> const DynTypedNode * {
+    Storage = Ctx.getParents(Node);
+    return (Storage.size() == 1) ? Storage.begin() : nullptr;
+  };
+
+  // We need to get to the enclosing scope: NamedDecl's parent is typically
+  // DeclStmt, so enclosing scope would be the second order parent.
+  const auto *Parent = GetSingleParent(DynTypedNode::create(RenamedDecl));
+  if (!Parent || !Parent->get<DeclStmt>())
+    return nullptr;
+  Parent = GetSingleParent(*Parent);
+
+  // The following helpers check corresponding AST nodes for variable
+  // declarations with the name collision.
+  auto CheckDeclStmt = [&](const DeclStmt *DS,
+                           StringRef Name) -> const NamedDecl * {
+    if (!DS)
+      return nullptr;
+    for (const auto &Child : DS->getDeclGroup())
+      if (const auto *ND = dyn_cast<NamedDecl>(Child))
+        if (ND != &RenamedDecl && ND->getName() == Name)
+          return ND;
+    return nullptr;
+  };
+  auto CheckCompoundStmt = [&](const Stmt *S,
+                               StringRef Name) -> const NamedDecl * {
+    if (const auto *CS = dyn_cast_or_null<CompoundStmt>(S))
+      for (const auto *Node : CS->children())
+        if (const auto *Result = CheckDeclStmt(dyn_cast<DeclStmt>(Node), Name))
+          return Result;
+    return nullptr;
+  };
+  auto CheckConditionVariable = [&](const auto *Scope,
+                                    StringRef Name) -> const NamedDecl * {
+    if (!Scope)
+      return nullptr;
+    return CheckDeclStmt(Scope->getConditionVariableDeclStmt(), Name);
+  };
+
+  // CompoundStmt is the most common enclosing scope for function-local symbols
+  // In the simplest case we just iterate through sibling DeclStmts and check
+  // for collisions.
+  if (const auto *EnclosingCS = Parent->get<CompoundStmt>()) {
+    if (const auto *Result = CheckCompoundStmt(EnclosingCS, NewName))
+      return Result;
+    const auto *ScopeParent = GetSingleParent(*Parent);
+    // CompoundStmt may be found within if/while/for. In these cases, rename can
+    // collide with the init-statement variable decalaration, they should be
+    // checked.
+    if (const auto *Result =
+            CheckConditionVariable(ScopeParent->get<IfStmt>(), NewName))
+      return Result;
+    if (const auto *Result =
+            CheckConditionVariable(ScopeParent->get<WhileStmt>(), NewName))
+      return Result;
+    if (const auto *For = ScopeParent->get<ForStmt>())
+      if (const auto *Result = CheckDeclStmt(
+              dyn_cast_or_null<DeclStmt>(For->getInit()), NewName))
+        return Result;
+    // Also check if there is a name collision with function arguments.
+    if (const auto *Function = ScopeParent->get<FunctionDecl>())
+      for (const auto *Parameter : Function->parameters())
+        if (Parameter->getName() == NewName)
+          return Parameter;
+    return nullptr;
+  }
+
+  // When renaming a variable within init-statement within if/while/for
+  // condition, also check the CompoundStmt in the body.
+  if (const auto *EnclosingIf = Parent->get<IfStmt>()) {
+    if (const auto *Result = CheckCompoundStmt(EnclosingIf->getElse(), NewName))
+      return Result;
+    return CheckCompoundStmt(EnclosingIf->getThen(), NewName);
+  }
+  if (const auto *EnclosingWhile = Parent->get<WhileStmt>())
+    return CheckCompoundStmt(EnclosingWhile->getBody(), NewName);
+  if (const auto *EnclosingFor = Parent->get<ForStmt>())
+    return CheckCompoundStmt(EnclosingFor->getBody(), NewName);
+
+  return nullptr;
+}
+
 // Lookup the declarations (if any) with the given Name in the context of
 // RenameDecl.
-const NamedDecl *lookupSiblingWithName(const ASTContext &Ctx,
+const NamedDecl *lookupSiblingsWithinContext(ASTContext &Ctx,
                                              const NamedDecl &RenamedDecl,
-                                       llvm::StringRef Name) {
-  trace::Span Tracer("LookupSiblingWithName");
-  const auto &II = Ctx.Idents.get(Name);
+                                             llvm::StringRef NewName) {
+  const auto &II = Ctx.Idents.get(NewName);
   DeclarationName LookupName(&II);
   DeclContextLookupResult LookupResult;
   const auto *DC = RenamedDecl.getDeclContext();
   while (DC && DC->isTransparentContext())
     DC = DC->getParent();
   switch (DC->getDeclKind()) {
   // The enclosing DeclContext may not be the enclosing scope, it might have
   // false positives and negatives, so we only choose "confident" DeclContexts
const NamedDecl *lookupSiblingsWithinContext(ASTContext &Ctx,
   }
   // Lookup may contain the RenameDecl itself, exclude it.
   for (const auto *D : LookupResult)
     if (D->getCanonicalDecl() != RenamedDecl.getCanonicalDecl())
       return D;
   return nullptr;
 }
 
+const NamedDecl *lookupSiblingWithName(ASTContext &Ctx,
+                                       const NamedDecl &RenamedDecl,
+                                       llvm::StringRef NewName) {
+  trace::Span Tracer("LookupSiblingWithName");
+  if (const auto *Result =
+          lookupSiblingsWithinContext(Ctx, RenamedDecl, NewName))
+    return Result;
+  return lookupSiblingWithinEnclosingScope(Ctx, RenamedDecl, NewName);
+}
+
 struct InvalidName {
   enum Kind {
     Keywords,
     Conflict,
   };
   Kind K;
   std::string Details;
 };

clang-tools-extra/clangd/unittests/RenameTests.cpp

Case Cases[] = {
       {R"cpp(
         int Conflict;
         enum E { // transparent context.
           F^oo,
         };
       )cpp",
        "conflict", !HeaderFile, nullptr, "Conflict"},
 
-      {R"cpp(// FIXME: detecting local variables is not supported yet.
+      {R"cpp(
         void func() {
-          int Conflict;
-          int [[V^ar]];
+          bool Whatever;
+          int V^ar;
+          char Conflict;
+        }
+      )cpp",
+       "conflict", !HeaderFile, nullptr, "Conflict"},
+
+      {R"cpp(
+        void func() {
+          if (int Conflict = 42) {
+            int V^ar;
+          }
         }
       )cpp",
-       nullptr, !HeaderFile, nullptr, "Conflict"},
+       "conflict", !HeaderFile, nullptr, "Conflict"},
+
+      {R"cpp(
+        void func() {
+          if (int Conflict = 42) {
+          } else {
+            bool V^ar;
+          }
+        }
+      )cpp",
+       "conflict", !HeaderFile, nullptr, "Conflict"},
+
+      {R"cpp(
+        void func() {
+          if (int V^ar = 42) {
+          } else {
+            bool Conflict;
+          }
+        }
+      )cpp",
+       "conflict", !HeaderFile, nullptr, "Conflict"},
+
+      {R"cpp(
+        void func() {
+          while (int V^ar = 10) {
+            bool Conflict = true;
+          }
+        }
+      )cpp",
+       "conflict", !HeaderFile, nullptr, "Conflict"},
+
+      {R"cpp(
+        void func() {
+          for (int Something = 9000, Anything = 14, Conflict = 42; Anything > 9;
+               ++Something) {
+            int V^ar;
+          }
+        }
+      )cpp",
+       "conflict", !HeaderFile, nullptr, "Conflict"},
+
+      {R"cpp(
+        void func(int Conflict) {
+          bool V^ar;
+        }
+      )cpp",
+       "conflict", !HeaderFile, nullptr, "Conflict"},
 
       {R"cpp(// Trying to rename into the same name, SameName == SameName.
         void func() {
           int S^ameName;
         }
       )cpp",
        "new name is the same", !HeaderFile, nullptr, "SameName"},
       {R"cpp(// Ensure it doesn't associate base specifier with base name.