[dataflow] CXXForRangeStmt should extend flow condition

[paulsemel]

This is needed in order to correctly assume implicit iterator validity in the iterator checker.

[llvmbot]

@llvm/pr-subscribers-clang-analysis

@llvm/pr-subscribers-clang

Author: Paul Semel (paulsemel)

Changes

This is needed in order to correctly assume implicit iterator validity in the iterator checker.

---

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

1 Files Affected:

• (modified) clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp (+6-3)

diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
index 4c88c46142d64d..f02c65094113ec 100644
--- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -103,9 +103,12 @@ class TerminatorVisitor
     return {nullptr, false};
   }
 
-  TerminatorVisitorRetTy VisitCXXForRangeStmt(const CXXForRangeStmt *) {
-    // Don't do anything special for CXXForRangeStmt, because the condition
-    // (being implicitly generated) isn't visible from the loop body.
+  TerminatorVisitorRetTy VisitCXXForRangeStmt(const CXXForRangeStmt *S) {
+    // Even though the condition isn't visible from the loop body, analysis
+    // might depend on the implicit implicit statements implied by the loop.
+    auto *Cond = S->getCond();
+    if (Cond != nullptr)
+      return extendFlowCondition(*Cond);
     return {nullptr, false};
   }
 

[martinboehme]

Can you add a test that fails without this fix?

[paulsemel]

Would this test work @martinboehme ?

[martinboehme]

See comments above -- I'm not sure that the test, if it fails, means that we need a flow condition for the range-based for. (User code will never be able to "observe" this flow condition, so I'm not sure why we need it.)

I still don't understand what the underlying purpose is that you need this flow condition for in the iterator check. Can you expand? (I want to make sure we're not dealing with an XY problem.)

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 1b87ebce924e507cbc27c2e0dc623941d16388c9 9cbe0ef60f0154cbbefaa7d3092d65e4a4ab4d2a -- clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp

View the diff from clang-format here.

diff --git a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
index b940fb8767..cd6d23c9c4 100644
--- a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
@@ -1722,13 +1722,15 @@ TEST_F(TopTest, ForRangeStmtHasFlowCondition) {
       }
     }
   )";
-  runDataflow(Code,
-              [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
-                 const AnalysisOutputs &AO) {
-                  ASSERT_THAT(Results.keys(), UnorderedElementsAre("p1"));
-                  const Environment &Env1 = getEnvironmentAtAnnotation(Results, "p1");
-                  ASSERT_TRUE(Env1.proves(Env1.arena().makeAtomRef(Env1.getFlowConditionToken())));
-              });
+  runDataflow(
+      Code,
+      [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+         const AnalysisOutputs &AO) {
+        ASSERT_THAT(Results.keys(), UnorderedElementsAre("p1"));
+        const Environment &Env1 = getEnvironmentAtAnnotation(Results, "p1");
+        ASSERT_TRUE(Env1.proves(
+            Env1.arena().makeAtomRef(Env1.getFlowConditionToken())));
+      });
 }
 
 } // namespace

[martinboehme]

Suggested change

	#include <array>
	void target(bool Foo) {
	std::array<int, 5> t;
	for (auto& i : t) {
	(void)0;
	/*[[p1]]*/
	}
	}
	#include <initializer_list>
	void target() {
	for (auto& i : {1, 2}) {
	(void)0;
	/*[[p1]]*/
	}
	}

Try to keep code snippets as simple as possible:

• I don't think Foo is needed here?
• An initializer_list is probably the simplest thing you can use to iterate over (it doesn't matter specifically what you're iterating over, right?)

[martinboehme]

Suggested change

	ASSERT_THAT(Results.keys(), UnorderedElementsAre("p1"));
	const Environment &Env1 = getEnvironmentAtAnnotation(Results, "p1");
	const Environment &Env = getEnvironmentAtAnnotation(Results, "p");

• A lot of tests still do the ASSERT_THAT(Results.keys(), UnorderedElementsAre(...));, but it's unnecessary -- getEnvironmentAtAnnotation() itself asserts that the annotation exists.

• When there is only one environment you want to query, the usual names are Env and p (will need to be changed in the code snippet too, obviously).

[martinboehme]

Does this fail without the other changes in this PR?

If it does, this looks to me more like a bug in DataflowEnvironment; this assertion should generally be true for any environment, whether or not we extended the flow condition. (In that sense, the name of the test isn't accurate, because we would expect this to pass even if the flow condition was not extended.)

[martinboehme]

Suggested change

	ASSERT_TRUE(Env1.proves(Env1.arena().makeAtomRef(Env1.getFlowConditionToken())));
	EXPECT_TRUE(Env1.proves(Env1.arena().makeAtomRef(Env1.getFlowConditionToken())));

ASSERT_ aborts the test if the check fails and should generally only be used if it's not meaningful to execute the rest of the test in this case. Otherwise, use EXPECT_ so that you can continue executing the rest of the test.

It obviously doesn't make a difference in this case, but a) it's good style to default to EXPECT_ anyway, and b) people might add more checks later, and they might forget to change the ASSERT_ to an EXPECT_.

[martinboehme]

See comments above -- I'm not sure that the test, if it fails, means that we need a flow condition for the range-based for. (User code will never be able to "observe" this flow condition, so I'm not sure why we need it.)

I still don't understand what the underlying purpose is that you need this flow condition for in the iterator check. Can you expand? (I want to make sure we're not dealing with an XY problem.)

[martinboehme]

Note code formatting failure in CI. (Use git clang-format to fix.)

[ymand]

Just a note: I've siginfiicantly simplified the code that you're modifying in this PR (#84499), so expect some merge conflicts next time you pull from main.