[clangd] Add basic conflict detection for the rename.

[hokein]

Summary

With this patch, we reject the rename if the new name would conflict with
any other decls in the decl context of the renamed decl.

[sammccall]

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

263

explain this list somewhat?
e.g. these are the declcontexts which form a namespace where conflicts can occur?
and why function doesn't belong here

263

I think you want to walk up DC while isTransparentContext()

315

Can you explain this a bit more (e.g. why?)

My guess is given:

void foo() {
  int bar;
}

foo is a DC but bar is not declared directly within it (rather within the CompoundStmt that is its body), therefore will not be looked up.

In which case an explanation like "Note that the enclosing DeclContext may not be the enclosing scope, particularly for function-local declarations, so this has both false positives and negatives." might help.

[hokein]

address comments.

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

263

yeah, that's a good point. added.

315

Done. This comment is about why we exclude functionDecl, moved it to the lookup function there.

[sammccall]

Thanks! Just style nits.

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

266

this comment is missing the *reason*.

FunctionDecl is a good example, but the fundamental reason is again not given.
Maybe change that paragraph to:

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.

268

confidence in what?

I think the condition is: don't have any subscopes that are neither DeclContexts nor transparent.

320

This is still a bit confusing: *here* you seem to decide which scope to look up in, but then inside the lookup() function you may change your mind.
I'd suggest moving the getDeclContext() inside the function and renaming the function to "findSiblingWithName" or something.

Also while early-exit is often good, I find early-exit in the *success* case confusing.
Consider:

// Name conflict detection.
// Function conflicts are subtle (overloading), so ignore them.
if (RenameDecl.getKind() != Decl::Function) {
  if (lookupSiblingWithName(ASTCtx, RenameDecl, NewName))
    return InvalidName{...};
}
return llvm::None;

[hokein]

address review comments.

[thakis]

This breaks compillation on windows when using clang as compiler: http://45.33.8.238/win/27605/step_4.txt

[hokein]

In D89790#2385267, @thakis wrote:

This breaks compillation on windows when using clang as compiler: http://45.33.8.238/win/27605/step_4.txt

sorry, looking at it now.

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

findExplicitReferences(TopLevelDecl, [&](ReferenceLoc Ref) {
       }
       Results.push_back(Ref.NameLoc);
     });
   }
 
   return Results;
 }
 
