[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)

[Alex Hoppen via cfe-commits]

https://github.com/ahoppen commented:

The outstanding comments from last review are:
- Use `SymbolName` instead of `StringRef`
  - https://github.com/llvm/llvm-project/pull/76466#discussion_r1476409221
  - https://github.com/llvm/llvm-project/pull/76466#discussion_r1476427027
- Don’t re-lex the source file in `collectRenameIdentifierRanges`
  - https://github.com/llvm/llvm-project/pull/76466#discussion_r1476471971
- Use index or AST data to find edits in the current file. AFAICT we do use 
index-data for the multi-file rename case in `adjustRenameRanges` but I AFAICT 
no semantic information is used for the current file.
  - https://github.com/llvm/llvm-project/pull/76466/files#r1479029824
  - I think a good test case to add would be
```cpp
// Input
R"cpp(
  @interface Foo
  - (void)performA^ction:(int)action with:(int)value;
  @end
  @implementation Foo
  - (void)performAction:(int)action with:(int)value {
[self performAction:action with:value];
  }
  @end
  @interface Bar
  - (void)performAction:(int)action with:(int)value;
  @end
  @implementation Bar
  - (void)performAction:(int)action with:(int)value {
  }
  @end
)cpp",
// New name
"performNewAction:by:",
// Expected
R"cpp(
  @interface Foo
  - (void)performNewAction:(int)action by:(int)value;
  @end
  @implementation Foo
  - (void)performNewAction:(int)action by:(int)value {
[self performNewAction:action by:value];
  }
  @end
  @interface Bar
  - (void)performAction:(int)action with:(int)value;
  @end
  @implementation Bar
  - (void)performAction:(int)action with:(int)value {
  }
)cpp",
```

https://github.com/llvm/llvm-project/pull/76466
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)

[Alex Hoppen via cfe-commits]

https://github.com/ahoppen edited 
https://github.com/llvm/llvm-project/pull/76466
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)

[Alex Hoppen via cfe-commits]

@@ -538,11 +564,222 @@ std::optional checkName(const NamedDecl 
&RenameDecl,
 Conflict->getLocation().printToString(ASTCtx.getSourceManager())};
 }
   }
-  if (Result)
+  if (Result) {
 InvalidNameMetric.record(1, toString(Result->K));
+return makeError(*Result);
+  }
+  return llvm::Error::success();
+}
+
+bool isSelectorLike(const syntax::Token &Cur, const syntax::Token &Next) {
+  return Cur.kind() == tok::identifier && Next.kind() == tok::colon &&
+ // We require the selector name and : to be contiguous.
+ // e.g. support `foo:` but not `foo :`.
+ Cur.endLocation() == Next.location();
+}
+
+bool isMatchingSelectorName(const syntax::Token &Cur, const syntax::Token 
&Next,
+const SourceManager &SM,
+llvm::StringRef SelectorName) {
+  if (SelectorName.empty())
+return Cur.kind() == tok::colon;
+  return isSelectorLike(Cur, Next) && Cur.text(SM) == SelectorName;
+}
+
+// Scan through Tokens to find ranges for each selector fragment in Sel at the
+// top level (not nested in any () or {} or []). The search will terminate upon

ahoppen wrote:

I think the `not nested in any () or {} or []` is quite misleading. Based on 
this comment, I would have expected `doStuff` in `[someObject someArgLabel: 
[foo doStuff: 1]]` or 

```objectivec
- (void) test {
  [foo doStuff: 1];
}
```

to not be found because it’s nested in `{}` or `[]`.

https://github.com/llvm/llvm-project/pull/76466
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)

[Alex Hoppen via cfe-commits]

@@ -538,11 +564,222 @@ std::optional checkName(const NamedDecl 
&RenameDecl,
 Conflict->getLocation().printToString(ASTCtx.getSourceManager())};
 }
   }
-  if (Result)
+  if (Result) {
 InvalidNameMetric.record(1, toString(Result->K));
+return makeError(*Result);
+  }
+  return llvm::Error::success();
+}
+
+bool isSelectorLike(const syntax::Token &Cur, const syntax::Token &Next) {
+  return Cur.kind() == tok::identifier && Next.kind() == tok::colon &&
+ // We require the selector name and : to be contiguous.
+ // e.g. support `foo:` but not `foo :`.
+ Cur.endLocation() == Next.location();
+}
+
+bool isMatchingSelectorName(const syntax::Token &Cur, const syntax::Token 
&Next,
+const SourceManager &SM,
+llvm::StringRef SelectorName) {
+  if (SelectorName.empty())
+return Cur.kind() == tok::colon;
+  return isSelectorLike(Cur, Next) && Cur.text(SM) == SelectorName;
+}
+
+// Scan through Tokens to find ranges for each selector fragment in Sel at the
+// top level (not nested in any () or {} or []). The search will terminate upon
+// seeing Terminator or a ; at the top level.
+std::optional
+findAllSelectorPieces(llvm::ArrayRef Tokens,
+  const SourceManager &SM, Selector Sel,
+  tok::TokenKind Terminator) {
+
+  unsigned NumArgs = Sel.getNumArgs();
+  llvm::SmallVector Closes;
+  std::vector SelectorPieces;
+  unsigned Last = Tokens.size() - 1;
+  for (unsigned Index = 0; Index < Last; ++Index) {
+const auto &Tok = Tokens[Index];
+
+if (Closes.empty()) {
+  auto PieceCount = SelectorPieces.size();
+  if (PieceCount < NumArgs &&
+  isMatchingSelectorName(Tok, Tokens[Index + 1], SM,
+ Sel.getNameForSlot(PieceCount))) {
+// If 'foo:' instead of ':' (empty selector), we need to skip the ':'
+// token after the name.
+if (!Sel.getNameForSlot(PieceCount).empty())
+  ++Index;
+SelectorPieces.push_back(
+halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
+continue;
+  }
+  // If we've found all pieces but the current token looks like another
+  // selector piece, it means the method being renamed is a strict prefix 
of
+  // the selector we've found - should be skipped.
+  if (SelectorPieces.size() >= NumArgs &&
+  isSelectorLike(Tok, Tokens[Index + 1]))
+return std::nullopt;
+}
+
+if (Closes.empty() && Tok.kind() == Terminator)
+  return SelectorPieces.size() == NumArgs
+ ? std::optional(SymbolRange(SelectorPieces))
+ : std::nullopt;
+
+switch (Tok.kind()) {
+case tok::l_square:
+  Closes.push_back(tok::r_square);
+  break;
+case tok::l_paren:
+  Closes.push_back(tok::r_paren);
+  break;
+case tok::l_brace:
+  Closes.push_back(tok::r_brace);
+  break;
+case tok::r_square:
+case tok::r_paren:
+case tok::r_brace:
+  if (Closes.empty() || Closes.back() != Tok.kind())
+return std::nullopt;
+  Closes.pop_back();
+  break;
+case tok::semi:
+  // top level ; terminates all statements.
+  if (Closes.empty())
+return SelectorPieces.size() == NumArgs
+   ? std::optional(SymbolRange(SelectorPieces))
+   : std::nullopt;
+  break;
+default:
+  break;
+}
+  }
+  return std::nullopt;
+}
+
+/// Collects all ranges of the given identifier/selector in the source code.
+///
+/// If a selector is given, this does a full lex of the given source code in
+/// order to identify all selector fragments (e.g. in method exprs/decls) since
+/// they are non-contiguous.
+std::vector collectRenameIdentifierRanges(
+llvm::StringRef Identifier, llvm::StringRef Content,
+const LangOptions &LangOpts, std::optional Selector) {
+  std::vector Ranges;
+  if (!Selector) {
+auto IdentifierRanges =
+collectIdentifierRanges(Identifier, Content, LangOpts);
+for (const auto &R : IdentifierRanges)
+  Ranges.emplace_back(R);
+return Ranges;
+  }
+  // FIXME: InMemoryFileAdapter crashes unless the buffer is null terminated!

ahoppen wrote:

In my rename PR I found that this is no longer an issue. See 
https://github.com/apple/llvm-project/pull/7915#discussion_r1442402332

https://github.com/llvm/llvm-project/pull/76466
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)

[Alex Hoppen via cfe-commits]

@@ -538,11 +564,222 @@ std::optional checkName(const NamedDecl 
&RenameDecl,
 Conflict->getLocation().printToString(ASTCtx.getSourceManager())};
 }
   }
