[PATCH] D98424: [clangd] Reject renames to non-identifier characters

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
sammccall marked an inline comment as done.
Closed by commit rG43d0b1c9c16c: [clangd] Reject renames to non-identifier 
characters (authored by sammccall).
Herald added a project: clang-tools-extra.

Changed prior to commit:
  https://reviews.llvm.org/D98424?vs=329955&id=330933#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98424/new/

https://reviews.llvm.org/D98424

Files:
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp


Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -1240,6 +1240,21 @@
               testing::HasSubstr("keyword"));
   EXPECT_THAT(Tracer.takeMetric("rename_name_invalid", "Keywords"),
               ElementsAre(1));
+
+  for (std::string BadIdent : {"foo!bar", "123foo", "😀@"}) {
+    Results = runPrepareRename(Server, FooCCPath, FooCC.point(),
+                               /*NewName=*/BadIdent, {});
+    EXPECT_FALSE(Results);
+    EXPECT_THAT(llvm::toString(Results.takeError()),
+                testing::HasSubstr("identifier"));
+    EXPECT_THAT(Tracer.takeMetric("rename_name_invalid", "BadIdentifier"),
+                ElementsAre(1));
+  }
+  for (std::string GoodIdent : {"fooBar", "__foo$", "😀"}) {
+    Results = runPrepareRename(Server, FooCCPath, FooCC.point(),
+                               /*NewName=*/GoodIdent, {});
+    EXPECT_TRUE(bool(Results));
+  }
 }
 
 TEST(CrossFileRenameTests, DirtyBuffer) {
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===================================================================
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -22,14 +22,17 @@
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/ParentMapContext.h"
 #include "clang/AST/Stmt.h"
+#include "clang/Basic/CharInfo.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/ADT/StringExtras.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/JSON.h"
 #include <algorithm>
 
 namespace clang {
@@ -178,8 +181,7 @@
   UnsupportedSymbol,
   AmbiguousSymbol,
 
-  // name validation.
-  RenameToKeywords,
+  // name validation. FIXME: reconcile with InvalidName
   SameName,
 };
 
@@ -241,8 +243,6 @@
       return "symbol is not a supported kind (e.g. namespace, macro)";
     case ReasonToReject::AmbiguousSymbol:
       return "there are multiple symbols at the given location";
-    case ReasonToReject::RenameToKeywords:
-      return "the chosen name is a keyword";
     case ReasonToReject::SameName:
       return "new name is the same as the old name";
     }
@@ -437,6 +437,7 @@
   enum Kind {
     Keywords,
     Conflict,
+    BadIdentifier,
   };
   Kind K;
   std::string Details;
@@ -447,6 +448,8 @@
     return "Keywords";
   case InvalidName::Conflict:
     return "Conflict";
+  case InvalidName::BadIdentifier:
+    return "BadIdentifier";
   }
   llvm_unreachable("unhandled InvalidName kind");
 }
@@ -459,12 +462,31 @@
                            Reason.Details);
     case InvalidName::Conflict:
       return llvm::formatv("conflict with the symbol in {0}", Reason.Details);
+    case InvalidName::BadIdentifier:
+      return llvm::formatv("the chosen name \"{0}\" is not a valid identifier",
+                           Reason.Details);
     }
     llvm_unreachable("unhandled InvalidName kind");
   };
   return error("invalid name: {0}", Message(Reason));
 }
 
+static bool mayBeValidIdentifier(llvm::StringRef Ident) {
+  assert(llvm::json::isUTF8(Ident));
+  if (Ident.empty())
+    return false;
+  // We don't check all the rules for non-ascii characters (most are allowed).
+  bool AllowDollar = true; // lenient
+  if (llvm::isASCII(Ident.front()) &&
+      !isIdentifierHead(Ident.front(), AllowDollar))
+    return false;
+  for (char C : Ident) {
+    if (llvm::isASCII(C) && !isIdentifierBody(C, AllowDollar))
+      return false;
+  }
+  return true;
+}
+
 // 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,
@@ -476,6 +498,8 @@
   llvm::Optional<InvalidName> Result;
   if (isKeyword(NewName, ASTCtx.getLangOpts()))
     Result = InvalidName{InvalidName::Keywords, NewName.str()};
+  else if (!mayBeValidIdentifier(NewName))
+    Result = InvalidName{InvalidName::BadIdentifier, NewName.str()};
   else {
     // Name conflict detection.
     // Function conflicts are subtle (overloading), so ignore them.

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits