[clangd] fix extract-to-function for overloaded operators #81640

5chmidti · 2024-02-13T18:02:01Z

When selecting code that contains the use of overloaded operators,
the SelectionTree will attribute the operator to the operator
declaration, not to the CXXOperatorCallExpr. To allow
extract-to-function to work with these operators, make unselected
CXXOperatorCallExprs valid root statements, just like DeclStmts.

Partially fixes clangd/clangd#1254

llvmbot · 2024-02-13T18:02:31Z

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clangd

Author: Julian Schmidt (5chmidti)

Changes

When selecting code that contains the use of overloaded operators,
the SelectionTree will attribute the operator to the operator
declaration, not to the CXXOperatorCallExpr. To allow
extract-to-function to work with these operators, make unselected
CXXOperatorCallExprs valid root statements, just like DeclStmts.

Fixes clangd/clangd#1254

Full diff: https://github.com/llvm/llvm-project/pull/81640.diff

3 Files Affected:

(modified) clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp (+9-6)
(modified) clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp (+47)
(modified) clang-tools-extra/docs/ReleaseNotes.rst (+3)

diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
index 0302839c58252e..aae480175b33f6 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -56,6 +56,7 @@
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
+#include "clang/AST/ExprCXX.h"
 #include "clang/AST/NestedNameSpecifier.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/Stmt.h"
@@ -70,7 +71,6 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Error.h"
-#include "llvm/Support/raw_os_ostream.h"
 #include <optional>
 
 namespace clang {
@@ -104,9 +104,12 @@ bool isRootStmt(const Node *N) {
   // Root statement cannot be partially selected.
   if (N->Selected == SelectionTree::Partial)
     return false;
-  // Only DeclStmt can be an unselected RootStmt since VarDecls claim the entire
-  // selection range in selectionTree.
-  if (N->Selected == SelectionTree::Unselected && !N->ASTNode.get<DeclStmt>())
+  // A DeclStmt can be an unselected RootStmt since VarDecls claim the entire
+  // selection range in selectionTree. Additionally, an CXXOperatorCallExpr of a
+  // binary operation can be unselected because it's children claim the entire
+  // selection range in the selection tree (e.g. <<).
+  if (N->Selected == SelectionTree::Unselected && !N->ASTNode.get<DeclStmt>() &&
+      !N->ASTNode.get<CXXOperatorCallExpr>())
     return false;
   return true;
 }
@@ -913,8 +916,8 @@ Expected<Tweak::Effect> ExtractFunction::apply(const Selection &Inputs) {
 
       tooling::Replacements OtherEdit(
           createForwardDeclaration(*ExtractedFunc, SM));
-      if (auto PathAndEdit = Tweak::Effect::fileEdit(SM, SM.getFileID(*FwdLoc),
-                                                 OtherEdit))
+      if (auto PathAndEdit =
+              Tweak::Effect::fileEdit(SM, SM.getFileID(*FwdLoc), OtherEdit))
         MultiFileEffect->ApplyEdits.try_emplace(PathAndEdit->first,
                                                 PathAndEdit->second);
       else
diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
index dec63d454d52c6..8e347b516c6ffe 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
@@ -571,6 +571,53 @@ int getNum(bool Superstitious, int Min, int Max) {
   EXPECT_EQ(apply(Before), After);
 }
 
+TEST_F(ExtractFunctionTest, OverloadedOperators) {
+  Context = File;
+  std::string Before = R"cpp(struct A {
+                int operator+(int x) { return x; }
+              };
+              A &operator<<(A &, int);
+              A &operator|(A &, int);
+
+              A stream{};
+
+              void foo(int, int);
+
+              int main() {
+                [[foo(1, 2);
+                foo(3, 4);
+                stream << 42;
+                stream + 42;
+                stream | 42;
+                foo(1, 2);
+                foo(3, 4);]]
+              })cpp";
+  std::string After =
+      R"cpp(struct A {
+                int operator+(int x) { return x; }
+              };
+              A &operator<<(A &, int);
+              A &operator|(A &, int);
+
+              A stream{};
+
+              void foo(int, int);
+
+              void extracted() {
+foo(1, 2);
+                foo(3, 4);
+                stream << 42;
+                stream + 42;
+                stream | 42;
+                foo(1, 2);
+                foo(3, 4);
+}
+int main() {
+                extracted();
+              })cpp";
+  EXPECT_EQ(apply(Before), After);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index f2fba9aa1450d6..01cd2278c1fb18 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -69,6 +69,9 @@ Code completion
 Code actions
 ^^^^^^^^^^^^
 
+- Improved the extract-to-function code action to allow extracting statements
+  with overloaded operators like ``<<`` of ``std::ostream``.
+
 Signature help
 ^^^^^^^^^^^^^^

5chmidti · 2024-02-24T13:39:24Z

Ping

5chmidti · 2024-03-24T13:09:54Z

Rebase & Ping

github-actions · 2024-03-24T13:12:22Z

✅ With the latest revision this PR passed the C/C++ code formatter.

github-actions · 2024-03-24T13:12:22Z

✅ With the latest revision this PR passed the Python code formatter.

BenBlumer · 2024-07-17T21:19:35Z

Would be great to see this merged. This bug prevents basic function extractions from working.

BenBlumer · 2024-07-17T23:20:57Z

I can confirm this builds and fixes the issue. For example, in:

#include <iostream>

#include <ostream>




int main() {

int a = 5;

int b = 7;

int c;

c =  a+b;

std::cout <<  a+b << std::endl;

return 0;

}

Without this patch, there is no refactor available for:

c =  a+b;

std::cout <<  a+b << std::endl

With the patch, it can be extracted

clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp

ckandeler · 2024-10-17T10:25:26Z

Needs rebase and another typo fix. Looks good to me otherwise.

BenBlumer · 2024-10-25T22:47:14Z

@5chmidti I'm excited for this to be mainlined. Give me a thumbs up if you want me to fix the typo and rebase for you -- happy to do it this weekend.

5chmidti

CC @HighCommander4 @kadircet

Technically, Sam told me last EuroLLVM that he had some refactor of the SelectionTree that would resolve issues like this in the general case, but as it is unclear that those changes will get in someday/in the 'near' future, I think that this PR makes sense to merge. Even though this is kind of a band-aid fix. Thoughts?

clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp

BenBlumer · 2024-10-26T18:54:08Z

I'm a bit of a outsider to the LLVM project, but I'd say a fix to a broken, already shipped, feature now is better than a general fix down the line.

…

On Sat, Oct 26, 2024, 3:23 AM Julian Schmidt ***@***.***> wrote: ***@***.**** commented on this pull request. CC @HighCommander4 <https://github.com/HighCommander4> @kadircet <https://github.com/kadircet> Technically, Sam told me last EuroLLVM that he had some refactor of the SelectionTree that would resolve issues like this in the general case, but as it is unclear that those changes will get in someday/in the 'near' future, I think that this PR makes sense to merge. Even though this is kind of a band-aid fix. Thoughts? ------------------------------ In clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp <#81640 (comment)>: > + // selection range in selectionTree. Additionally, a CXXOperatorCallExpr of a + // binary operation can be unselected because it's children claim the entire ⬇️ Suggested change - // selection range in selectionTree. Additionally, a CXXOperatorCallExpr of a - // binary operation can be unselected because it's children claim the entire + // selection range in selectionTree. Additionally, a CXXOperatorCallExpr of a + // binary operation can be unselected because its children claim the entire — Reply to this email directly, view it on GitHub <#81640 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA3J4MJEX4VEB24H7HVXXWTZ5NURZAVCNFSM6AAAAABDG6LIWCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGOJXGE2DCNBXGQ> . You are receiving this because you commented.Message ID: ***@***.***>

HighCommander4 · 2024-10-28T07:18:34Z

Do I understand correctly that this is a partial fix for clangd/clangd#1254 in that it addresses the issue with overloaded operators in particular, but it still leaves in place the limitation that a single expression statement (of any kind) cannot be extracted?

If so, would you mind adding an "unavailable" test case for the latter case, with some sort of "TODO, we'd like to relax this limitation" comment?

5chmidti · 2024-10-31T09:37:19Z

Do I understand correctly that this is a partial fix for clangd/clangd#1254 in that it addresses the issue with overloaded operators in particular, but it still leaves in place the limitation that a single expression statement (of any kind) cannot be extracted?

Exactly

If so, would you mind adding an "unavailable" test case for the latter case, with some sort of "TODO, we'd like to relax this limitation" comment?

Done

BenBlumer · 2024-11-01T16:15:18Z

Thanks for your work on this, Julian! I'm excited to have this tool work properly!

HighCommander4 · 2024-11-04T07:56:43Z

clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp

+  // selection range in selectionTree. Additionally, a CXXOperatorCallExpr of a
+  // binary operation can be unselected because it's children claim the entire
+  // selection range in the selection tree (e.g. <<).
+  if (N->Selected == SelectionTree::Unselected && !N->ASTNode.get<DeclStmt>() &&


Do we need to make a corresponding change here? The comment suggests that it's special-casing DeclStmt for the same reason as this code.

I don't think that would have any effect, because to hit this case, the common ancestor of our root statements needs to be completely selected, but the CXXOperatorCallExpr will be unselected as shown in this if here.

I guess the difference is that a DeclStmt can have a single VarDecl child which could be completely selected, leaving the DeclStmt unselected. There is no analogous situation for a CXXOperatorCallExpr which has multiple children.

HighCommander4 · 2024-11-04T08:00:08Z

clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp

+  // FIXME: Expressions are currently not extracted
+  EXPECT_EQ(apply(R"cpp(
+                void sink(int);
+                void call() { sink([[1+1]]); }


I was thinking more specifically about entire expression-statements, e.g. [[stream << 42;]].

Extraction of arbitrary subexpressions would be nice as well, but likely more involved; there is a work in progress patch here.

(We can keep this test case too.)

I've added another testcase for extracting a single expression

When selecting code that contains the use of overloaded operators, the SelectionTree will attribute the operator to the operator declaration, not to the `CXXOperatorCallExpr`. To allow extract-to-function to work with these operators, make unselected `CXXOperatorCallExpr`s valid root statements, just like `DeclStmt`s. Partially fixes clangd/clangd#1254

HighCommander4

Thanks. +1 to taking this targeted fix without waiting for a larger refactor.

HighCommander4 · 2024-11-15T05:44:50Z

clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp

+  // selection range in selectionTree. Additionally, a CXXOperatorCallExpr of a
+  // binary operation can be unselected because it's children claim the entire
+  // selection range in the selection tree (e.g. <<).
+  if (N->Selected == SelectionTree::Unselected && !N->ASTNode.get<DeclStmt>() &&


I guess the difference is that a DeclStmt can have a single VarDecl child which could be completely selected, leaving the DeclStmt unselected. There is no analogous situation for a CXXOperatorCallExpr which has multiple children.

llvmbot added clang-tools-extra clangd labels Feb 13, 2024

5chmidti force-pushed the clangd-extract-function-operators branch from 6efc30e to 6b62ed1 Compare March 24, 2024 13:09

ckandeler reviewed Oct 17, 2024

View reviewed changes

clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp Outdated Show resolved Hide resolved

5chmidti commented Oct 26, 2024

View reviewed changes

clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp Outdated Show resolved Hide resolved

5chmidti force-pushed the clangd-extract-function-operators branch from 9d67aa7 to a8f90e7 Compare October 31, 2024 09:37

HighCommander4 reviewed Nov 4, 2024

View reviewed changes

5chmidti added 4 commits November 10, 2024 18:55

fix typo

0fda432

add test for single-expr-only

6df0c2a

add test for single expression extraction being not available

38003e3

5chmidti force-pushed the clangd-extract-function-operators branch from a8f90e7 to 38003e3 Compare November 10, 2024 18:05

HighCommander4 added 2 commits November 15, 2024 00:42

fix typos

b7dc747

make fixme comment more precise

84f6ec5

HighCommander4 approved these changes Nov 15, 2024

View reviewed changes

HighCommander4 merged commit 9ae21b0 into llvm:main Nov 15, 2024
9 checks passed

5chmidti deleted the clangd-extract-function-operators branch November 26, 2024 19:12

[clangd] fix extract-to-function for overloaded operators #81640

[clangd] fix extract-to-function for overloaded operators #81640

Uh oh!

Conversation

5chmidti commented Feb 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Feb 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

5chmidti commented Feb 24, 2024

Uh oh!

5chmidti commented Mar 24, 2024

Uh oh!

github-actions bot commented Mar 24, 2024

Uh oh!

github-actions bot commented Mar 24, 2024

Uh oh!

BenBlumer commented Jul 17, 2024

Uh oh!

BenBlumer commented Jul 17, 2024

Uh oh!

Uh oh!

ckandeler commented Oct 17, 2024

Uh oh!

BenBlumer commented Oct 25, 2024

Uh oh!

5chmidti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BenBlumer commented Oct 26, 2024 via email

Uh oh!

HighCommander4 commented Oct 28, 2024

Uh oh!

5chmidti commented Oct 31, 2024

Uh oh!

BenBlumer commented Nov 1, 2024

Uh oh!

HighCommander4 Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

5chmidti Nov 10, 2024

Choose a reason for hiding this comment

Uh oh!

HighCommander4 Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

HighCommander4 Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

5chmidti Nov 10, 2024

Choose a reason for hiding this comment

Uh oh!

HighCommander4 left a comment

Choose a reason for hiding this comment

Uh oh!

HighCommander4 Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

5chmidti commented Feb 13, 2024 •

edited

Loading

llvmbot commented Feb 13, 2024 •

edited

Loading

HighCommander4 Nov 4, 2024 •

edited

Loading