-  if (Result)
+  if (Result) {
 InvalidNameMetric.record(1, toString(Result->K));
+return makeError(*Result);
+  }
+  return llvm::Error::success();
+}
+
+bool isSelectorLike(const syntax::Token &Cur, const syntax::Token &Next) {
+  return Cur.kind() == tok::identifier && Next.kind() == tok::colon &&
+ // We require the selector name and : to be contiguous.
+ // e.g. support `foo:` but not `foo :`.

ahoppen wrote:

Why don’t we support `foo : `? Or what am I missing here? `[bar foo : 1]` is 
perfectly valid AFAICT.

https://github.com/llvm/llvm-project/pull/76466
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-tools-extra] [clangd] Use `SymbolName` to represent Objective-C selectors (PR #82061)

[Alex Hoppen via cfe-commits]

ahoppen wrote:

For some context: When I’m talking about * finding the ranges to rename based 
on an index that’s not clangd’s built-in index* I meant a request like 
https://github.com/apple/llvm-project/pull/7973. This allows us to use Apple’s 
IndexStore to find the locations of symbols to rename and then invoke clangd to 
get the edits to rename the symbols and deal with the parsing of Objective-C 
selector pieces. 

Functionality-wise, I would be fine with using a `optional>` 
instead of `Selector` in `collectRenameIdentifierRanges`. FWIW I disagree that 
using a `vector` is cleaner than using a dedicated type for it because 
there’s no type-level information about what the `vector` represents 
and that `SymbolName` could be made to some day support the cases you mention 
in https://github.com/llvm/llvm-project/pull/82061#discussion_r1500161008. But 
I’m new to clangd’s development and will follow your guidance here if you 
prefer `vector`. 

https://github.com/llvm/llvm-project/pull/82061
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang] [clangd] Add support to rename Objective-C selectors (PR #78872)

[Alex Hoppen via cfe-commits]

https://github.com/ahoppen updated 
https://github.com/llvm/llvm-project/pull/78872

>From 26d9d1a5a342df34cde921f7d9921e7b94dadd87 Mon Sep 17 00:00:00 2001
From: Alex Hoppen 
Date: Tue, 19 Dec 2023 15:54:40 -0800
Subject: [PATCH 1/5] [Refactoring] Add capabilities to `SymbolName` to
 represent Objective-C selector names

---
 .../Tooling/Refactoring/Rename/SymbolName.h   | 30 +++-
 clang/lib/Tooling/Refactoring/CMakeLists.txt  |  1 +
 .../Refactoring/Rename/RenamingAction.cpp |  4 +-
 .../Refactoring/Rename/USRLocFinder.cpp   |  4 +-
 clang/lib/Tooling/Refactoring/SymbolName.cpp  | 46 +++
 clang/tools/clang-rename/CMakeLists.txt   |  1 +
 6 files changed, 70 insertions(+), 16 deletions(-)
 create mode 100644 clang/lib/Tooling/Refactoring/SymbolName.cpp

diff --git a/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h 
b/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h
index 6c28d40f3679c..077f315f657e7 100644
--- a/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h
+++ b/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h
@@ -15,6 +15,9 @@
 #include "llvm/ADT/StringRef.h"
 
 namespace clang {
+
+class LangOptions;
+
 namespace tooling {
 
 /// A name of a symbol.
@@ -27,19 +30,22 @@ namespace tooling {
 /// //   ^~ string 0 ~ ^~ string 1 ~
 /// \endcode
 class SymbolName {
+  llvm::SmallVector NamePieces;
+
 public:
-  explicit SymbolName(StringRef Name) {
-// While empty symbol names are valid (Objective-C selectors can have empty
-// name pieces), occurrences Objective-C selectors are created using an
-// array of strings instead of just one string.
-assert(!Name.empty() && "Invalid symbol name!");
-this->Name.push_back(Name.str());
-  }
-
-  ArrayRef getNamePieces() const { return Name; }
-
-private:
-  llvm::SmallVector Name;
+  /// Create a new \c SymbolName with the specified pieces.
+  explicit SymbolName(ArrayRef NamePieces);
+
+  /// Creates a \c SymbolName from the given string representation.
+  ///
+  /// For Objective-C symbol names, this splits a selector into multiple pieces
+  /// on `:`. For all other languages the name is used as the symbol name.
+  SymbolName(StringRef Name, bool IsObjectiveCSelector);
+  SymbolName(StringRef Name, const LangOptions &LangOpts);
+
+  ArrayRef getNamePieces() const { return NamePieces; }
+
+  void print(raw_ostream &OS) const;
 };
 
 } // end namespace tooling
diff --git a/clang/lib/Tooling/Refactoring/CMakeLists.txt 
b/clang/lib/Tooling/Refactoring/CMakeLists.txt
index d3077be8810aa..a4a80ce834431 100644
--- a/clang/lib/Tooling/Refactoring/CMakeLists.txt
+++ b/clang/lib/Tooling/Refactoring/CMakeLists.txt
@@ -13,6 +13,7 @@ add_clang_library(clangToolingRefactoring
   Rename/USRFinder.cpp
   Rename/USRFindingAction.cpp
   Rename/USRLocFinder.cpp
+  SymbolName.cpp
 
   LINK_LIBS
   clangAST
diff --git a/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp 
b/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp
index 72598601d47d6..4965977d1f7aa 100644
--- a/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp
+++ b/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp
@@ -82,7 +82,7 @@ 
RenameOccurrences::createSourceReplacements(RefactoringRuleContext &Context) {
   if (!Occurrences)
 return Occurrences.takeError();
   // FIXME: Verify that the new name is valid.
-  SymbolName Name(NewName);
+  SymbolName Name(NewName, /*IsObjectiveCSelector=*/false);
   return createRenameReplacements(
   *Occurrences, Context.getASTContext().getSourceManager(), Name);
 }
@@ -219,7 +219,7 @@ class RenamingASTConsumer : public ASTConsumer {
 }
 // FIXME: Support multi-piece names.
 // FIXME: better error handling (propagate error out).
-SymbolName NewNameRef(NewName);
+SymbolName NewNameRef(NewName, /*IsObjectiveCSelector=*/false);
 Expected> Change =
 createRenameReplacements(Occurrences, SourceMgr, NewNameRef);
 if (!Change) {
diff --git a/clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp 
b/clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
index c18f9290471fe..43e48f24caa9e 100644
--- a/clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
+++ b/clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
@@ -60,8 +60,8 @@ class USRLocFindingASTVisitor
const ASTContext &Context)
   : RecursiveSymbolVisitor(Context.getSourceManager(),
Context.getLangOpts()),
-USRSet(USRs.begin(), USRs.end()), PrevName(PrevName), Context(Context) 
{
-  }
+USRSet(USRs.begin(), USRs.end()),
+PrevName(PrevName, /*IsObjectiveCSelector=*/false), Context(Context) {}
 
   bool visitSymbolOccurrence(const NamedDecl *ND,
  ArrayRef NameRanges) {
diff --git a/clang/lib/Tooling/Refactoring/SymbolName.cpp 
b/clang/lib/Tooling/Refactoring/SymbolName.cpp
new file mode 100644
index 0..7fd50b2c4df7b
--- /

[clang] [clang-tools-extra] [clangd] Add support to rename Objective-C selectors (PR #78872)

[Alex Hoppen via cfe-commits]

https://github.com/ahoppen updated 
https://github.com/llvm/llvm-project/pull/78872

>From 26d9d1a5a342df34cde921f7d9921e7b94dadd87 Mon Sep 17 00:00:00 2001
From: Alex Hoppen 
Date: Tue, 19 Dec 2023 15:54:40 -0800
Subject: [PATCH 1/5] [Refactoring] Add capabilities to `SymbolName` to
 represent Objective-C selector names

---
 .../Tooling/Refactoring/Rename/SymbolName.h   | 30 +++-
 clang/lib/Tooling/Refactoring/CMakeLists.txt  |  1 +
 .../Refactoring/Rename/RenamingAction.cpp |  4 +-
 .../Refactoring/Rename/USRLocFinder.cpp   |  4 +-
 clang/lib/Tooling/Refactoring/SymbolName.cpp  | 46 +++
 clang/tools/clang-rename/CMakeLists.txt   |  1 +
 6 files changed, 70 insertions(+), 16 deletions(-)
 create mode 100644 clang/lib/Tooling/Refactoring/SymbolName.cpp

diff --git a/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h 
b/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h
index 6c28d40f3679c..077f315f657e7 100644
--- a/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h
+++ b/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h
@@ -15,6 +15,9 @@
 #include "llvm/ADT/StringRef.h"
 
 namespace clang {
+
+class LangOptions;
+
 namespace tooling {
 
 /// A name of a symbol.
@@ -27,19 +30,22 @@ namespace tooling {
 /// //   ^~ string 0 ~ ^~ string 1 ~
 /// \endcode
 class SymbolName {
+  llvm::SmallVector NamePieces;
+
 public:
-  explicit SymbolName(StringRef Name) {
-// While empty symbol names are valid (Objective-C selectors can have empty
-// name pieces), occurrences Objective-C selectors are created using an
-// array of strings instead of just one string.
-assert(!Name.empty() && "Invalid symbol name!");
-this->Name.push_back(Name.str());
-  }
-
-  ArrayRef getNamePieces() const { return Name; }
-
-private:
-  llvm::SmallVector Name;
+  /// Create a new \c SymbolName with the specified pieces.
+  explicit SymbolName(ArrayRef NamePieces);
+
+  /// Creates a \c SymbolName from the given string representation.
+  ///
+  /// For Objective-C symbol names, this splits a selector into multiple pieces
+  /// on `:`. For all other languages the name is used as the symbol name.
+  SymbolName(StringRef Name, bool IsObjectiveCSelector);
+  SymbolName(StringRef Name, const LangOptions &LangOpts);
+
+  ArrayRef getNamePieces() const { return NamePieces; }
+
+  void print(raw_ostream &OS) const;
 };
 
 } // end namespace tooling
diff --git a/clang/lib/Tooling/Refactoring/CMakeLists.txt 
b/clang/lib/Tooling/Refactoring/CMakeLists.txt
index d3077be8810aa..a4a80ce834431 100644
--- a/clang/lib/Tooling/Refactoring/CMakeLists.txt
+++ b/clang/lib/Tooling/Refactoring/CMakeLists.txt
@@ -13,6 +13,7 @@ add_clang_library(clangToolingRefactoring
   Rename/USRFinder.cpp
   Rename/USRFindingAction.cpp
   Rename/USRLocFinder.cpp
+  SymbolName.cpp
 
   LINK_LIBS
   clangAST
diff --git a/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp 
b/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp
index 72598601d47d6..4965977d1f7aa 100644
--- a/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp
+++ b/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp
@@ -82,7 +82,7 @@ 
RenameOccurrences::createSourceReplacements(RefactoringRuleContext &Context) {
   if (!Occurrences)
 return Occurrences.takeError();
   // FIXME: Verify that the new name is valid.
-  SymbolName Name(NewName);
+  SymbolName Name(NewName, /*IsObjectiveCSelector=*/false);
   return createRenameReplacements(
   *Occurrences, Context.getASTContext().getSourceManager(), Name);
 }
@@ -219,7 +219,7 @@ class RenamingASTConsumer : public ASTConsumer {
 }
 // FIXME: Support multi-piece names.
 // FIXME: better error handling (propagate error out).
-SymbolName NewNameRef(NewName);
+SymbolName NewNameRef(NewName, /*IsObjectiveCSelector=*/false);
 Expected> Change =
 createRenameReplacements(Occurrences, SourceMgr, NewNameRef);
 if (!Change) {
diff --git a/clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp 
b/clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
index c18f9290471fe..43e48f24caa9e 100644
--- a/clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
+++ b/clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
@@ -60,8 +60,8 @@ class USRLocFindingASTVisitor
const ASTContext &Context)
   : RecursiveSymbolVisitor(Context.getSourceManager(),
Context.getLangOpts()),
-USRSet(USRs.begin(), USRs.end()), PrevName(PrevName), Context(Context) 
{
-  }
+USRSet(USRs.begin(), USRs.end()),
+PrevName(PrevName, /*IsObjectiveCSelector=*/false), Context(Context) {}
 
   bool visitSymbolOccurrence(const NamedDecl *ND,
  ArrayRef NameRanges) {
diff --git a/clang/lib/Tooling/Refactoring/SymbolName.cpp 
b/clang/lib/Tooling/Refactoring/SymbolName.cpp
new file mode 100644
index 0..7fd50b2c4df7b
--- /

[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)

[Alex Hoppen via cfe-commits]

@@ -538,11 +565,254 @@ std::optional checkName(const NamedDecl 
&RenameDecl,
 Conflict->getLocation().printToString(ASTCtx.getSourceManager())};
 }
   }
-  if (Result)
+  if (Result) {
 InvalidNameMetric.record(1, toString(Result->K));
+return makeError(*Result);
+  }
+  return std::nullopt;
+}
+
+bool isMatchingSelectorName(const syntax::Token &Cur, const syntax::Token 
&Next,
+const SourceManager &SM,
+llvm::StringRef SelectorName) {
+  if (SelectorName.empty())
+return Cur.kind() == tok::colon;
+  return Cur.kind() == tok::identifier && Next.kind() == tok::colon &&
+ Cur.text(SM) == SelectorName &&
+ // We require the selector name and : to be contiguous to avoid
+ // potential conflicts with ternary expression.
+ //
+ // e.g. support `foo:` but not `foo :`.
+ Cur.endLocation() == Next.location();
+}
+
+bool isSelectorLike(const syntax::Token &Cur, const syntax::Token &Next) {
+  return Cur.kind() == tok::identifier && Next.kind() == tok::colon &&
+ // We require the selector name and : to be contiguous.
+ // e.g. support `foo:` but not `foo :`.
+ Cur.endLocation() == Next.location();
+}
+
+bool parseMessageExpression(llvm::ArrayRef Tokens,
+const SourceManager &SM, unsigned Index,
+unsigned Last, Selector Sel,
+std::vector &SelectorPieces) {
+
+  unsigned NumArgs = Sel.getNumArgs();
+  llvm::SmallVector Closes;
+  SelectorPieces.clear();
+  while (Index < Last) {
+const auto &Tok = Tokens[Index];
+
+if (Closes.empty()) {
+  auto PieceCount = SelectorPieces.size();
+  if (PieceCount < NumArgs &&
+  isMatchingSelectorName(Tok, Tokens[Index + 1], SM,
+ Sel.getNameForSlot(PieceCount))) {
+// If 'foo:' instead of ':' (empty selector), we need to skip the ':'
+// token after the name.
+if (!Sel.getNameForSlot(PieceCount).empty()) {
+  ++Index;
+}
+SelectorPieces.push_back(
+halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
+continue;
+  }
+  // If we've found all pieces but the current token looks like another
+  // selector piece, it means the method being renamed is a strict prefix 
of
+  // the selector we've found - should be skipped.
+  if (SelectorPieces.size() >= NumArgs &&
+  isSelectorLike(Tok, Tokens[Index + 1]))
+return false;
+}
+
+switch (Tok.kind()) {
+case tok::l_square:
+  Closes.push_back(']');
+  break;
+case tok::l_paren:
+  Closes.push_back(')');
+  break;
+case tok::l_brace:
+  Closes.push_back('}');
+  break;
+case tok::r_square:
+  if (Closes.empty())
+return SelectorPieces.size() == NumArgs;
+
+  if (Closes.back() != ']')
+return false;
+  Closes.pop_back();
+  break;
+case tok::r_paren:
+  if (Closes.empty() || Closes.back() != ')')
+return false;
+  Closes.pop_back();
+  break;
+case tok::r_brace:
+  if (Closes.empty() || Closes.back() != '}')
+return false;
+  Closes.pop_back();
+  break;
+case tok::semi:
+  // top level ; ends all statements.
+  if (Closes.empty())
+return false;
+  break;
+default:
+  break;
+}
+
+++Index;
+  }
+  return false;
+}
+
+/// Collects all ranges of the given identifier/selector in the source code.
+///
+/// If a selector is given, this does a full lex of the given source code in
+/// order to identify all selector fragments (e.g. in method exprs/decls) since
+/// they are non-contiguous.
+std::vector collectRenameIdentifierRanges(
+llvm::StringRef Identifier, llvm::StringRef Content,
+const LangOptions &LangOpts, std::optional Selector) {
+  std::vector Ranges;
+  if (!Selector) {
+auto IdentifierRanges =
+collectIdentifierRanges(Identifier, Content, LangOpts);
+for (const auto &R : IdentifierRanges)
+  Ranges.emplace_back(R);
+return Ranges;
+  }
+  // FIXME: InMemoryFileAdapter crashes unless the buffer is null terminated!
+  std::string NullTerminatedCode = Content.str();
+  SourceManagerForFile FileSM("mock_file_name.cpp", NullTerminatedCode);
+  auto &SM = FileSM.get();
+
+  // We track parens and braces to ensure that we don't accidentally try 
parsing
+  // a method declaration or definition which isn't at the top level or similar
+  // looking expressions (e.g. an @selector() expression).
+  unsigned ParenCount = 0;
+  unsigned BraceCount = 0;
+  unsigned NumArgs = Selector->getNumArgs();
+
+  std::vector SelectorPieces;
+  auto Tokens = syntax::tokenize(SM.getMainFileID(), SM, LangOpts);
+  unsigned Last = Tokens.size() - 1;
+  for (unsigned Index = 0; Index < Last; ++Index) {
+const auto &Tok = Tokens[Index];
+
+if (BraceCou

[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)

[Alex Hoppen via cfe-commits]

@@ -538,11 +565,254 @@ std::optional checkName(const NamedDecl 
&RenameDecl,
 Conflict->getLocation().printToString(ASTCtx.getSourceManager())};
 }
   }
-  if (Result)
+  if (Result) {
 InvalidNameMetric.record(1, toString(Result->K));
+return makeError(*Result);
+  }
+  return std::nullopt;
+}
+
+bool isMatchingSelectorName(const syntax::Token &Cur, const syntax::Token 
&Next,
+const SourceManager &SM,
+llvm::StringRef SelectorName) {
+  if (SelectorName.empty())
+return Cur.kind() == tok::colon;
+  return Cur.kind() == tok::identifier && Next.kind() == tok::colon &&
+ Cur.text(SM) == SelectorName &&
+ // We require the selector name and : to be contiguous to avoid
+ // potential conflicts with ternary expression.
+ //
+ // e.g. support `foo:` but not `foo :`.
+ Cur.endLocation() == Next.location();
+}
+
+bool isSelectorLike(const syntax::Token &Cur, const syntax::Token &Next) {
+  return Cur.kind() == tok::identifier && Next.kind() == tok::colon &&
+ // We require the selector name and : to be contiguous.
+ // e.g. support `foo:` but not `foo :`.
+ Cur.endLocation() == Next.location();
+}
+
+bool parseMessageExpression(llvm::ArrayRef Tokens,

ahoppen wrote:

I think a doc comment of what this method does on a high level would be very 
helpful.

https://github.com/llvm/llvm-project/pull/76466
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)

[Alex Hoppen via cfe-commits]

@@ -681,12 +957,22 @@ renameOutsideFile(const NamedDecl &RenameDecl, 
llvm::StringRef MainFilePath,
ExpBuffer.getError().message());
   continue;
 }
+std::string RenameIdentifier = RenameDecl.getNameAsString();
+std::optional Selector = std::nullopt;
+llvm::SmallVector NewNames;
+if (const auto *MD = dyn_cast(&RenameDecl)) {

ahoppen wrote:

A high-level comment. Instead of dealing with a vector of `StringRef` for 
`NewNames`, I think we should use `SymbolName`. It is already designed to be 
able to represent Objective-C selectors and that way you could do the name 
splitting in `SymbolName`.

https://github.com/llvm/llvm-project/pull/76466
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)

[Alex Hoppen via cfe-commits]

ahoppen wrote:

I think we could also include my tests from 
https://github.com/llvm/llvm-project/pull/78872/files#diff-26ff7c74af8a1d882abbad43625d3cd4bf8d024ef5c7064993f5ede3eec8752eR834

More tests never hurt.

https://github.com/llvm/llvm-project/pull/76466
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)

[Alex Hoppen via cfe-commits]

@@ -681,12 +957,22 @@ renameOutsideFile(const NamedDecl &RenameDecl, 
llvm::StringRef MainFilePath,
ExpBuffer.getError().message());
   continue;
 }
+std::string RenameIdentifier = RenameDecl.getNameAsString();

ahoppen wrote:

Technically `RenameIdentifier` isn’t correct here because this can be an 
Objective-C selector name, right? Which would be multiple identifiers and 
colons.

https://github.com/llvm/llvm-project/pull/76466
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)

[Alex Hoppen via cfe-commits]

@@ -508,24 +513,46 @@ static bool mayBeValidIdentifier(llvm::StringRef Ident) {
   !isAsciiIdentifierStart(Ident.front(), AllowDollar))
 return false;
   for (char C : Ident) {
+if (AllowColon && C == ':')
+  continue;
 if (llvm::isASCII(C) && !isAsciiIdentifierContinue(C, AllowDollar))
   return false;
   }
   return true;
 }
 
+std::string getName(const NamedDecl &RenameDecl) {
+  if (const auto *MD = dyn_cast(&RenameDecl))
+return MD->getSelector().getAsString();
+  if (const auto *ID = RenameDecl.getIdentifier())
+return ID->getName().str();
+  return "";
+}
+
 // Check if we can rename the given RenameDecl into NewName.
 // Return details if the rename would produce a conflict.
-std::optional checkName(const NamedDecl &RenameDecl,
- llvm::StringRef NewName) {
+std::optional checkName(const NamedDecl &RenameDecl,
+ llvm::StringRef NewName,
+ llvm::StringRef OldName) {
   trace::Span Tracer("CheckName");
   static constexpr trace::Metric InvalidNameMetric(
   "rename_name_invalid", trace::Metric::Counter, "invalid_kind");
+
+  if (OldName == NewName)
+return makeError(ReasonToReject::SameName);
+
+  if (const auto *MD = dyn_cast(&RenameDecl)) {
+const auto Sel = MD->getSelector();
+if (Sel.getNumArgs() != NewName.count(':') &&
+NewName != "__clangd_rename_placeholder")
+  return makeError(InvalidName{InvalidName::BadIdentifier, NewName.str()});

ahoppen wrote:

This seems awfully ad-hoc to check for `__clangd_rename_placeholder`. I would 
prefer to change the prepare rename request to just change one of the selector 
pieces to `__clangd_rename_placeholder`.

I’ve been doing this here

https://github.com/llvm/llvm-project/pull/78872/files#diff-26ff7c74af8a1d882abbad43625d3cd4bf8d024ef5c7064993f5ede3eec8752eR817-R829

https://github.com/llvm/llvm-project/pull/76466
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)

[Alex Hoppen via cfe-commits]

@@ -538,11 +565,254 @@ std::optional checkName(const NamedDecl 
&RenameDecl,
 Conflict->getLocation().printToString(ASTCtx.getSourceManager())};
 }
   }
-  if (Result)
+  if (Result) {
 InvalidNameMetric.record(1, toString(Result->K));
+return makeError(*Result);
+  }
+  return std::nullopt;
+}
+
+bool isMatchingSelectorName(const syntax::Token &Cur, const syntax::Token 
&Next,
+const SourceManager &SM,
+llvm::StringRef SelectorName) {
+  if (SelectorName.empty())
+return Cur.kind() == tok::colon;
+  return Cur.kind() == tok::identifier && Next.kind() == tok::colon &&
+ Cur.text(SM) == SelectorName &&
+ // We require the selector name and : to be contiguous to avoid
+ // potential conflicts with ternary expression.
+ //
+ // e.g. support `foo:` but not `foo :`.
+ Cur.endLocation() == Next.location();
+}
+
+bool isSelectorLike(const syntax::Token &Cur, const syntax::Token &Next) {
+  return Cur.kind() == tok::identifier && Next.kind() == tok::colon &&
+ // We require the selector name and : to be contiguous.
+ // e.g. support `foo:` but not `foo :`.
+ Cur.endLocation() == Next.location();
+}
+
+bool parseMessageExpression(llvm::ArrayRef Tokens,
+const SourceManager &SM, unsigned Index,
+unsigned Last, Selector Sel,
+std::vector &SelectorPieces) {
+
+  unsigned NumArgs = Sel.getNumArgs();
+  llvm::SmallVector Closes;
+  SelectorPieces.clear();
+  while (Index < Last) {
+const auto &Tok = Tokens[Index];
+
+if (Closes.empty()) {
+  auto PieceCount = SelectorPieces.size();
+  if (PieceCount < NumArgs &&
+  isMatchingSelectorName(Tok, Tokens[Index + 1], SM,
+ Sel.getNameForSlot(PieceCount))) {
+// If 'foo:' instead of ':' (empty selector), we need to skip the ':'
+// token after the name.
+if (!Sel.getNameForSlot(PieceCount).empty()) {
+  ++Index;
+}
+SelectorPieces.push_back(
+halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
+continue;
+  }
+  // If we've found all pieces but the current token looks like another
+  // selector piece, it means the method being renamed is a strict prefix 
of
+  // the selector we've found - should be skipped.
+  if (SelectorPieces.size() >= NumArgs &&
+  isSelectorLike(Tok, Tokens[Index + 1]))
+return false;
+}
+
+switch (Tok.kind()) {
+case tok::l_square:
+  Closes.push_back(']');
+  break;
+case tok::l_paren:
+  Closes.push_back(')');
+  break;
+case tok::l_brace:
+  Closes.push_back('}');
+  break;
+case tok::r_square:
+  if (Closes.empty())
+return SelectorPieces.size() == NumArgs;
+
+  if (Closes.back() != ']')
+return false;
+  Closes.pop_back();
+  break;
+case tok::r_paren:
+  if (Closes.empty() || Closes.back() != ')')
+return false;
+  Closes.pop_back();
+  break;
+case tok::r_brace:
+  if (Closes.empty() || Closes.back() != '}')
+return false;
+  Closes.pop_back();
+  break;
+case tok::semi:
+  // top level ; ends all statements.
+  if (Closes.empty())
+return false;
+  break;
+default:
+  break;
+}
+
+++Index;
+  }
+  return false;
+}
+
+/// Collects all ranges of the given identifier/selector in the source code.
+///
+/// If a selector is given, this does a full lex of the given source code in
+/// order to identify all selector fragments (e.g. in method exprs/decls) since
+/// they are non-contiguous.
+std::vector collectRenameIdentifierRanges(
+llvm::StringRef Identifier, llvm::StringRef Content,
+const LangOptions &LangOpts, std::optional Selector) {
+  std::vector Ranges;
+  if (!Selector) {
+auto IdentifierRanges =
+collectIdentifierRanges(Identifier, Content, LangOpts);
+for (const auto &R : IdentifierRanges)
+  Ranges.emplace_back(R);
+return Ranges;
+  }
+  // FIXME: InMemoryFileAdapter crashes unless the buffer is null terminated!
+  std::string NullTerminatedCode = Content.str();
+  SourceManagerForFile FileSM("mock_file_name.cpp", NullTerminatedCode);
+  auto &SM = FileSM.get();
+
+  // We track parens and braces to ensure that we don't accidentally try 
parsing
+  // a method declaration or definition which isn't at the top level or similar
+  // looking expressions (e.g. an @selector() expression).
+  unsigned ParenCount = 0;
+  unsigned BraceCount = 0;
+  unsigned NumArgs = Selector->getNumArgs();
+
+  std::vector SelectorPieces;
+  auto Tokens = syntax::tokenize(SM.getMainFileID(), SM, LangOpts);

ahoppen wrote:

When calling `collectRenameIdentifierRanges` from `renameObjCMethodWithinFile`, 
we already have a `ParsedAST` and 

[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)

[Alex Hoppen via cfe-commits]

@@ -508,24 +513,46 @@ static bool mayBeValidIdentifier(llvm::StringRef Ident) {
   !isAsciiIdentifierStart(Ident.front(), AllowDollar))
 return false;
   for (char C : Ident) {
+if (AllowColon && C == ':')
+  continue;
 if (llvm::isASCII(C) && !isAsciiIdentifierContinue(C, AllowDollar))
   return false;
   }
   return true;
 }
 
+std::string getName(const NamedDecl &RenameDecl) {
+  if (const auto *MD = dyn_cast(&RenameDecl))
+return MD->getSelector().getAsString();
+  if (const auto *ID = RenameDecl.getIdentifier())
+return ID->getName().str();
+  return "";
+}
+
 // Check if we can rename the given RenameDecl into NewName.
 // Return details if the rename would produce a conflict.
-std::optional checkName(const NamedDecl &RenameDecl,
- llvm::StringRef NewName) {
+std::optional checkName(const NamedDecl &RenameDecl,
+ llvm::StringRef NewName,
+ llvm::StringRef OldName) {
   trace::Span Tracer("CheckName");
   static constexpr trace::Metric InvalidNameMetric(
   "rename_name_invalid", trace::Metric::Counter, "invalid_kind");
+
+  if (OldName == NewName)
+return makeError(ReasonToReject::SameName);
+
+  if (const auto *MD = dyn_cast(&RenameDecl)) {
+const auto Sel = MD->getSelector();
+if (Sel.getNumArgs() != NewName.count(':') &&

ahoppen wrote:

Instead of effectively doing a stripped-down selector parsing here, I would 
make `checkName` take a `SymbolName`.

https://github.com/llvm/llvm-project/pull/76466
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)

[Alex Hoppen via cfe-commits]

@@ -569,8 +840,13 @@ renameWithinFile(ParsedAST &AST, const NamedDecl 
&RenameDecl,
 //   }
 if (!isInsideMainFile(RenameLoc, SM))
   continue;
+Locs.push_back(RenameLoc);
+  }
+  if (const auto *MD = dyn_cast(&RenameDecl))
+return renameObjCMethodWithinFile(AST, MD, NewName, std::move(Locs));

ahoppen wrote:

If it is a zero-arg or one-arg Objective-C selector, I think we can still use 
the old, faster path that doesn’t need to scan the source file for Objective-C 
selectors using `renameObjCMethodWithinFile`.

https://github.com/llvm/llvm-project/pull/76466
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)

[Alex Hoppen via cfe-commits]

@@ -508,24 +513,46 @@ static bool mayBeValidIdentifier(llvm::StringRef Ident) {
   !isAsciiIdentifierStart(Ident.front(), AllowDollar))
 return false;
   for (char C : Ident) {
+if (AllowColon && C == ':')

ahoppen wrote:

Should we also allow spaces here if you spell the new method name as 
`doSomething: with:`?

--- 

Should we also check that the first selector piece is not empty? Ie. that `: 
with:` is not a valid selector.

https://github.com/llvm/llvm-project/pull/76466
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)

[Alex Hoppen via cfe-commits]

@@ -538,11 +565,254 @@ std::optional checkName(const NamedDecl 
&RenameDecl,
 Conflict->getLocation().printToString(ASTCtx.getSourceManager())};
 }
   }
-  if (Result)
+  if (Result) {
 InvalidNameMetric.record(1, toString(Result->K));
+return makeError(*Result);
+  }
+  return std::nullopt;
+}
+
+bool isMatchingSelectorName(const syntax::Token &Cur, const syntax::Token 
&Next,
+const SourceManager &SM,
+llvm::StringRef SelectorName) {
+  if (SelectorName.empty())
+return Cur.kind() == tok::colon;
+  return Cur.kind() == tok::identifier && Next.kind() == tok::colon &&
+ Cur.text(SM) == SelectorName &&
+ // We require the selector name and : to be contiguous to avoid
+ // potential conflicts with ternary expression.
+ //
+ // e.g. support `foo:` but not `foo :`.
+ Cur.endLocation() == Next.location();

ahoppen wrote:

Could this be simplified to 

```suggestion
  return isSelectorLike(Cur, Next) && Cur.text(SM) == SelectorName;
```

https://github.com/llvm/llvm-project/pull/76466
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)

[Alex Hoppen via cfe-commits]

@@ -538,11 +565,254 @@ std::optional checkName(const NamedDecl 
&RenameDecl,
 Conflict->getLocation().printToString(ASTCtx.getSourceManager())};
 }
   }
