[PATCH] D119165: [clang-tidy] Add processing lambda captures at bugprone-use-after-move check

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL abandoned this revision.
PiotrZSL added a comment.

Already fixed by D126780 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119165

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

[PATCH] D119165: [clang-tidy] Add processing lambda captures at bugprone-use-after-move check

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood requested changes to this revision.
LegalizeAdulthood added a comment.
This revision now requires changes to proceed.

Clang-Tidy tests and docs have moved to module subdirectories.

Please rebase this onto `main:HEAD` and:

- fold your changes into the appropriate subdirs, stripping the module prefix 
from new files.
- make the target `check-clang-extra` to validate your tests
- make the target `docs-clang-tools-html` to validate any docs changes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119165

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

[PATCH] D119165: [clang-tidy] Add processing lambda captures at bugprone-use-after-move check

[Martin Böhme via Phabricator via cfe-commits]

mboehme added a comment.

Note: https://reviews.llvm.org/D126780, which fixes the same bug as this patch, 
has now been landed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119165

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

[PATCH] D119165: [clang-tidy] Add processing lambda captures at bugprone-use-after-move check

[Martin Böhme via Phabricator via cfe-commits]

mboehme added inline comments.
Herald added a project: All.



Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:403
hasArgument(0, declRefExpr().bind("arg")),
-   anyOf(hasAncestor(lambdaExpr().bind("containing-lambda")),
- hasAncestor(functionDecl().bind("containing-func"))),

mboehme wrote:
> I believe this can be done more simply.
> 
> IIUC, the root of the problem is that a std::move() in a lambda capture is 
> being associated with the lambda, when in fact it should be associated with 
> the function that contains the lambda.
> 
> This is because the `hasAncestor(lambdaExpr())` matches not just a 
> `std::move` inside the body of the lambda, but also in captures.
> 
> I believe this can be solved simply by changing this line so that it only 
> matches a `std::move` inside the body of the lambda, i.e. something like this:
> 
> ```
> hasAncestor(compoundStmt(hasParent(lambdaExpr().bind("containing-lambda"
> ```
> 
> This would then no longer match a `std::move` in a capture; the existing 
> logic would instead associate it with the function that contains the lambda.
> ```
It appears that this works:

https://reviews.llvm.org/D126780


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119165

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

[PATCH] D119165: [clang-tidy] Add processing lambda captures at bugprone-use-after-move check

[Martin Böhme via Phabricator via cfe-commits]

mboehme added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:403
hasArgument(0, declRefExpr().bind("arg")),
-   anyOf(hasAncestor(lambdaExpr().bind("containing-lambda")),
- hasAncestor(functionDecl().bind("containing-func"))),

I believe this can be done more simply.

IIUC, the root of the problem is that a std::move() in a lambda capture is 
being associated with the lambda, when in fact it should be associated with the 
function that contains the lambda.

This is because the `hasAncestor(lambdaExpr())` matches not just a `std::move` 
inside the body of the lambda, but also in captures.

I believe this can be solved simply by changing this line so that it only 
matches a `std::move` inside the body of the lambda, i.e. something like this:

```
hasAncestor(compoundStmt(hasParent(lambdaExpr().bind("containing-lambda"
```

This would then no longer match a `std::move` in a capture; the existing logic 
would instead associate it with the function that contains the lambda.
```



Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:418
+  forEachLambdaCapture(lambdaCapture(capturesVar(varDecl(
+  hasInitializer(expr(ignoringParenImpCasts(BaseMoveMatcher))),
+  AncestorMatcher);

sammccall wrote:
> This only matches when the initializer is exactly `std::move(x)`.
> Not e.g. if it's `SomeType(std::move(x))`. 
> 
> Former is probably the common case, but is this deliberate? If we're not sure 
> exactly which cases are safe, maybe add a FIXME?
If this isn't a deliberate restriction, can you add a test for this?



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp:1355
+// Check std::move usage inside lambda captures
+int lambdaCaptures() {
+  int a = 0;

Can  you put this test with the other tests for lambdas? My suggestion would be 
to insert it below line 418.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp:1356
+int lambdaCaptures() {
+  int a = 0;
+  int b = 0;

ivanmurashko wrote:
> sammccall wrote:
> > It's pretty interesting that use-after-move fires for ints!
> > 
> > Someone might decide to "fix" that though, so probably best to use A like 
> > the other tests.
> There is also another case that I want to address as a separate patch.
> ```
> void autoCapture() {
>   auto var = [](auto &) {
> auto f = [blk = std::move(res)]() {};
> return std::move(res);
>   };
> }
> ```
> This one is matched as `UnresolvedLookupExpr` and requires another modified 
> matcher
I assume the reason you don't want to address this case within this particular 
patch is that it requires a lot of additional code?



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp:1363
+// CHECK-NOTES: [[@LINE+4]]:14: warning: 'b' used after it was moved
+// CHECK-NOTES: [[@LINE-4]]:39: note: move occurred here
+return aa + bb + cc;

Can you put these `CHECK-NOTES` after the line `return a + b + c`?

The other tests put the `CHECK-NOTES` after the use, not the move, and it would 
be nice to be consistent with that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119165

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

[PATCH] D119165: [clang-tidy] Add processing lambda captures at bugprone-use-after-move check

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:416
 
+  auto CallMoveMatcherLambda = lambdaExpr(
+  forEachLambdaCapture(lambdaCapture(capturesVar(varDecl(

sammccall wrote:
> sammccall wrote:
> > do we want to bind the lambda itself as `moving-call`?
> we should probably have a comment explaining *why* lambdas get handled 
> specially.
> 
> If I understand right:
> - the normal matcher would already match
> - but either MovingCall or ContainingLambda (which?) point at unhelpful nodes 
> and
> - we end up doing the analysis inside the lambda rather than in the enclosing 
> function
> - so never find the following use
> 
> (I wonder if it's possible to fix this slightly more directly by tweaking the 
> MovingCall or ContainingLambda logic)
> If I understand right:

There are some troubles with the original matcher. The most obvious one is 
correctly described at your comment :
The original matcher
```
callExpr(callee(functionDecl(hasName("::std::move"))),
   ... hasAncestor(lambdaExpr().bind("containing-lambda")),
   ...
```
applied to the code
```
auto []() { // lambda_1
   int a = 0;
   ...
   auto [](aa = std::move(a)) { // lambda_2
   }
}
```
will return *lambda_2* binded to the "containing-lambda" but we expect it to be 
*lambda_1*



> (I wonder if it's possible to fix this slightly more directly by tweaking the 
> MovingCall or ContainingLambda logic)
It would be good to find it if it's possible



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp:1356
+int lambdaCaptures() {
+  int a = 0;
+  int b = 0;

sammccall wrote:
> It's pretty interesting that use-after-move fires for ints!
> 
> Someone might decide to "fix" that though, so probably best to use A like the 
> other tests.
There is also another case that I want to address as a separate patch.
```
void autoCapture() {
  auto var = [](auto &) {
auto f = [blk = std::move(res)]() {};
return std::move(res);
  };
}
```
This one is matched as `UnresolvedLookupExpr` and requires another modified 
matcher


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119165

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

[PATCH] D119165: [clang-tidy] Add processing lambda captures at bugprone-use-after-move check

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This is a subtle and complicated check I'm not so familiar with. I have some 
ideas but I'm not confident in the suggestions or if we're missing something.

@mboehme is the expert on this check, he's OOO this week/next week. I'm happy 
to muddle along or we can wait for him to take a look :-)




Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:404
+
+  auto BaseMoveMatcher =
   callExpr(callee(functionDecl(hasName("::std::move"))), 
argumentCountIs(1),

Hmm, I think the original name (`CallMoveMatcher`) was good as it matches the 
actual call.

I'd probably name the lambda one LambdaWithMoveInitMatcher and the other... 
actually it's very close to CallMoveMatcher itself, maybe just inline it?



Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:416
 
+  auto CallMoveMatcherLambda = lambdaExpr(
+  forEachLambdaCapture(lambdaCapture(capturesVar(varDecl(

do we want to bind the lambda itself as `moving-call`?



Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:416
 
+  auto CallMoveMatcherLambda = lambdaExpr(
+  forEachLambdaCapture(lambdaCapture(capturesVar(varDecl(

sammccall wrote:
> do we want to bind the lambda itself as `moving-call`?
we should probably have a comment explaining *why* lambdas get handled 
specially.

If I understand right:
- the normal matcher would already match
- but either MovingCall or ContainingLambda (which?) point at unhelpful nodes 
and
- we end up doing the analysis inside the lambda rather than in the enclosing 
function
- so never find the following use

(I wonder if it's possible to fix this slightly more directly by tweaking the 
MovingCall or ContainingLambda logic)



Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:418
+  forEachLambdaCapture(lambdaCapture(capturesVar(varDecl(
+  hasInitializer(expr(ignoringParenImpCasts(BaseMoveMatcher))),
+  AncestorMatcher);

This only matches when the initializer is exactly `std::move(x)`.
Not e.g. if it's `SomeType(std::move(x))`. 

Former is probably the common case, but is this deliberate? If we're not sure 
exactly which cases are safe, maybe add a FIXME?



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp:1356
+int lambdaCaptures() {
+  int a = 0;
+  int b = 0;

It's pretty interesting that use-after-move fires for ints!

Someone might decide to "fix" that though, so probably best to use A like the 
other tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119165

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

[PATCH] D119165: [clang-tidy] Add processing lambda captures at bugprone-use-after-move check

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko created this revision.
ivanmurashko added reviewers: alexfh, sammccall, mboehme, aaron.ballman.
ivanmurashko added projects: clang, clang-tools-extra.
Herald added subscribers: carlosgalvezp, xazax.hun.
ivanmurashko requested review of this revision.
Herald added a subscriber: cfe-commits.

The diff adds processing for `std::move` inside lambda captures at 
`bugprone-use-after-move` check. Especially it detects invalid `std::move` 
inside following code

  int foo() {
int a = 0;
auto fun = [aa = std::move(a)]{
  return aa;
};
return a;
  }

Test Plan

  ./bin/llvm-lit -v  
../clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D119165

Files:
  clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
@@ -1350,3 +1350,18 @@
 private:
   std::string val_;
 };
+
+// Check std::move usage inside lambda captures
+int lambdaCaptures() {
+  int a = 0;
+  int b = 0;
+  int c = 0;
+  auto foo = [aa = std::move(a), bb = std::move(b), cc = c] {
+// CHECK-NOTES: [[@LINE+6]]:10: warning: 'a' used after it was moved
+// CHECK-NOTES: [[@LINE-2]]:20: note: move occurred here
+// CHECK-NOTES: [[@LINE+4]]:14: warning: 'b' used after it was moved
+// CHECK-NOTES: [[@LINE-4]]:39: note: move occurred here
+return aa + bb + cc;
+  };
+  return a + b + c;
+};
Index: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -397,11 +397,13 @@
 }
 
 void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) {
-  auto CallMoveMatcher =
+  auto AncestorMatcher =
+  anyOf(hasAncestor(lambdaExpr().bind("containing-lambda")),
+hasAncestor(functionDecl().bind("containing-func")));
+
+  auto BaseMoveMatcher =
   callExpr(callee(functionDecl(hasName("::std::move"))), 
argumentCountIs(1),
hasArgument(0, declRefExpr().bind("arg")),
-   anyOf(hasAncestor(lambdaExpr().bind("containing-lambda")),
- hasAncestor(functionDecl().bind("containing-func"))),
unless(inDecltypeOrTemplateArg()),
// try_emplace is a common maybe-moving function that returns a
// bool to tell callers whether it moved. Ignore std::move 
inside
@@ -411,6 +413,16 @@
callee(cxxMethodDecl(hasName("try_emplace")))
   .bind("call-move");
 
+  auto CallMoveMatcherLambda = lambdaExpr(
+  forEachLambdaCapture(lambdaCapture(capturesVar(varDecl(
+  hasInitializer(expr(ignoringParenImpCasts(BaseMoveMatcher))),
+  AncestorMatcher);
+
+  Finder->addMatcher(CallMoveMatcherLambda, this);
+
+  auto CallMoveMatcherNoLambda =
+  expr(ignoringParenImpCasts(BaseMoveMatcher), AncestorMatcher);
+
   Finder->addMatcher(
   traverse(
   TK_AsIs,
@@ -418,7 +430,7 @@
   // for the direct ancestor of the std::move() that isn't one of the
   // node types ignored by ignoringParenImpCasts().
   stmt(
-  forEach(expr(ignoringParenImpCasts(CallMoveMatcher))),
+  forEach(CallMoveMatcherNoLambda),
   // Don't allow an InitListExpr to be the moving call. An
   // InitListExpr has both a syntactic and a semantic form, and the
   // parent-child relationships are different between the two. This


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
@@ -1350,3 +1350,18 @@
 private:
   std::string val_;
 };
+
+// Check std::move usage inside lambda captures
+int lambdaCaptures() {
+  int a = 0;
+  int b = 0;
+  int c = 0;
+  auto foo = [aa = std::move(a), bb = std::move(b), cc = c] {
+// CHECK-NOTES: [[@LINE+6]]:10: warning: 'a' used after it was moved
+// CHECK-NOTES: [[@LINE-2]]:20: note: move occurred here
+// CHECK-NOTES: [[@LINE+4]]:14: warning: 'b' used after it was moved
+// CHECK-NOTES: [[@LINE-4]]:39: note: move occurred here
+return aa + bb + cc;
+  };
+  return a + b + c;
+};
Index: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++