[Clang] enhance loop analysis to handle variable changes inside lambdas

[a-tarasyuk]

Fixes #132038

---

This PR extends -Wloop-analysis to handle variable modifications inside lambda expressions.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Oleksandr T. (a-tarasyuk)

Changes

Fixes #132038

---

This PR extends -Wloop-analysis to handle variable modifications inside lambda expressions.

---

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

3 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+2)
• (modified) clang/lib/Sema/SemaStmt.cpp (+12-2)
• (modified) clang/test/SemaCXX/warn-loop-analysis.cpp (+15)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 5217e04b5e83f..77d15d798097b 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -319,6 +319,8 @@ Improvements to Clang's diagnostics
 - ``-Wc++98-compat`` no longer diagnoses use of ``__auto_type`` or
   ``decltype(auto)`` as though it was the extension for ``auto``. (#GH47900)
 
+- The ``-Wloop-analysis`` warning now handles variable modifications inside lambda expressions (#GH132038).
+
 Improvements to Clang's time-trace
 ----------------------------------
 
diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
index e1b9ccc693bd5..a1718d5a549e4 100644
--- a/clang/lib/Sema/SemaStmt.cpp
+++ b/clang/lib/Sema/SemaStmt.cpp
@@ -1995,9 +1995,19 @@ namespace {
     }
 
     void VisitDeclRefExpr(DeclRefExpr *E) {
-      if (VarDecl *VD = dyn_cast<VarDecl>(E->getDecl()))
+      if (VarDecl *VD = dyn_cast<VarDecl>(E->getDecl())) {
         if (Decls.count(VD))
           FoundDecl = true;
+      } else {
+        if (CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(E->getDecl()))
+          if (isLambdaCallOperator(MD))
+            Visit(MD->getBody());
+      }
+    }
+
+    void VisitCXXOperatorCallExpr(CXXOperatorCallExpr *E) {
+      if (Expr *Callee = E->getCallee())
+        Visit(Callee);
     }
 
     void VisitPseudoObjectExpr(PseudoObjectExpr *POE) {
@@ -2014,7 +2024,7 @@ namespace {
 
     bool FoundDeclInUse() { return FoundDecl; }
 
-  };  // end class DeclMatcher
+  }; // end class DeclMatcher
 
   void CheckForLoopConditionalStatement(Sema &S, Expr *Second,
                                         Expr *Third, Stmt *Body) {
diff --git a/clang/test/SemaCXX/warn-loop-analysis.cpp b/clang/test/SemaCXX/warn-loop-analysis.cpp
index 324dd386292ac..cfed3c23a2e9e 100644
--- a/clang/test/SemaCXX/warn-loop-analysis.cpp
+++ b/clang/test/SemaCXX/warn-loop-analysis.cpp
@@ -299,3 +299,18 @@ void test10() {
   for (auto[i, j, k] = arr; i < a; ++i) { }
   for (auto[i, j, k] = arr; i < a; ++arr[0]) { }
 };
+
+extern void foo(int);
+void test11() {
+  int a = 0;
+  auto incr_a = [&a]() { ++a; };
+
+  for (int b = 10; a <= b; incr_a())
+    foo(a);
+
+  for (int b = 10; a <= b;)
+    incr_a();
+
+  for (int b = 10; a <= b; [&a]() { ++a; }()) { }
+  for (int b = 10; a <= b; [&a]() { }()) { } // expected-warning {{variables 'a' and 'b' used in loop condition not modified in loop body}}
+}

[shafik]

We normally wrap tests from bug reports in namespace GHXXXX where XXXX is the bug report number.

[shafik]

Can we also get tests that show diagnostics for lambda cases as well.

[a-tarasyuk]

@shafik, thanks for the review. I've updated tests

[cor3ntin]

LGTM, Thanks!

[zyn0217]

do you know why we don't complain about this case?

[a-tarasyuk]

@zyn0217, that's a good question. In this case, I followed a similar example with functions where the body isn't taken into account

llvm-project/clang/test/SemaCXX/warn-loop-analysis.cpp

Line 23 in db0f754

for (int i; i < 1; ) { by_ref(i); }

llvm-project/clang/test/SemaCXX/warn-loop-analysis.cpp

Line 8 in db0f754

void by_ref(int &value) { }

[zyn0217]

Can you test this case?

void test() {
  int a = 0;
  int *b = &a;
  auto increase = [b]() { ++*b; };
  for (a = 10; a <= 20; increase()) {}
}

(Sorry, I mean for (a = 10; ...))

[zyn0217]

Or if that is something we can't handle at the moment, can you please add a FIXME?

[a-tarasyuk]

@zyn0217 I've added a test case with a FIXME. The original test already briefly mentions cases involving dereferencing...

llvm-project/clang/test/SemaCXX/warn-loop-analysis.cpp

Lines 40 to 41 in db0f754

	// Dereferencing pointers is ignored for now.
	for (int *i; *i; ) {}

rewritten using dataflow analysis

I suppose it’s better to rely on dataflow analysis when handling more complex cases...

[zyn0217]

@cor3ntin I question if -Wloop-analysis should be completely rewritten using dataflow analysis, rather than AST based matching.

[cor3ntin]

@cor3ntin I question if -Wloop-analysis should be completely rewritten using dataflow analysis, rather than AST based matching.

That's an interesting idea, but I don't think that should stop us from landing this improvement.
Maybe we want to open an issue for that?
@AaronBallman

[AaronBallman]

@cor3ntin I question if -Wloop-analysis should be completely rewritten using dataflow analysis, rather than AST based matching.

That's an interesting idea, but I don't think that should stop us from landing this improvement. Maybe we want to open an issue for that? @AaronBallman

Yeah, that really should be reworked -- the diagnostic is already off-by-default, so users need to explicitly request it, which means making it an analysis-based diagnostic based on the CFG isn't going to cause compile time performance concerns IMO.

[a-tarasyuk]

@AaronBallman Should this issue ideally be resolved by CFG rather than the current AST matcher fix?

[AaronBallman]

@AaronBallman Should this issue ideally be resolved by CFG rather than the current AST matcher fix?

I think so. CC @gribozavr @ymand @sgatev for additional opinions