-  if (Result)
+  if (Result) {
 InvalidNameMetric.record(1, toString(Result->K));
+return makeError(*Result);
+  }
+  return std::nullopt;
+}
+
+bool isMatchingSelectorName(const syntax::Token &Cur, const syntax::Token 
&Next,
+const SourceManager &SM,
+llvm::StringRef SelectorName) {
+  if (SelectorName.empty())
+return Cur.kind() == tok::colon;
+  return Cur.kind() == tok::identifier && Next.kind() == tok::colon &&
+ Cur.text(SM) == SelectorName &&
+ // We require the selector name and : to be contiguous to avoid
+ // potential conflicts with ternary expression.
+ //
+ // e.g. support `foo:` but not `foo :`.
+ Cur.endLocation() == Next.location();
+}
+
+bool isSelectorLike(const syntax::Token &Cur, const syntax::Token &Next) {
+  return Cur.kind() == tok::identifier && Next.kind() == tok::colon &&
+ // We require the selector name and : to be contiguous.
+ // e.g. support `foo:` but not `foo :`.
+ Cur.endLocation() == Next.location();
+}
+
+bool parseMessageExpression(llvm::ArrayRef Tokens,
+const SourceManager &SM, unsigned Index,
+unsigned Last, Selector Sel,
+std::vector &SelectorPieces) {
+
+  unsigned NumArgs = Sel.getNumArgs();
+  llvm::SmallVector Closes;

ahoppen wrote:

Wouldn’t it be cleaner if `Closes` is a vector of token kinds? I think then you 
could also just check something like the following instead of repeating it in 
the `switch`.

```cpp
if (!Closes.empty() && Closes.back() == Tok.kind()) {
  Closes.pop_back();
  continue;
}

https://github.com/llvm/llvm-project/pull/76466
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)

[Alex Hoppen via cfe-commits]

@@ -53,13 +55,34 @@ struct RenameInputs {
 struct RenameResult {
   // The range of the symbol that the user can attempt to rename.
   Range Target;
+  // Placeholder text for the rename operation if non-empty.
+  std::string Placeholder;
   // Rename occurrences for the current main file.
   std::vector LocalChanges;
   // Complete edits for the rename, including LocalChanges.
   // If the full set of changes is unknown, this field is empty.
   FileEdits GlobalChanges;
 };
 
+/// Represents a symbol range where the symbol can potentially have multiple
+/// tokens.
+struct SymbolRange {
+  /// Ranges for the tokens that make up the symbol's name.
+  /// Usually a single range, but there can be multiple ranges if the tokens 
for
+  /// the symbol are split, e.g. ObjC selectors.
+  std::vector Ranges;

ahoppen wrote:

I think it would be good to clarify if 
- These ranges are only the selector piece names or also the colons
- If an empty selector piece is represented by an empty range in here

https://github.com/llvm/llvm-project/pull/76466
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)

[Alex Hoppen via cfe-commits]

@@ -538,11 +565,254 @@ std::optional checkName(const NamedDecl 
&RenameDecl,
 Conflict->getLocation().printToString(ASTCtx.getSourceManager())};
 }
   }
-  if (Result)
+  if (Result) {
 InvalidNameMetric.record(1, toString(Result->K));
+return makeError(*Result);
+  }
+  return std::nullopt;
+}
+
+bool isMatchingSelectorName(const syntax::Token &Cur, const syntax::Token 
&Next,
+const SourceManager &SM,
+llvm::StringRef SelectorName) {
+  if (SelectorName.empty())
+return Cur.kind() == tok::colon;
+  return Cur.kind() == tok::identifier && Next.kind() == tok::colon &&
+ Cur.text(SM) == SelectorName &&
+ // We require the selector name and : to be contiguous to avoid
+ // potential conflicts with ternary expression.
+ //
+ // e.g. support `foo:` but not `foo :`.
+ Cur.endLocation() == Next.location();
+}
+
+bool isSelectorLike(const syntax::Token &Cur, const syntax::Token &Next) {
+  return Cur.kind() == tok::identifier && Next.kind() == tok::colon &&
+ // We require the selector name and : to be contiguous.
+ // e.g. support `foo:` but not `foo :`.
+ Cur.endLocation() == Next.location();
+}
+
+bool parseMessageExpression(llvm::ArrayRef Tokens,
+const SourceManager &SM, unsigned Index,
+unsigned Last, Selector Sel,
+std::vector &SelectorPieces) {
+
+  unsigned NumArgs = Sel.getNumArgs();
+  llvm::SmallVector Closes;
+  SelectorPieces.clear();
+  while (Index < Last) {
+const auto &Tok = Tokens[Index];
+
+if (Closes.empty()) {
+  auto PieceCount = SelectorPieces.size();
+  if (PieceCount < NumArgs &&
+  isMatchingSelectorName(Tok, Tokens[Index + 1], SM,
+ Sel.getNameForSlot(PieceCount))) {
+// If 'foo:' instead of ':' (empty selector), we need to skip the ':'
+// token after the name.
+if (!Sel.getNameForSlot(PieceCount).empty()) {
+  ++Index;
+}
+SelectorPieces.push_back(
+halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
+continue;
+  }
+  // If we've found all pieces but the current token looks like another
+  // selector piece, it means the method being renamed is a strict prefix 
of
+  // the selector we've found - should be skipped.
+  if (SelectorPieces.size() >= NumArgs &&
+  isSelectorLike(Tok, Tokens[Index + 1]))
+return false;
+}
+
+switch (Tok.kind()) {
+case tok::l_square:
+  Closes.push_back(']');
+  break;
+case tok::l_paren:
+  Closes.push_back(')');
+  break;
+case tok::l_brace:
+  Closes.push_back('}');
+  break;
+case tok::r_square:
+  if (Closes.empty())
+return SelectorPieces.size() == NumArgs;
+
+  if (Closes.back() != ']')
+return false;
+  Closes.pop_back();
+  break;
+case tok::r_paren:
+  if (Closes.empty() || Closes.back() != ')')
+return false;
+  Closes.pop_back();
+  break;
+case tok::r_brace:
+  if (Closes.empty() || Closes.back() != '}')
+return false;
+  Closes.pop_back();
+  break;
+case tok::semi:
+  // top level ; ends all statements.
+  if (Closes.empty())
+return false;
+  break;
+default:
+  break;
+}
+
+++Index;
+  }
+  return false;
+}
+
+/// Collects all ranges of the given identifier/selector in the source code.
+///
+/// If a selector is given, this does a full lex of the given source code in
+/// order to identify all selector fragments (e.g. in method exprs/decls) since
+/// they are non-contiguous.
+std::vector collectRenameIdentifierRanges(
+llvm::StringRef Identifier, llvm::StringRef Content,
+const LangOptions &LangOpts, std::optional Selector) {
+  std::vector Ranges;
+  if (!Selector) {
+auto IdentifierRanges =
+collectIdentifierRanges(Identifier, Content, LangOpts);
+for (const auto &R : IdentifierRanges)
+  Ranges.emplace_back(R);
+return Ranges;
+  }
+  // FIXME: InMemoryFileAdapter crashes unless the buffer is null terminated!
+  std::string NullTerminatedCode = Content.str();
+  SourceManagerForFile FileSM("mock_file_name.cpp", NullTerminatedCode);
+  auto &SM = FileSM.get();
+
+  // We track parens and braces to ensure that we don't accidentally try 
parsing
+  // a method declaration or definition which isn't at the top level or similar
+  // looking expressions (e.g. an @selector() expression).
+  unsigned ParenCount = 0;
+  unsigned BraceCount = 0;
+  unsigned NumArgs = Selector->getNumArgs();
+
+  std::vector SelectorPieces;
+  auto Tokens = syntax::tokenize(SM.getMainFileID(), SM, LangOpts);
+  unsigned Last = Tokens.size() - 1;
+  for (unsigned Index = 0; Index < Last; ++Index) {
+const auto &Tok = Tokens[Index];
+

[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)

[Alex Hoppen via cfe-commits]

@@ -538,11 +565,254 @@ std::optional checkName(const NamedDecl 
&RenameDecl,
 Conflict->getLocation().printToString(ASTCtx.getSourceManager())};
 }
   }
-  if (Result)
+  if (Result) {
 InvalidNameMetric.record(1, toString(Result->K));
+return makeError(*Result);
+  }
+  return std::nullopt;
+}
+
+bool isMatchingSelectorName(const syntax::Token &Cur, const syntax::Token 
&Next,
+const SourceManager &SM,
+llvm::StringRef SelectorName) {
+  if (SelectorName.empty())
+return Cur.kind() == tok::colon;
+  return Cur.kind() == tok::identifier && Next.kind() == tok::colon &&
+ Cur.text(SM) == SelectorName &&
+ // We require the selector name and : to be contiguous to avoid
+ // potential conflicts with ternary expression.
+ //
+ // e.g. support `foo:` but not `foo :`.
+ Cur.endLocation() == Next.location();
+}
+
+bool isSelectorLike(const syntax::Token &Cur, const syntax::Token &Next) {
+  return Cur.kind() == tok::identifier && Next.kind() == tok::colon &&
+ // We require the selector name and : to be contiguous.
+ // e.g. support `foo:` but not `foo :`.
+ Cur.endLocation() == Next.location();
+}
+
+bool parseMessageExpression(llvm::ArrayRef Tokens,
+const SourceManager &SM, unsigned Index,
+unsigned Last, Selector Sel,
+std::vector &SelectorPieces) {
+
+  unsigned NumArgs = Sel.getNumArgs();
+  llvm::SmallVector Closes;
+  SelectorPieces.clear();
+  while (Index < Last) {
+const auto &Tok = Tokens[Index];
+
+if (Closes.empty()) {
+  auto PieceCount = SelectorPieces.size();
+  if (PieceCount < NumArgs &&
+  isMatchingSelectorName(Tok, Tokens[Index + 1], SM,
+ Sel.getNameForSlot(PieceCount))) {
+// If 'foo:' instead of ':' (empty selector), we need to skip the ':'
+// token after the name.
+if (!Sel.getNameForSlot(PieceCount).empty()) {
+  ++Index;
+}
+SelectorPieces.push_back(
+halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
+continue;
+  }
+  // If we've found all pieces but the current token looks like another
+  // selector piece, it means the method being renamed is a strict prefix 
of
+  // the selector we've found - should be skipped.
+  if (SelectorPieces.size() >= NumArgs &&
+  isSelectorLike(Tok, Tokens[Index + 1]))
+return false;
+}
+
+switch (Tok.kind()) {
+case tok::l_square:
+  Closes.push_back(']');
+  break;
+case tok::l_paren:
+  Closes.push_back(')');
+  break;
+case tok::l_brace:
+  Closes.push_back('}');
+  break;
+case tok::r_square:
+  if (Closes.empty())
+return SelectorPieces.size() == NumArgs;
+
+  if (Closes.back() != ']')
+return false;
+  Closes.pop_back();
+  break;
+case tok::r_paren:
+  if (Closes.empty() || Closes.back() != ')')
+return false;
+  Closes.pop_back();
+  break;
+case tok::r_brace:
+  if (Closes.empty() || Closes.back() != '}')
+return false;
+  Closes.pop_back();
+  break;
+case tok::semi:
+  // top level ; ends all statements.
+  if (Closes.empty())
+return false;
+  break;
+default:
+  break;
+}
+
+++Index;
+  }
+  return false;
+}
+
+/// Collects all ranges of the given identifier/selector in the source code.
+///
+/// If a selector is given, this does a full lex of the given source code in
+/// order to identify all selector fragments (e.g. in method exprs/decls) since
+/// they are non-contiguous.
+std::vector collectRenameIdentifierRanges(
+llvm::StringRef Identifier, llvm::StringRef Content,
+const LangOptions &LangOpts, std::optional Selector) {
+  std::vector Ranges;
+  if (!Selector) {
+auto IdentifierRanges =
+collectIdentifierRanges(Identifier, Content, LangOpts);
+for (const auto &R : IdentifierRanges)
+  Ranges.emplace_back(R);
+return Ranges;
+  }
+  // FIXME: InMemoryFileAdapter crashes unless the buffer is null terminated!
+  std::string NullTerminatedCode = Content.str();
+  SourceManagerForFile FileSM("mock_file_name.cpp", NullTerminatedCode);
+  auto &SM = FileSM.get();
+
+  // We track parens and braces to ensure that we don't accidentally try 
parsing
+  // a method declaration or definition which isn't at the top level or similar
+  // looking expressions (e.g. an @selector() expression).
+  unsigned ParenCount = 0;
+  unsigned BraceCount = 0;
+  unsigned NumArgs = Selector->getNumArgs();
+
+  std::vector SelectorPieces;
+  auto Tokens = syntax::tokenize(SM.getMainFileID(), SM, LangOpts);
+  unsigned Last = Tokens.size() - 1;
+  for (unsigned Index = 0; Index < Last; ++Index) {
+const auto &Tok = Tokens[Index];
+
+if (BraceCou

[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)

[Alex Hoppen via cfe-commits]

@@ -508,24 +513,46 @@ static bool mayBeValidIdentifier(llvm::StringRef Ident) {
   !isAsciiIdentifierStart(Ident.front(), AllowDollar))
 return false;
   for (char C : Ident) {
+if (AllowColon && C == ':')

ahoppen wrote:

Oh, I just assumed that the first selector argument had to be named. I never 
checked if it can be unnamed.

https://github.com/llvm/llvm-project/pull/76466
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)

[Alex Hoppen via cfe-commits]

@@ -538,11 +565,254 @@ std::optional checkName(const NamedDecl 
&RenameDecl,
 Conflict->getLocation().printToString(ASTCtx.getSourceManager())};
 }
   }
-  if (Result)
+  if (Result) {
 InvalidNameMetric.record(1, toString(Result->K));
+return makeError(*Result);
+  }
+  return std::nullopt;
+}
+
+bool isMatchingSelectorName(const syntax::Token &Cur, const syntax::Token 
&Next,
+const SourceManager &SM,
+llvm::StringRef SelectorName) {
+  if (SelectorName.empty())
+return Cur.kind() == tok::colon;
+  return Cur.kind() == tok::identifier && Next.kind() == tok::colon &&
+ Cur.text(SM) == SelectorName &&
+ // We require the selector name and : to be contiguous to avoid
+ // potential conflicts with ternary expression.
+ //
+ // e.g. support `foo:` but not `foo :`.
+ Cur.endLocation() == Next.location();
+}
+
+bool isSelectorLike(const syntax::Token &Cur, const syntax::Token &Next) {
+  return Cur.kind() == tok::identifier && Next.kind() == tok::colon &&
+ // We require the selector name and : to be contiguous.
+ // e.g. support `foo:` but not `foo :`.
+ Cur.endLocation() == Next.location();
+}
+
+bool parseMessageExpression(llvm::ArrayRef Tokens,
+const SourceManager &SM, unsigned Index,
+unsigned Last, Selector Sel,
+std::vector &SelectorPieces) {
+
+  unsigned NumArgs = Sel.getNumArgs();
+  llvm::SmallVector Closes;
+  SelectorPieces.clear();
+  while (Index < Last) {
+const auto &Tok = Tokens[Index];
+
+if (Closes.empty()) {
+  auto PieceCount = SelectorPieces.size();
+  if (PieceCount < NumArgs &&
+  isMatchingSelectorName(Tok, Tokens[Index + 1], SM,
+ Sel.getNameForSlot(PieceCount))) {
+// If 'foo:' instead of ':' (empty selector), we need to skip the ':'
+// token after the name.
+if (!Sel.getNameForSlot(PieceCount).empty()) {
+  ++Index;
+}
+SelectorPieces.push_back(
+halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
+continue;
+  }
+  // If we've found all pieces but the current token looks like another
+  // selector piece, it means the method being renamed is a strict prefix 
of
+  // the selector we've found - should be skipped.
+  if (SelectorPieces.size() >= NumArgs &&
+  isSelectorLike(Tok, Tokens[Index + 1]))
+return false;
+}
+
+switch (Tok.kind()) {
+case tok::l_square:
+  Closes.push_back(']');
+  break;
+case tok::l_paren:
+  Closes.push_back(')');
+  break;
+case tok::l_brace:
+  Closes.push_back('}');
+  break;
+case tok::r_square:
+  if (Closes.empty())
+return SelectorPieces.size() == NumArgs;
+
+  if (Closes.back() != ']')
+return false;
+  Closes.pop_back();
+  break;
+case tok::r_paren:
+  if (Closes.empty() || Closes.back() != ')')
+return false;
+  Closes.pop_back();
+  break;
+case tok::r_brace:
+  if (Closes.empty() || Closes.back() != '}')
+return false;
+  Closes.pop_back();
+  break;
+case tok::semi:
+  // top level ; ends all statements.
+  if (Closes.empty())
+return false;
+  break;
+default:
+  break;
+}
+
+++Index;
+  }
+  return false;
+}
+
+/// Collects all ranges of the given identifier/selector in the source code.
+///
+/// If a selector is given, this does a full lex of the given source code in
+/// order to identify all selector fragments (e.g. in method exprs/decls) since
+/// they are non-contiguous.
+std::vector collectRenameIdentifierRanges(
+llvm::StringRef Identifier, llvm::StringRef Content,
+const LangOptions &LangOpts, std::optional Selector) {
+  std::vector Ranges;
+  if (!Selector) {
+auto IdentifierRanges =
+collectIdentifierRanges(Identifier, Content, LangOpts);
+for (const auto &R : IdentifierRanges)
+  Ranges.emplace_back(R);
+return Ranges;
+  }
+  // FIXME: InMemoryFileAdapter crashes unless the buffer is null terminated!
+  std::string NullTerminatedCode = Content.str();
+  SourceManagerForFile FileSM("mock_file_name.cpp", NullTerminatedCode);
+  auto &SM = FileSM.get();
+
+  // We track parens and braces to ensure that we don't accidentally try 
parsing
+  // a method declaration or definition which isn't at the top level or similar
+  // looking expressions (e.g. an @selector() expression).
+  unsigned ParenCount = 0;
+  unsigned BraceCount = 0;
+  unsigned NumArgs = Selector->getNumArgs();
+
+  std::vector SelectorPieces;
+  auto Tokens = syntax::tokenize(SM.getMainFileID(), SM, LangOpts);

ahoppen wrote:

I don’t think it’s too bad to force that onto the caller. AFAICT we have two 
calls to `collectRenameIdentifierRang

[clang] [clang-tools-extra] [clangd] Add support to rename Objective-C selectors (PR #78872)

[Alex Hoppen via cfe-commits]

https://github.com/ahoppen created 
https://github.com/llvm/llvm-project/pull/78872

This adds support to rename Objective-C method declarations and calls to clangd.

>From 26d9d1a5a342df34cde921f7d9921e7b94dadd87 Mon Sep 17 00:00:00 2001
From: Alex Hoppen 
Date: Tue, 19 Dec 2023 15:54:40 -0800
Subject: [PATCH 1/5] [Refactoring] Add capabilities to `SymbolName` to
 represent Objective-C selector names

---
 .../Tooling/Refactoring/Rename/SymbolName.h   | 30 +++-
 clang/lib/Tooling/Refactoring/CMakeLists.txt  |  1 +
 .../Refactoring/Rename/RenamingAction.cpp |  4 +-
 .../Refactoring/Rename/USRLocFinder.cpp   |  4 +-
 clang/lib/Tooling/Refactoring/SymbolName.cpp  | 46 +++
 clang/tools/clang-rename/CMakeLists.txt   |  1 +
 6 files changed, 70 insertions(+), 16 deletions(-)
 create mode 100644 clang/lib/Tooling/Refactoring/SymbolName.cpp

diff --git a/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h 
b/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h
index 6c28d40f3679c2..077f315f657e77 100644
--- a/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h
+++ b/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h
@@ -15,6 +15,9 @@
 #include "llvm/ADT/StringRef.h"
 
 namespace clang {
+
+class LangOptions;
+
 namespace tooling {
 
 /// A name of a symbol.
@@ -27,19 +30,22 @@ namespace tooling {
 /// //   ^~ string 0 ~ ^~ string 1 ~
 /// \endcode
 class SymbolName {
+  llvm::SmallVector NamePieces;
+
 public:
-  explicit SymbolName(StringRef Name) {
-// While empty symbol names are valid (Objective-C selectors can have empty
-// name pieces), occurrences Objective-C selectors are created using an
-// array of strings instead of just one string.
-assert(!Name.empty() && "Invalid symbol name!");
-this->Name.push_back(Name.str());
-  }
-
-  ArrayRef getNamePieces() const { return Name; }
-
-private:
-  llvm::SmallVector Name;
+  /// Create a new \c SymbolName with the specified pieces.
+  explicit SymbolName(ArrayRef NamePieces);
+
+  /// Creates a \c SymbolName from the given string representation.
+  ///
+  /// For Objective-C symbol names, this splits a selector into multiple pieces
+  /// on `:`. For all other languages the name is used as the symbol name.
+  SymbolName(StringRef Name, bool IsObjectiveCSelector);
+  SymbolName(StringRef Name, const LangOptions &LangOpts);
+
+  ArrayRef getNamePieces() const { return NamePieces; }
+
+  void print(raw_ostream &OS) const;
 };
 
 } // end namespace tooling
diff --git a/clang/lib/Tooling/Refactoring/CMakeLists.txt 
b/clang/lib/Tooling/Refactoring/CMakeLists.txt
index d3077be8810aad..a4a80ce8344313 100644
--- a/clang/lib/Tooling/Refactoring/CMakeLists.txt
+++ b/clang/lib/Tooling/Refactoring/CMakeLists.txt
@@ -13,6 +13,7 @@ add_clang_library(clangToolingRefactoring
   Rename/USRFinder.cpp
   Rename/USRFindingAction.cpp
   Rename/USRLocFinder.cpp
+  SymbolName.cpp
 
   LINK_LIBS
   clangAST
diff --git a/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp 
b/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp
index 72598601d47d67..4965977d1f7aa4 100644
--- a/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp
+++ b/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp
@@ -82,7 +82,7 @@ 
RenameOccurrences::createSourceReplacements(RefactoringRuleContext &Context) {
   if (!Occurrences)
 return Occurrences.takeError();
   // FIXME: Verify that the new name is valid.
-  SymbolName Name(NewName);
+  SymbolName Name(NewName, /*IsObjectiveCSelector=*/false);
   return createRenameReplacements(
   *Occurrences, Context.getASTContext().getSourceManager(), Name);
 }
@@ -219,7 +219,7 @@ class RenamingASTConsumer : public ASTConsumer {
 }
 // FIXME: Support multi-piece names.
 // FIXME: better error handling (propagate error out).
-SymbolName NewNameRef(NewName);
+SymbolName NewNameRef(NewName, /*IsObjectiveCSelector=*/false);
 Expected> Change =
 createRenameReplacements(Occurrences, SourceMgr, NewNameRef);
 if (!Change) {
diff --git a/clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp 
b/clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
index c18f9290471fe4..43e48f24caa9ea 100644
--- a/clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
+++ b/clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
@@ -60,8 +60,8 @@ class USRLocFindingASTVisitor
const ASTContext &Context)
   : RecursiveSymbolVisitor(Context.getSourceManager(),
Context.getLangOpts()),
-USRSet(USRs.begin(), USRs.end()), PrevName(PrevName), Context(Context) 
{
-  }
+USRSet(USRs.begin(), USRs.end()),
+PrevName(PrevName, /*IsObjectiveCSelector=*/false), Context(Context) {}
 
   bool visitSymbolOccurrence(const NamedDecl *ND,
  ArrayRef NameRanges) {
diff --git a/clang/lib/Tooling/Refactoring/SymbolName.cpp 
b/clang/lib/Toolin

[clang-tools-extra] [clang] [clangd] Add support to rename Objective-C selectors (PR #78872)

[Alex Hoppen via cfe-commits]

https://github.com/ahoppen updated 
https://github.com/llvm/llvm-project/pull/78872

>From 26d9d1a5a342df34cde921f7d9921e7b94dadd87 Mon Sep 17 00:00:00 2001
From: Alex Hoppen 
Date: Tue, 19 Dec 2023 15:54:40 -0800
Subject: [PATCH 1/5] [Refactoring] Add capabilities to `SymbolName` to
 represent Objective-C selector names

---
 .../Tooling/Refactoring/Rename/SymbolName.h   | 30 +++-
 clang/lib/Tooling/Refactoring/CMakeLists.txt  |  1 +
 .../Refactoring/Rename/RenamingAction.cpp |  4 +-
 .../Refactoring/Rename/USRLocFinder.cpp   |  4 +-
 clang/lib/Tooling/Refactoring/SymbolName.cpp  | 46 +++
 clang/tools/clang-rename/CMakeLists.txt   |  1 +
 6 files changed, 70 insertions(+), 16 deletions(-)
 create mode 100644 clang/lib/Tooling/Refactoring/SymbolName.cpp

diff --git a/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h 
b/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h
index 6c28d40f3679c2..077f315f657e77 100644
--- a/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h
+++ b/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h
@@ -15,6 +15,9 @@
 #include "llvm/ADT/StringRef.h"
 
 namespace clang {
+
+class LangOptions;
+
 namespace tooling {
 
 /// A name of a symbol.
@@ -27,19 +30,22 @@ namespace tooling {
 /// //   ^~ string 0 ~ ^~ string 1 ~
 /// \endcode
 class SymbolName {
+  llvm::SmallVector NamePieces;
+
 public:
-  explicit SymbolName(StringRef Name) {
-// While empty symbol names are valid (Objective-C selectors can have empty
-// name pieces), occurrences Objective-C selectors are created using an
-// array of strings instead of just one string.
-assert(!Name.empty() && "Invalid symbol name!");
-this->Name.push_back(Name.str());
-  }
-
-  ArrayRef getNamePieces() const { return Name; }
-
-private:
-  llvm::SmallVector Name;
+  /// Create a new \c SymbolName with the specified pieces.
+  explicit SymbolName(ArrayRef NamePieces);
+
+  /// Creates a \c SymbolName from the given string representation.
+  ///
+  /// For Objective-C symbol names, this splits a selector into multiple pieces
+  /// on `:`. For all other languages the name is used as the symbol name.
+  SymbolName(StringRef Name, bool IsObjectiveCSelector);
+  SymbolName(StringRef Name, const LangOptions &LangOpts);
+
+  ArrayRef getNamePieces() const { return NamePieces; }
+
+  void print(raw_ostream &OS) const;
 };
 
 } // end namespace tooling
diff --git a/clang/lib/Tooling/Refactoring/CMakeLists.txt 
b/clang/lib/Tooling/Refactoring/CMakeLists.txt
index d3077be8810aad..a4a80ce8344313 100644
--- a/clang/lib/Tooling/Refactoring/CMakeLists.txt
+++ b/clang/lib/Tooling/Refactoring/CMakeLists.txt
@@ -13,6 +13,7 @@ add_clang_library(clangToolingRefactoring
   Rename/USRFinder.cpp
   Rename/USRFindingAction.cpp
   Rename/USRLocFinder.cpp
+  SymbolName.cpp
 
   LINK_LIBS
   clangAST
diff --git a/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp 
b/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp
index 72598601d47d67..4965977d1f7aa4 100644
--- a/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp
+++ b/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp
@@ -82,7 +82,7 @@ 
RenameOccurrences::createSourceReplacements(RefactoringRuleContext &Context) {
   if (!Occurrences)
 return Occurrences.takeError();
   // FIXME: Verify that the new name is valid.
-  SymbolName Name(NewName);
+  SymbolName Name(NewName, /*IsObjectiveCSelector=*/false);
   return createRenameReplacements(
   *Occurrences, Context.getASTContext().getSourceManager(), Name);
 }
@@ -219,7 +219,7 @@ class RenamingASTConsumer : public ASTConsumer {
 }
 // FIXME: Support multi-piece names.
 // FIXME: better error handling (propagate error out).
-SymbolName NewNameRef(NewName);
+SymbolName NewNameRef(NewName, /*IsObjectiveCSelector=*/false);
 Expected> Change =
 createRenameReplacements(Occurrences, SourceMgr, NewNameRef);
 if (!Change) {
diff --git a/clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp 
b/clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
index c18f9290471fe4..43e48f24caa9ea 100644
--- a/clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
+++ b/clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
@@ -60,8 +60,8 @@ class USRLocFindingASTVisitor
const ASTContext &Context)
   : RecursiveSymbolVisitor(Context.getSourceManager(),
Context.getLangOpts()),
-USRSet(USRs.begin(), USRs.end()), PrevName(PrevName), Context(Context) 
{
-  }
+USRSet(USRs.begin(), USRs.end()),
+PrevName(PrevName, /*IsObjectiveCSelector=*/false), Context(Context) {}
 
   bool visitSymbolOccurrence(const NamedDecl *ND,
  ArrayRef NameRanges) {
diff --git a/clang/lib/Tooling/Refactoring/SymbolName.cpp 
b/clang/lib/Tooling/Refactoring/SymbolName.cpp
new file mode 100644
index 00..7fd50b2c4d

[clang] [clang-tools-extra] [clangd] Use `SymbolName` to represent Objective-C selectors (PR #82061)

[Alex Hoppen via cfe-commits]

https://github.com/ahoppen created 
https://github.com/llvm/llvm-project/pull/82061

This is a cleaner design than using identifier and an optional `Selector`. It 
also allows rename of Objective-C method names if no declaration is at hand and 
thus no `Selector` instance can be formed. For example, when finding the ranges 
to rename based on an index that’s not clangd’s built-in index.

CC @DavidGoldman @bnbarham @kadircet

>From 7eb837b74f30e55423e1ace6fe29fdec6774f95b Mon Sep 17 00:00:00 2001
From: Alex Hoppen 
Date: Fri, 16 Feb 2024 14:50:25 -0800
Subject: [PATCH] [clangd] Use `SymbolName` to represent Objective-C selectors
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This is a cleaner design than using identifier and an optional `Selector`. It 
also allows rename of Objective-C method names if no declaration is at hand and 
thus no `Selector` instance can be formed. For example, when finding the ranges 
to rename based on an index that’s not clangd’s built-in index.
---
 clang-tools-extra/clangd/refactor/Rename.cpp  | 57 +++
 clang-tools-extra/clangd/refactor/Rename.h|  6 +-
 .../clangd/unittests/RenameTests.cpp  |  6 +-
 .../Tooling/Refactoring/Rename/SymbolName.h   | 50 ++---
 clang/lib/Tooling/Refactoring/CMakeLists.txt  |  2 +
 .../Refactoring/Rename/RenamingAction.cpp |  4 +-
 .../Refactoring/Rename/USRLocFinder.cpp   |  4 +-
 clang/lib/Tooling/Refactoring/SymbolName.cpp  | 70 +++
 8 files changed, 147 insertions(+), 52 deletions(-)
 create mode 100644 clang/lib/Tooling/Refactoring/SymbolName.cpp

diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp 
b/clang-tools-extra/clangd/refactor/Rename.cpp
index 650862c99bcd12..bd2fcbb7ab1008 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -40,6 +40,8 @@ namespace clang {
 namespace clangd {
 namespace {
 
+using tooling::SymbolName;
+
 std::optional filePath(const SymbolLocation &Loc,
 llvm::StringRef HintFilePath) {
   if (!Loc)
@@ -591,11 +593,11 @@ bool isMatchingSelectorName(const syntax::Token &Cur, 
const syntax::Token &Next,
 // The search will terminate upon seeing Terminator or a ; at the top level.
 std::optional
 findAllSelectorPieces(llvm::ArrayRef Tokens,
-  const SourceManager &SM, Selector Sel,
+  const SourceManager &SM, const SymbolName &Name,
   tok::TokenKind Terminator) {
   assert(!Tokens.empty());
 
-  unsigned NumArgs = Sel.getNumArgs();
+  unsigned NumArgs = Name.getNamePieces().size();
   llvm::SmallVector Closes;
   std::vector SelectorPieces;
   for (unsigned Index = 0, Last = Tokens.size(); Index < Last - 1; ++Index) {
@@ -605,12 +607,12 @@ findAllSelectorPieces(llvm::ArrayRef 
Tokens,
   auto PieceCount = SelectorPieces.size();
   if (PieceCount < NumArgs &&
   isMatchingSelectorName(Tok, Tokens[Index + 1], SM,
- Sel.getNameForSlot(PieceCount))) {
+ Name.getNamePieces()[PieceCount])) {
 // If 'foo:' instead of ':' (empty selector), we need to skip the ':'
 // token after the name. We don't currently properly support empty
 // selectors since we may lex them improperly due to ternary statements
 // as well as don't properly support storing their ranges for edits.
-if (!Sel.getNameForSlot(PieceCount).empty())
+if (!Name.getNamePieces()[PieceCount].empty())
   ++Index;
 SelectorPieces.push_back(
 halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
@@ -662,16 +664,17 @@ findAllSelectorPieces(llvm::ArrayRef 
Tokens,
 
 /// Collects all ranges of the given identifier/selector in the source code.
 ///
-/// If a selector is given, this does a full lex of the given source code in
-/// order to identify all selector fragments (e.g. in method exprs/decls) since
-/// they are non-contiguous.
-std::vector collectRenameIdentifierRanges(
-llvm::StringRef Identifier, llvm::StringRef Content,
-const LangOptions &LangOpts, std::optional Selector) {
+/// If `Name` is an Objective-C symbol name, this does a full lex of the given
+/// source code in order to identify all selector fragments (e.g. in method
+/// exprs/decls) since they are non-contiguous.
+std::vector
+collectRenameIdentifierRanges(const tooling::SymbolName &Name,
+  llvm::StringRef Content,
+  const LangOptions &LangOpts) {
   std::vector Ranges;
-  if (!Selector) {
+  if (auto SinglePiece = Name.getSinglePiece()) {
 auto IdentifierRanges =
-collectIdentifierRanges(Identifier, Content, LangOpts);
+collectIdentifierRanges(*SinglePiece, Content, LangOpts);
 for (const auto &R : IdentifierRanges)
   Ranges.emplace_back(R);
 return Ranges;
@@ -685,7 +688,7 @@ std::vector col

[clang] [clang-tools-extra] [clangd] Use `SymbolName` to represent Objective-C selectors (PR #82061)

[Alex Hoppen via cfe-commits]

https://github.com/ahoppen updated 
https://github.com/llvm/llvm-project/pull/82061

>From be2388c8552ea7a4466046c4d2c9b041a5bf78f2 Mon Sep 17 00:00:00 2001
From: Alex Hoppen 
Date: Fri, 16 Feb 2024 14:50:25 -0800
Subject: [PATCH] [clangd] Use `SymbolName` to represent Objective-C selectors
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This is a cleaner design than using identifier and an optional `Selector`. It 
also allows rename of Objective-C method names if no declaration is at hand and 
thus no `Selector` instance can be formed. For example, when finding the ranges 
to rename based on an index that’s not clangd’s built-in index.
---
 clang-tools-extra/clangd/refactor/Rename.cpp  | 57 +++
 clang-tools-extra/clangd/refactor/Rename.h|  6 +-
 .../clangd/unittests/RenameTests.cpp  |  6 +-
 .../Tooling/Refactoring/Rename/SymbolName.h   | 50 ++---
 clang/lib/Tooling/Refactoring/CMakeLists.txt  |  2 +
 .../Refactoring/Rename/RenamingAction.cpp |  4 +-
 .../Refactoring/Rename/USRLocFinder.cpp   |  4 +-
 clang/lib/Tooling/Refactoring/SymbolName.cpp  | 70 +++
 .../Tooling/RefactoringActionRulesTest.cpp|  6 +-
 9 files changed, 150 insertions(+), 55 deletions(-)
 create mode 100644 clang/lib/Tooling/Refactoring/SymbolName.cpp

diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp 
b/clang-tools-extra/clangd/refactor/Rename.cpp
index 650862c99bcd12..bd2fcbb7ab1008 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -40,6 +40,8 @@ namespace clang {
 namespace clangd {
 namespace {
 
+using tooling::SymbolName;
+
 std::optional filePath(const SymbolLocation &Loc,
 llvm::StringRef HintFilePath) {
   if (!Loc)
@@ -591,11 +593,11 @@ bool isMatchingSelectorName(const syntax::Token &Cur, 
const syntax::Token &Next,
 // The search will terminate upon seeing Terminator or a ; at the top level.
 std::optional
 findAllSelectorPieces(llvm::ArrayRef Tokens,
-  const SourceManager &SM, Selector Sel,
+  const SourceManager &SM, const SymbolName &Name,
   tok::TokenKind Terminator) {
   assert(!Tokens.empty());
 
-  unsigned NumArgs = Sel.getNumArgs();
+  unsigned NumArgs = Name.getNamePieces().size();
   llvm::SmallVector Closes;
   std::vector SelectorPieces;
   for (unsigned Index = 0, Last = Tokens.size(); Index < Last - 1; ++Index) {
@@ -605,12 +607,12 @@ findAllSelectorPieces(llvm::ArrayRef 
Tokens,
   auto PieceCount = SelectorPieces.size();
   if (PieceCount < NumArgs &&
   isMatchingSelectorName(Tok, Tokens[Index + 1], SM,
- Sel.getNameForSlot(PieceCount))) {
+ Name.getNamePieces()[PieceCount])) {
 // If 'foo:' instead of ':' (empty selector), we need to skip the ':'
 // token after the name. We don't currently properly support empty
 // selectors since we may lex them improperly due to ternary statements
 // as well as don't properly support storing their ranges for edits.
-if (!Sel.getNameForSlot(PieceCount).empty())
+if (!Name.getNamePieces()[PieceCount].empty())
   ++Index;
 SelectorPieces.push_back(
 halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
@@ -662,16 +664,17 @@ findAllSelectorPieces(llvm::ArrayRef 
Tokens,
 
 /// Collects all ranges of the given identifier/selector in the source code.
 ///
-/// If a selector is given, this does a full lex of the given source code in
-/// order to identify all selector fragments (e.g. in method exprs/decls) since
-/// they are non-contiguous.
-std::vector collectRenameIdentifierRanges(
-llvm::StringRef Identifier, llvm::StringRef Content,
-const LangOptions &LangOpts, std::optional Selector) {
+/// If `Name` is an Objective-C symbol name, this does a full lex of the given
+/// source code in order to identify all selector fragments (e.g. in method
+/// exprs/decls) since they are non-contiguous.
+std::vector
+collectRenameIdentifierRanges(const tooling::SymbolName &Name,
+  llvm::StringRef Content,
+  const LangOptions &LangOpts) {
   std::vector Ranges;
-  if (!Selector) {
+  if (auto SinglePiece = Name.getSinglePiece()) {
 auto IdentifierRanges =
-collectIdentifierRanges(Identifier, Content, LangOpts);
+collectIdentifierRanges(*SinglePiece, Content, LangOpts);
 for (const auto &R : IdentifierRanges)
   Ranges.emplace_back(R);
 return Ranges;
@@ -685,7 +688,7 @@ std::vector collectRenameIdentifierRanges(
   // parsing a method declaration or definition which isn't at the top level or
   // similar looking expressions (e.g. an @selector() expression).
   llvm::SmallVector Closes;
-  llvm::StringRef FirstSelPiece = Selector->getNameForSlot(0);
+  llvm::StringRef 

[clang-tools-extra] [clangd] Fix renaming single argument ObjC methods (PR #82396)

[Alex Hoppen via cfe-commits]

https://github.com/ahoppen approved this pull request.

Thanks for fixing!

https://github.com/llvm/llvm-project/pull/82396
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-tools-extra] [clangd] Use `SymbolName` to represent Objective-C selectors (PR #82061)

[Alex Hoppen via cfe-commits]

https://github.com/ahoppen updated 
https://github.com/llvm/llvm-project/pull/82061

>From 8b4d8134f581dd44595f347e2ca2465a069411a4 Mon Sep 17 00:00:00 2001
From: Alex Hoppen 
Date: Fri, 16 Feb 2024 14:50:25 -0800
Subject: [PATCH] [clangd] Use `SymbolName` to represent Objective-C selectors
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This is a cleaner design than using identifier and an optional `Selector`. It 
also allows rename of Objective-C method names if no declaration is at hand and 
thus no `Selector` instance can be formed. For example, when finding the ranges 
to rename based on an index that’s not clangd’s built-in index.
---
 clang-tools-extra/clangd/refactor/Rename.cpp  | 57 +++
 clang-tools-extra/clangd/refactor/Rename.h|  6 +-
 .../clangd/unittests/RenameTests.cpp  |  6 +-
 .../Tooling/Refactoring/Rename/SymbolName.h   | 50 ++---
 clang/lib/Tooling/Refactoring/CMakeLists.txt  |  2 +
 .../Refactoring/Rename/RenamingAction.cpp |  4 +-
 .../Refactoring/Rename/USRLocFinder.cpp   |  4 +-
 clang/lib/Tooling/Refactoring/SymbolName.cpp  | 70 +++
 .../Tooling/RefactoringActionRulesTest.cpp|  6 +-
 9 files changed, 150 insertions(+), 55 deletions(-)
 create mode 100644 clang/lib/Tooling/Refactoring/SymbolName.cpp

diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp 
b/clang-tools-extra/clangd/refactor/Rename.cpp
index 650862c99bcd12..bd2fcbb7ab1008 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -40,6 +40,8 @@ namespace clang {
 namespace clangd {
 namespace {
 
+using tooling::SymbolName;
+
 std::optional filePath(const SymbolLocation &Loc,
 llvm::StringRef HintFilePath) {
   if (!Loc)
@@ -591,11 +593,11 @@ bool isMatchingSelectorName(const syntax::Token &Cur, 
const syntax::Token &Next,
 // The search will terminate upon seeing Terminator or a ; at the top level.
 std::optional
 findAllSelectorPieces(llvm::ArrayRef Tokens,
-  const SourceManager &SM, Selector Sel,
+  const SourceManager &SM, const SymbolName &Name,
   tok::TokenKind Terminator) {
   assert(!Tokens.empty());
 
-  unsigned NumArgs = Sel.getNumArgs();
+  unsigned NumArgs = Name.getNamePieces().size();
   llvm::SmallVector Closes;
   std::vector SelectorPieces;
   for (unsigned Index = 0, Last = Tokens.size(); Index < Last - 1; ++Index) {
@@ -605,12 +607,12 @@ findAllSelectorPieces(llvm::ArrayRef 
Tokens,
   auto PieceCount = SelectorPieces.size();
   if (PieceCount < NumArgs &&
   isMatchingSelectorName(Tok, Tokens[Index + 1], SM,
- Sel.getNameForSlot(PieceCount))) {
+ Name.getNamePieces()[PieceCount])) {
 // If 'foo:' instead of ':' (empty selector), we need to skip the ':'
 // token after the name. We don't currently properly support empty
 // selectors since we may lex them improperly due to ternary statements
 // as well as don't properly support storing their ranges for edits.
-if (!Sel.getNameForSlot(PieceCount).empty())
+if (!Name.getNamePieces()[PieceCount].empty())
   ++Index;
 SelectorPieces.push_back(
 halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
@@ -662,16 +664,17 @@ findAllSelectorPieces(llvm::ArrayRef 
Tokens,
 
 /// Collects all ranges of the given identifier/selector in the source code.
 ///
-/// If a selector is given, this does a full lex of the given source code in
-/// order to identify all selector fragments (e.g. in method exprs/decls) since
-/// they are non-contiguous.
-std::vector collectRenameIdentifierRanges(
-llvm::StringRef Identifier, llvm::StringRef Content,
-const LangOptions &LangOpts, std::optional Selector) {
+/// If `Name` is an Objective-C symbol name, this does a full lex of the given
+/// source code in order to identify all selector fragments (e.g. in method
+/// exprs/decls) since they are non-contiguous.
+std::vector
+collectRenameIdentifierRanges(const tooling::SymbolName &Name,
+  llvm::StringRef Content,
+  const LangOptions &LangOpts) {
   std::vector Ranges;
-  if (!Selector) {
+  if (auto SinglePiece = Name.getSinglePiece()) {
 auto IdentifierRanges =
-collectIdentifierRanges(Identifier, Content, LangOpts);
+collectIdentifierRanges(*SinglePiece, Content, LangOpts);
 for (const auto &R : IdentifierRanges)
   Ranges.emplace_back(R);
 return Ranges;
@@ -685,7 +688,7 @@ std::vector collectRenameIdentifierRanges(
   // parsing a method declaration or definition which isn't at the top level or
   // similar looking expressions (e.g. an @selector() expression).
   llvm::SmallVector Closes;
-  llvm::StringRef FirstSelPiece = Selector->getNameForSlot(0);
+  llvm::StringRef 

[clang] [clang-tools-extra] [clangd] Use `SymbolName` to represent Objective-C selectors (PR #82061)

[Alex Hoppen via cfe-commits]

https://github.com/ahoppen updated 
https://github.com/llvm/llvm-project/pull/82061

>From a8f769d2376e01c789ebf10df95e18b8c23cb79f Mon Sep 17 00:00:00 2001
From: Alex Hoppen 
Date: Fri, 16 Feb 2024 14:50:25 -0800
Subject: [PATCH] [clangd] Use `SymbolName` to represent Objective-C selectors
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This is a cleaner design than using identifier and an optional `Selector`. It 
also allows rename of Objective-C method names if no declaration is at hand and 
thus no `Selector` instance can be formed. For example, when finding the ranges 
to rename based on an index that’s not clangd’s built-in index.
---
 clang-tools-extra/clangd/refactor/Rename.cpp  | 57 +++
 clang-tools-extra/clangd/refactor/Rename.h|  6 +-
 .../clangd/unittests/RenameTests.cpp  |  6 +-
 .../Tooling/Refactoring/Rename/SymbolName.h   | 50 ++---
 clang/lib/Tooling/Refactoring/CMakeLists.txt  |  2 +
 .../Refactoring/Rename/RenamingAction.cpp |  4 +-
 .../Refactoring/Rename/USRLocFinder.cpp   |  4 +-
 clang/lib/Tooling/Refactoring/SymbolName.cpp  | 70 +++
 .../Tooling/RefactoringActionRulesTest.cpp|  6 +-
 9 files changed, 150 insertions(+), 55 deletions(-)
 create mode 100644 clang/lib/Tooling/Refactoring/SymbolName.cpp

diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp 
b/clang-tools-extra/clangd/refactor/Rename.cpp
index 650862c99bcd12..bd2fcbb7ab1008 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -40,6 +40,8 @@ namespace clang {
 namespace clangd {
 namespace {
 
+using tooling::SymbolName;
+
 std::optional filePath(const SymbolLocation &Loc,
 llvm::StringRef HintFilePath) {
   if (!Loc)
@@ -591,11 +593,11 @@ bool isMatchingSelectorName(const syntax::Token &Cur, 
const syntax::Token &Next,
 // The search will terminate upon seeing Terminator or a ; at the top level.
 std::optional
 findAllSelectorPieces(llvm::ArrayRef Tokens,
-  const SourceManager &SM, Selector Sel,
+  const SourceManager &SM, const SymbolName &Name,
   tok::TokenKind Terminator) {
   assert(!Tokens.empty());
 
-  unsigned NumArgs = Sel.getNumArgs();
+  unsigned NumArgs = Name.getNamePieces().size();
   llvm::SmallVector Closes;
   std::vector SelectorPieces;
   for (unsigned Index = 0, Last = Tokens.size(); Index < Last - 1; ++Index) {
@@ -605,12 +607,12 @@ findAllSelectorPieces(llvm::ArrayRef 
Tokens,
   auto PieceCount = SelectorPieces.size();
   if (PieceCount < NumArgs &&
   isMatchingSelectorName(Tok, Tokens[Index + 1], SM,
- Sel.getNameForSlot(PieceCount))) {
+ Name.getNamePieces()[PieceCount])) {
 // If 'foo:' instead of ':' (empty selector), we need to skip the ':'
 // token after the name. We don't currently properly support empty
 // selectors since we may lex them improperly due to ternary statements
 // as well as don't properly support storing their ranges for edits.
-if (!Sel.getNameForSlot(PieceCount).empty())
+if (!Name.getNamePieces()[PieceCount].empty())
   ++Index;
 SelectorPieces.push_back(
 halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
@@ -662,16 +664,17 @@ findAllSelectorPieces(llvm::ArrayRef 
Tokens,
 
 /// Collects all ranges of the given identifier/selector in the source code.
 ///
-/// If a selector is given, this does a full lex of the given source code in
-/// order to identify all selector fragments (e.g. in method exprs/decls) since
-/// they are non-contiguous.
-std::vector collectRenameIdentifierRanges(
-llvm::StringRef Identifier, llvm::StringRef Content,
-const LangOptions &LangOpts, std::optional Selector) {
+/// If `Name` is an Objective-C symbol name, this does a full lex of the given
+/// source code in order to identify all selector fragments (e.g. in method
+/// exprs/decls) since they are non-contiguous.
+std::vector
+collectRenameIdentifierRanges(const tooling::SymbolName &Name,
+  llvm::StringRef Content,
+  const LangOptions &LangOpts) {
   std::vector Ranges;
-  if (!Selector) {
+  if (auto SinglePiece = Name.getSinglePiece()) {
 auto IdentifierRanges =
-collectIdentifierRanges(Identifier, Content, LangOpts);
+collectIdentifierRanges(*SinglePiece, Content, LangOpts);
 for (const auto &R : IdentifierRanges)
   Ranges.emplace_back(R);
 return Ranges;
@@ -685,7 +688,7 @@ std::vector collectRenameIdentifierRanges(
   // parsing a method declaration or definition which isn't at the top level or
   // similar looking expressions (e.g. an @selector() expression).
   llvm::SmallVector Closes;
-  llvm::StringRef FirstSelPiece = Selector->getNameForSlot(0);
+  llvm::StringRef 

[clang] [clang-tools-extra] [clangd] Use `SymbolName` to represent Objective-C selectors (PR #82061)

[Alex Hoppen via cfe-commits]

https://github.com/ahoppen updated 
https://github.com/llvm/llvm-project/pull/82061

>From fca2389759d73380312e284c05ddc1662209de2e Mon Sep 17 00:00:00 2001
From: Alex Hoppen 
Date: Fri, 16 Feb 2024 14:50:25 -0800
Subject: [PATCH] [clangd] Use `SymbolName` to represent Objective-C selectors
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This is a cleaner design than using identifier and an optional `Selector`. It 
also allows rename of Objective-C method names if no declaration is at hand and 
thus no `Selector` instance can be formed. For example, when finding the ranges 
to rename based on an index that’s not clangd’s built-in index.
---
 clang-tools-extra/clangd/refactor/Rename.cpp  | 57 +++
 clang-tools-extra/clangd/refactor/Rename.h|  6 +-
 .../clangd/unittests/RenameTests.cpp  |  6 +-
 .../Tooling/Refactoring/Rename/SymbolName.h   | 50 ++---
 clang/lib/Tooling/Refactoring/CMakeLists.txt  |  2 +
 .../Refactoring/Rename/RenamingAction.cpp |  4 +-
 .../Tooling/Refactoring/Rename/SymbolName.cpp | 70 +++
 .../Refactoring/Rename/USRLocFinder.cpp   |  4 +-
 .../Tooling/RefactoringActionRulesTest.cpp|  6 +-
 9 files changed, 150 insertions(+), 55 deletions(-)
 create mode 100644 clang/lib/Tooling/Refactoring/Rename/SymbolName.cpp

diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp 
b/clang-tools-extra/clangd/refactor/Rename.cpp
index 650862c99bcd12..bd2fcbb7ab1008 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -40,6 +40,8 @@ namespace clang {
 namespace clangd {
 namespace {
 
+using tooling::SymbolName;
+
 std::optional filePath(const SymbolLocation &Loc,
 llvm::StringRef HintFilePath) {
   if (!Loc)
@@ -591,11 +593,11 @@ bool isMatchingSelectorName(const syntax::Token &Cur, 
const syntax::Token &Next,
 // The search will terminate upon seeing Terminator or a ; at the top level.
 std::optional
 findAllSelectorPieces(llvm::ArrayRef Tokens,
-  const SourceManager &SM, Selector Sel,
+  const SourceManager &SM, const SymbolName &Name,
   tok::TokenKind Terminator) {
   assert(!Tokens.empty());
 
-  unsigned NumArgs = Sel.getNumArgs();
+  unsigned NumArgs = Name.getNamePieces().size();
   llvm::SmallVector Closes;
   std::vector SelectorPieces;
   for (unsigned Index = 0, Last = Tokens.size(); Index < Last - 1; ++Index) {
@@ -605,12 +607,12 @@ findAllSelectorPieces(llvm::ArrayRef 
Tokens,
   auto PieceCount = SelectorPieces.size();
   if (PieceCount < NumArgs &&
   isMatchingSelectorName(Tok, Tokens[Index + 1], SM,
- Sel.getNameForSlot(PieceCount))) {
+ Name.getNamePieces()[PieceCount])) {
 // If 'foo:' instead of ':' (empty selector), we need to skip the ':'
 // token after the name. We don't currently properly support empty
 // selectors since we may lex them improperly due to ternary statements
 // as well as don't properly support storing their ranges for edits.
-if (!Sel.getNameForSlot(PieceCount).empty())
+if (!Name.getNamePieces()[PieceCount].empty())
   ++Index;
 SelectorPieces.push_back(
 halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
@@ -662,16 +664,17 @@ findAllSelectorPieces(llvm::ArrayRef 
Tokens,
 
 /// Collects all ranges of the given identifier/selector in the source code.
 ///
-/// If a selector is given, this does a full lex of the given source code in
-/// order to identify all selector fragments (e.g. in method exprs/decls) since
-/// they are non-contiguous.
-std::vector collectRenameIdentifierRanges(
-llvm::StringRef Identifier, llvm::StringRef Content,
-const LangOptions &LangOpts, std::optional Selector) {
+/// If `Name` is an Objective-C symbol name, this does a full lex of the given
+/// source code in order to identify all selector fragments (e.g. in method
+/// exprs/decls) since they are non-contiguous.
+std::vector
+collectRenameIdentifierRanges(const tooling::SymbolName &Name,
+  llvm::StringRef Content,
+  const LangOptions &LangOpts) {
   std::vector Ranges;
-  if (!Selector) {
+  if (auto SinglePiece = Name.getSinglePiece()) {
 auto IdentifierRanges =
-collectIdentifierRanges(Identifier, Content, LangOpts);
+collectIdentifierRanges(*SinglePiece, Content, LangOpts);
 for (const auto &R : IdentifierRanges)
   Ranges.emplace_back(R);
 return Ranges;
@@ -685,7 +688,7 @@ std::vector collectRenameIdentifierRanges(
   // parsing a method declaration or definition which isn't at the top level or
   // similar looking expressions (e.g. an @selector() expression).
   llvm::SmallVector Closes;
-  llvm::StringRef FirstSelPiece = Selector->getNameForSlot(0);
+  llvm::Str

[clang] [clang-tools-extra] [clangd] Use `SymbolName` to represent Objective-C selectors (PR #82061)

[Alex Hoppen via cfe-commits]

@@ -13,6 +13,7 @@ add_clang_library(clangToolingRefactoring
   Rename/USRFinder.cpp
   Rename/USRFindingAction.cpp
   Rename/USRLocFinder.cpp
+  SymbolName.cpp

ahoppen wrote:

Goode suggestions 👍🏽 

https://github.com/llvm/llvm-project/pull/82061
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Use `SymbolName` to represent Objective-C selectors (PR #82061)

[Alex Hoppen via cfe-commits]

ahoppen wrote:

@sam-mccall If you could review my latest changes, that would be great.

https://github.com/llvm/llvm-project/pull/82061
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Use `SymbolName` to represent Objective-C selectors (PR #82061)

[Alex Hoppen via cfe-commits]

ahoppen wrote:

@DavidGoldman Would you mind reviewing my changes?

https://github.com/llvm/llvm-project/pull/82061
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Use `SymbolName` to represent Objective-C selectors (PR #82061)

[Alex Hoppen via cfe-commits]

https://github.com/ahoppen updated 
https://github.com/llvm/llvm-project/pull/82061

>From 2ae885429ae7658f014eacebd89e204512f3a1c2 Mon Sep 17 00:00:00 2001
From: Alex Hoppen 
Date: Mon, 29 Jul 2024 17:33:43 -0700
Subject: [PATCH] [clangd] Use `RenameSymbolName` to represent Objective-C
 selectors
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This is a cleaner design than using identifier and an optional `Selector`. It 
also allows rename of Objective-C method names if no declaration is at hand and 
thus no `Selector` instance can be formed. For example, when finding the ranges 
to rename based on an index that’s not clangd’s built-in index.
---
 clang-tools-extra/clangd/refactor/Rename.cpp  | 101 --
 clang-tools-extra/clangd/refactor/Rename.h|  48 -
 .../clangd/unittests/RenameTests.cpp  |   6 +-
 3 files changed, 118 insertions(+), 37 deletions(-)

diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp 
b/clang-tools-extra/clangd/refactor/Rename.cpp
index b7894b8918eed..26059167208aa 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -18,9 +18,11 @@
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTTypeTraits.h"
 #include "clang/AST/Decl.h"
+#include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/AST/DeclarationName.h"
 #include "clang/AST/ParentMapContext.h"
 #include "clang/AST/Stmt.h"
 #include "clang/Basic/CharInfo.h"
@@ -597,11 +599,12 @@ bool isMatchingSelectorName(const syntax::Token &Cur, 
const syntax::Token &Next,
 // The search will terminate upon seeing Terminator or a ; at the top level.
 std::optional
 findAllSelectorPieces(llvm::ArrayRef Tokens,
-  const SourceManager &SM, Selector Sel,
+  const SourceManager &SM, const RenameSymbolName &Name,
   tok::TokenKind Terminator) {
   assert(!Tokens.empty());
 
-  unsigned NumArgs = Sel.getNumArgs();
+  ArrayRef NamePieces = Name.getNamePieces();
+  unsigned NumArgs = NamePieces.size();
   llvm::SmallVector Closes;
   std::vector SelectorPieces;
   for (unsigned Index = 0, Last = Tokens.size(); Index < Last - 1; ++Index) {
@@ -611,12 +614,12 @@ findAllSelectorPieces(llvm::ArrayRef 
Tokens,
   auto PieceCount = SelectorPieces.size();
   if (PieceCount < NumArgs &&
   isMatchingSelectorName(Tok, Tokens[Index + 1], SM,
- Sel.getNameForSlot(PieceCount))) {
+ NamePieces[PieceCount])) {
 // If 'foo:' instead of ':' (empty selector), we need to skip the ':'
 // token after the name. We don't currently properly support empty
 // selectors since we may lex them improperly due to ternary statements
 // as well as don't properly support storing their ranges for edits.
-if (!Sel.getNameForSlot(PieceCount).empty())
+if (!NamePieces[PieceCount].empty())
   ++Index;
 SelectorPieces.push_back(
 halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
@@ -668,16 +671,17 @@ findAllSelectorPieces(llvm::ArrayRef 
Tokens,
 
 /// Collects all ranges of the given identifier/selector in the source code.
 ///
-/// If a selector is given, this does a full lex of the given source code in
-/// order to identify all selector fragments (e.g. in method exprs/decls) since
-/// they are non-contiguous.
-std::vector collectRenameIdentifierRanges(
-llvm::StringRef Identifier, llvm::StringRef Content,
-const LangOptions &LangOpts, std::optional Selector) {
+/// If `Name` is an Objective-C symbol name, this does a full lex of the given
+/// source code in order to identify all selector fragments (e.g. in method
+/// exprs/decls) since they are non-contiguous.
+std::vector
+collectRenameIdentifierRanges(const RenameSymbolName &Name,
+  llvm::StringRef Content,
+  const LangOptions &LangOpts) {
   std::vector Ranges;
-  if (!Selector) {
+  if (auto SinglePiece = Name.getSinglePiece()) {
 auto IdentifierRanges =
-collectIdentifierRanges(Identifier, Content, LangOpts);
+collectIdentifierRanges(*SinglePiece, Content, LangOpts);
 for (const auto &R : IdentifierRanges)
   Ranges.emplace_back(R);
 return Ranges;
@@ -691,7 +695,7 @@ std::vector collectRenameIdentifierRanges(
   // parsing a method declaration or definition which isn't at the top level or
   // similar looking expressions (e.g. an @selector() expression).
   llvm::SmallVector Closes;
-  llvm::StringRef FirstSelPiece = Selector->getNameForSlot(0);
+  llvm::StringRef FirstSelPiece = Name.getNamePieces()[0];
 
   auto Tokens = syntax::tokenize(SM.getMainFileID(), SM, LangOpts);
   unsigned Last = Tokens.size() - 1;
@@ -723,7 +727,7 @@ std::vector collectRenameIdentifierRanges(

[clang-tools-extra] [clangd] Use `SymbolName` to represent Objective-C selectors (PR #82061)

[Alex Hoppen via cfe-commits]

@@ -35,6 +35,49 @@ struct RenameOptions {
   bool RenameVirtual = true;
 };
 
+/// A name of a symbol that should be renamed.
+///
+/// Symbol's name can be composed of multiple strings. For example, Objective-C
+/// methods can contain multiple argument labels:
+///
+/// \code
+/// - (void) myMethodNamePiece: (int)x anotherNamePieces:(int)y;
+/// //   ^~ string 0 ~ ^~ string 1 ~

ahoppen wrote:

Updated. Could you merge the change when CI has passed?

https://github.com/llvm/llvm-project/pull/82061
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Use `SymbolName` to represent Objective-C selectors (PR #82061)

[Alex Hoppen via cfe-commits]

https://github.com/ahoppen updated 
https://github.com/llvm/llvm-project/pull/82061

>From 1285081e8fc7d7ecd162ed8ec7201437dbd708da Mon Sep 17 00:00:00 2001
From: Alex Hoppen 
Date: Mon, 29 Jul 2024 17:33:43 -0700
Subject: [PATCH] [clangd] Use `RenameSymbolName` to represent Objective-C
 selectors
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This is a cleaner design than using identifier and an optional `Selector`. It 
also allows rename of Objective-C method names if no declaration is at hand and 
thus no `Selector` instance can be formed. For example, when finding the ranges 
to rename based on an index that’s not clangd’s built-in index.
---
 clang-tools-extra/clangd/refactor/Rename.cpp  | 101 --
 clang-tools-extra/clangd/refactor/Rename.h|  48 -
 .../clangd/unittests/RenameTests.cpp  |   6 +-
 3 files changed, 118 insertions(+), 37 deletions(-)

diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp 
b/clang-tools-extra/clangd/refactor/Rename.cpp
index b7894b8918eed..26059167208aa 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -18,9 +18,11 @@
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTTypeTraits.h"
 #include "clang/AST/Decl.h"
+#include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/AST/DeclarationName.h"
 #include "clang/AST/ParentMapContext.h"
 #include "clang/AST/Stmt.h"
 #include "clang/Basic/CharInfo.h"
@@ -597,11 +599,12 @@ bool isMatchingSelectorName(const syntax::Token &Cur, 
const syntax::Token &Next,
 // The search will terminate upon seeing Terminator or a ; at the top level.
 std::optional
 findAllSelectorPieces(llvm::ArrayRef Tokens,
-  const SourceManager &SM, Selector Sel,
+  const SourceManager &SM, const RenameSymbolName &Name,
   tok::TokenKind Terminator) {
   assert(!Tokens.empty());
 
-  unsigned NumArgs = Sel.getNumArgs();
+  ArrayRef NamePieces = Name.getNamePieces();
+  unsigned NumArgs = NamePieces.size();
   llvm::SmallVector Closes;
   std::vector SelectorPieces;
   for (unsigned Index = 0, Last = Tokens.size(); Index < Last - 1; ++Index) {
@@ -611,12 +614,12 @@ findAllSelectorPieces(llvm::ArrayRef 
Tokens,
   auto PieceCount = SelectorPieces.size();
   if (PieceCount < NumArgs &&
   isMatchingSelectorName(Tok, Tokens[Index + 1], SM,
- Sel.getNameForSlot(PieceCount))) {
+ NamePieces[PieceCount])) {
 // If 'foo:' instead of ':' (empty selector), we need to skip the ':'
 // token after the name. We don't currently properly support empty
 // selectors since we may lex them improperly due to ternary statements
 // as well as don't properly support storing their ranges for edits.
-if (!Sel.getNameForSlot(PieceCount).empty())
+if (!NamePieces[PieceCount].empty())
   ++Index;
 SelectorPieces.push_back(
 halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
@@ -668,16 +671,17 @@ findAllSelectorPieces(llvm::ArrayRef 
Tokens,
 
 /// Collects all ranges of the given identifier/selector in the source code.
 ///
-/// If a selector is given, this does a full lex of the given source code in
-/// order to identify all selector fragments (e.g. in method exprs/decls) since
-/// they are non-contiguous.
-std::vector collectRenameIdentifierRanges(
-llvm::StringRef Identifier, llvm::StringRef Content,
-const LangOptions &LangOpts, std::optional Selector) {
+/// If `Name` is an Objective-C symbol name, this does a full lex of the given
+/// source code in order to identify all selector fragments (e.g. in method
+/// exprs/decls) since they are non-contiguous.
+std::vector
+collectRenameIdentifierRanges(const RenameSymbolName &Name,
+  llvm::StringRef Content,
+  const LangOptions &LangOpts) {
   std::vector Ranges;
-  if (!Selector) {
+  if (auto SinglePiece = Name.getSinglePiece()) {
 auto IdentifierRanges =
-collectIdentifierRanges(Identifier, Content, LangOpts);
+collectIdentifierRanges(*SinglePiece, Content, LangOpts);
 for (const auto &R : IdentifierRanges)
   Ranges.emplace_back(R);
 return Ranges;
@@ -691,7 +695,7 @@ std::vector collectRenameIdentifierRanges(
   // parsing a method declaration or definition which isn't at the top level or
   // similar looking expressions (e.g. an @selector() expression).
   llvm::SmallVector Closes;
-  llvm::StringRef FirstSelPiece = Selector->getNameForSlot(0);
+  llvm::StringRef FirstSelPiece = Name.getNamePieces()[0];
 
   auto Tokens = syntax::tokenize(SM.getMainFileID(), SM, LangOpts);
   unsigned Last = Tokens.size() - 1;
@@ -723,7 +727,7 @@ std::vector collectRenameIdentifierRanges(

[clang-tools-extra] [clangd] Use `SymbolName` to represent Objective-C selectors (PR #82061)

[Alex Hoppen via cfe-commits]

ahoppen wrote:

Thanks for the review, @DavidGoldman. I addressed your comments.

https://github.com/llvm/llvm-project/pull/82061
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits