Skip to content

[clang-tidy] fix false positive in bugprone-return-const-ref-from-parameter #117734

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 30, 2024

Conversation

jcsxky
Copy link
Contributor

@jcsxky jcsxky commented Nov 26, 2024

Fix #115743

@llvmbot
Copy link
Member

llvmbot commented Nov 26, 2024

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

@llvm/pr-subscribers-clang-tidy

Author: Qizhi Hu (jcsxky)

Changes

Fix #115743


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

3 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/bugprone/ReturnConstRefFromParameterCheck.cpp (+2-2)
  • (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4)
  • (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/return-const-ref-from-parameter.cpp (+6)
diff --git a/clang-tools-extra/clang-tidy/bugprone/ReturnConstRefFromParameterCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ReturnConstRefFromParameterCheck.cpp
index adb26ade955c5e..874f35f055094e 100644
--- a/clang-tools-extra/clang-tidy/bugprone/ReturnConstRefFromParameterCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/ReturnConstRefFromParameterCheck.cpp
@@ -21,10 +21,10 @@ void ReturnConstRefFromParameterCheck::registerMatchers(MatchFinder *Finder) {
               to(parmVarDecl(hasType(hasCanonicalType(
                                  qualType(lValueReferenceType(pointee(
                                               qualType(isConstQualified()))))
-                                     .bind("type"))))
+                                     .bind("type"))), parmVarDecl(hasDeclContext(functionDecl().bind("owner"))))
                      .bind("param")))),
           hasAncestor(
-              functionDecl(hasReturnTypeLoc(loc(qualType(
+              functionDecl(equalsBoundNode("owner"), hasReturnTypeLoc(loc(qualType(
                                hasCanonicalType(equalsBoundNode("type"))))))
                   .bind("func")))
           .bind("ret"),
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index b72d109b3d3938..9c3a5e4f40f930 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -104,6 +104,10 @@ New check aliases
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
+- Improved :doc:`bugprone-return-const-ref-from-parameter
+  <clang-tidy/checks/bugprone/return-const-ref-from-parameter>` check by no longer
+  giving false positives for lambda.
+
 - Improved :doc:`readability-redundant-smartptr-get
   <clang-tidy/checks/readability/redundant-smartptr-get>` check to
   remove `->`, when reduntant `get()` is removed.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/return-const-ref-from-parameter.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/return-const-ref-from-parameter.cpp
index d13c127da7c2a1..391e8f96401ab4 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/return-const-ref-from-parameter.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/return-const-ref-from-parameter.cpp
@@ -142,6 +142,12 @@ void instantiate(const int &param, const float &paramf, int &mut_param, float &m
         itf6(mut_paramf);
 }
 
+template<class T>
+void f(const T& t) {
+    const auto get = [&t] -> const T& { return t; };
+    return T{};
+}
+
 } // namespace valid
 
 namespace overload {

@EugeneZelenko
Copy link
Contributor

Please rebase from current main.

@jcsxky jcsxky force-pushed the check-param-decl-context branch from 37a518b to 80e12b2 Compare November 26, 2024 16:59
Copy link

github-actions bot commented Nov 26, 2024

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

@jcsxky jcsxky force-pushed the check-param-decl-context branch 2 times, most recently from 7dd899e to 5d35398 Compare November 26, 2024 17:06
void f(const T& t) {
const auto get = [&t] -> const T& { return t; };
return T{};
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add test:

const auto get = [](const T& t2) -> const T& { return t2; };

and verify that it's being still detected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

qualType(lValueReferenceType(
pointee(qualType(isConstQualified()))))
.bind("type"))),
parmVarDecl(hasDeclContext(functionDecl().bind("owner"))))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for duplicating parmVarDecl

Copy link
Member

@PiotrZSL PiotrZSL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add positive test for lambdas.

@jcsxky jcsxky force-pushed the check-param-decl-context branch from 5d35398 to 7100846 Compare November 28, 2024 14:39
@jcsxky
Copy link
Contributor Author

jcsxky commented Nov 28, 2024

Please add positive test for lambdas.

Positive test cases have been added.

@jcsxky jcsxky force-pushed the check-param-decl-context branch from 7100846 to 0a7192d Compare November 28, 2024 14:43
@@ -179,7 +179,8 @@ Changes in existing checks
- Improved :doc:`bugprone-return-const-ref-from-parameter
<clang-tidy/checks/bugprone/return-const-ref-from-parameter>` check to
diagnose potential dangling references when returning a ``const &`` parameter
by using the conditional operator ``cond ? var1 : var2``.
by using the conditional operator ``cond ? var1 : var2`` and no longer giving
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure it is a fix of false positives. Maybe in generic, it more looks like add support of lambda function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function in this issue contains lambda and thus gives a false positive(because it should not report diagnose since the return statement is not belongs to current function). I have modified the release note and make it easier to be understood.

@jcsxky jcsxky force-pushed the check-param-decl-context branch from 0a7192d to 528bc58 Compare November 29, 2024 14:32
@jcsxky jcsxky merged commit 04ab599 into llvm:main Nov 30, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[clang-tidy] False-positive with returning a constant reference parameter.
5 participants