+// Lookup the declarations (if any) with the given Name in the given DC.
+NamedDecl *lookup(const ASTContext &Ctx, const DeclContext *DC,
+                  llvm::StringRef Name) {
+  const auto &II = Ctx.Idents.get(Name);
+  DeclarationName LookupName(&II);
+  DeclContextLookupResult LookupResult;
+  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 the DeclContexts that
+  // we have confidence.
+  // (!) FunctionDecl is excluded, becase it is not the enclosing scope for
+  // function-local variables (performing lookup will not find these variables).
+  case Decl::TranslationUnit:
+  case Decl::Namespace:
+  case Decl::Record:
+  case Decl::Enum:
+  case Decl::CXXRecord:
+    LookupResult = DC->lookup(LookupName);
+    break;
+  default:
+    break;
+  }
+  if (!LookupResult.empty())
+    return LookupResult.front();
+  return nullptr;
+}
+
+struct InvalidName {
+  enum Kind {
+    Keywords,
+    Conflict,
+  };
+  Kind K;
+  std::string Details;
+};
+
+llvm::Error makeError(InvalidName Reason) {
+  auto Message = [](InvalidName Reason) {
+    switch (Reason.K) {
+    case InvalidName::Keywords:
+      return llvm::formatv("the chosen name \"{0}\" is a keyword",
+                           Reason.Details);
+    case InvalidName::Conflict:
+      return llvm::formatv("conflict with the symbol in {0}", Reason.Details);
+    }
+    llvm_unreachable("unhandled InvalidName kind");
+  };
+  return error("invalid name: {0}", Message(Reason));
+}
+
+// Check if we can rename the given RenameDecl into NewName.
+// Return details if the rename would produce a conflict.
+llvm::Optional<InvalidName> checkName(const NamedDecl &RenameDecl,
+                                      llvm::StringRef NewName) {
+  auto &ASTCtx = RenameDecl.getASTContext();
+  if (isKeyword(NewName, ASTCtx.getLangOpts()))
+    return InvalidName{InvalidName::Keywords, NewName.str()};
+  // FIXME: detecting function conflicts is tricky, e.g. we can have a same name
+  // for two overload functions.
+  if (RenameDecl.getKind() == Decl::Function)
+    return llvm::None;
+  // Perform a lookup in the decl context of the RenameDecl, to find out any
+  // conflicts if we perform the rename.
+  auto *Conflict = lookup(ASTCtx, RenameDecl.getDeclContext(), NewName);
+  if (!Conflict)
+    return llvm::None;
+  return InvalidName{
+      InvalidName::Conflict,
+      Conflict->getLocation().printToString(ASTCtx.getSourceManager())};
+}
+
 // AST-based rename, it renames all occurrences in the main file.
 llvm::Expected<tooling::Replacements>
 renameWithinFile(ParsedAST &AST, const NamedDecl &RenameDecl,
                  llvm::StringRef NewName) {
   trace::Span Tracer("RenameWithinFile");
   const SourceManager &SM = AST.getSourceManager();
 
   tooling::Replacements FilteredChanges;
llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
   if (locateMacroAt(*IdentifierToken, AST.getPreprocessor()))
     return makeError(ReasonToReject::UnsupportedSymbol);
 
   auto DeclsUnderCursor = locateDeclAt(AST, IdentifierToken->location());
   if (DeclsUnderCursor.empty())
     return makeError(ReasonToReject::NoSymbolFound);
   if (DeclsUnderCursor.size() > 1)
     return makeError(ReasonToReject::AmbiguousSymbol);
-  if (isKeyword(RInputs.NewName, AST.getLangOpts()))
-    return makeError(ReasonToReject::RenameToKeywords);
-
   const auto &RenameDecl =
       llvm::cast<NamedDecl>(*(*DeclsUnderCursor.begin())->getCanonicalDecl());
+  auto Invalid = checkName(RenameDecl, RInputs.NewName);
+  if (Invalid)
+    return makeError(*Invalid);
+
   auto Reject = renameable(RenameDecl, RInputs.MainFilePath, RInputs.Index,
                            Opts.AllowCrossFile);
   if (Reject)
     return makeError(*Reject);
 
   // We have two implementations of the rename:
   //   - AST-based rename: used for renaming local symbols, e.g. variables
   //     defined in a function body;

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

Case Cases[] = {
        )cpp",
        "no symbol", !HeaderFile, nullptr},
 
       {R"cpp(// disallow rename on unrelated token.
          temp^late<typename T>
          class Foo {};
        )cpp",
        "no symbol", !HeaderFile, nullptr},
+
+      {R"cpp(
+        namespace {
+        int Conflict;
+        int Va^r;
+        }
+      )cpp",
+       "conflict", !HeaderFile, nullptr, "Conflict"},
+
+      {R"cpp(
+        int Conflict;
+        int Va^r;
+      )cpp",
+       "conflict", !HeaderFile, nullptr, "Conflict"},
+
+      {R"cpp(
+        class Foo {
+          int Conflict;
+          int Va^r;
+        };
+      )cpp",
+       "conflict", !HeaderFile, nullptr, "Conflict"},
+
+      {R"cpp(
+        enum E {
+          Conflict,
+          Fo^o,
+        };
+      )cpp",
+       "conflict", !HeaderFile, nullptr, "Conflict"},
+
+      {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.
+        void func() {
+          int Conflict;
+          int [[V^ar]];
+        }
+      )cpp",
+       nullptr, !HeaderFile, nullptr, "Conflict"},
   };
 
   for (const auto& Case : Cases) {
     SCOPED_TRACE(Case.Code);
     Annotations T(Case.Code);
     TestTU TU = TestTU::withCode(T.code());
     TU.HeaderCode = CommonHeader;
     TU.ExtraArgs.push_back("-fno-delayed-template-parsing");