[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

338

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 {
  ...
}

339

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.
}

352

nit: VarDecl => NamedDecl

[kbobyrev]

Resolve review comments.

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

339

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

330

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

336

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

343

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

348

nit: can be just return CheckDeclStmt(DS);

353

nit: explicit spell out the auto type?

357

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

362

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 :)

380

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

353

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

331

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

332

const DynTypedNode &

360

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

370

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

376

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

388

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

332

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

409

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/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/AST/ParentMapContext.h"
+#include "clang/AST/Stmt.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) {
+  DynTypedNodeList Parents = Ctx.getParents(RenamedDecl);
+  if (Parents.size() != 1 || !Parents.begin()->get<DeclStmt>())
+    return nullptr;
+  Parents = Ctx.getParents(*Parents.begin()->get<DeclStmt>());
+  if (Parents.size() != 1)
+    return nullptr;
+  // For now, only CompoundStmts are supported;
+  const auto *ParentNode = Parents.begin();
+  const CompoundStmt *EnclosingScope = ParentNode->get<CompoundStmt>();
+  if (const auto *If = ParentNode->get<IfStmt>())
+    if (const auto *Then = dyn_cast<CompoundStmt>(If->getThen()))
+      EnclosingScope = Then;
+  if (const auto *While = ParentNode->get<WhileStmt>())
+    if (const auto *Body = dyn_cast<CompoundStmt>(While->getBody()))
+      EnclosingScope = Body;
+  if (const auto *For = ParentNode->get<ForStmt>())
+    if (const auto *Body = dyn_cast<CompoundStmt>(For->getBody()))
+      EnclosingScope = Body;
+  if (!EnclosingScope)
+    return nullptr;
+  // This helper checks if any statement within DeclStmt has NewName.
+  auto CheckDeclStmt = [&](const DeclStmt *DS) -> const NamedDecl * {
+    for (const auto &Child : DS->getDeclGroup())
+      if (const auto *VD = dyn_cast<VarDecl>(Child))
+        if (VD != &RenamedDecl && VD->getName() == NewName)
+          return VD;
+    return nullptr;
+  };
+  for (const auto *Node : EnclosingScope->children())
+    if (const auto *DS = dyn_cast<DeclStmt>(Node))
+      if (const auto *Result = CheckDeclStmt(DS))
+        return Result;
+  Parents = Ctx.getParents(*EnclosingScope);
+  if (Parents.size() != 1)
+    return nullptr;
+  // This helper checks if there is a condition variable has NewName.
+  auto CheckConditionVariable = [&](const auto *Scope) -> const NamedDecl * {
+    if (!Scope)
+      return nullptr;
+    if (const auto *ConditionDS = Scope->getConditionVariableDeclStmt())
+      if (const auto *Result = CheckDeclStmt(ConditionDS))
+        return Result;
+    return nullptr;
+  };
+  const auto *Parent = Parents.begin();
+  if (const auto *Result = CheckConditionVariable(Parent->get<IfStmt>()))
+    return Result;
+  if (const auto *Result = CheckConditionVariable(Parent->get<WhileStmt>()))
+    return Result;
+  if (const auto *For = Parent->get<ForStmt>())
+    if (const auto *Init = dyn_cast<DeclStmt>(For->getInit()))
+      if (const auto *Result = CheckDeclStmt(Init))
+        return Result;
+  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
   // that don't have any subscopes that are neither DeclContexts nor
   // transparent.
   //
   // Notably, FunctionDecl is excluded -- because local variables are not scoped
   // to the function, but rather to the CompoundStmt that is its body. Lookup
   // will not find function-local variables.
   case Decl::TranslationUnit:
   case Decl::Namespace:
   case Decl::Record:
   case Decl::Enum:
   case Decl::CXXRecord:
     LookupResult = DC->lookup(LookupName);
     break;
   default:
     break;
   }
   // 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]];
+          int V^ar;
+        }
+      )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() {
+          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(// 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.