[clang] [StaticAnalyzer] Fix state update in VisitObjCForCollectionStmt (PR #124477)

[Ziqing Luo via cfe-commits]

ziqingluo-90 wrote:

> I wanted to push to your branch but for some reason I could not. Here is what I would have proposed to unblock this change:
> 
> ```diff
> commit ab9670b613be2bdd802342f031bd5e3d20680925
> Author: Balazs Benics <benicsbalazs at gmail.com>
> Date:   2025.01.29 13:02:16
> 
>     Add a unittest demonstrating that we no longer crash
> 
> diff --git a/clang/unittests/StaticAnalyzer/CMakeLists.txt b/clang/unittests/StaticAnalyzer/CMakeLists.txt
> index f5da86e54560..7b574bdf7cbc 100644
> --- a/clang/unittests/StaticAnalyzer/CMakeLists.txt
> +++ b/clang/unittests/StaticAnalyzer/CMakeLists.txt
> @@ -15,6 +15,7 @@ add_clang_unittest(StaticAnalysisTests
>    IsCLibraryFunctionTest.cpp
>    MemRegionDescriptiveNameTest.cpp
>    NoStateChangeFuncVisitorTest.cpp
> +  ObjCTest.cpp
>    ParamRegionTest.cpp
>    RangeSetTest.cpp
>    RegisterCustomCheckersTest.cpp
> diff --git a/clang/unittests/StaticAnalyzer/ObjCTest.cpp b/clang/unittests/StaticAnalyzer/ObjCTest.cpp
> new file mode 100644
> index 000000000000..16cab5336813
> --- /dev/null
> +++ b/clang/unittests/StaticAnalyzer/ObjCTest.cpp
> @@ -0,0 +1,62 @@
> +//===----------------------------------------------------------------------===//
> +//
> +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
> +// See https://llvm.org/LICENSE.txt for license information.
> +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
> +//
> +//===----------------------------------------------------------------------===//
> +
> +#include "CheckerRegistration.h"
> +#include "clang/StaticAnalyzer/Core/Checker.h"
> +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
> +#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
> +#include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
> +#include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
> +#include "gtest/gtest.h"
> +
> +using namespace clang;
> +using namespace ento;
> +
> +// Some dummy trait that we can mutate back and forth to force a new State.
> +REGISTER_TRAIT_WITH_PROGRAMSTATE(Flag, bool)
> +
> +namespace {
> +class FlipFlagOnCheckLocation : public Checker<check::Location> {
> +public:
> +  // We make sure we alter the State every time we model a checkLocation event.
> +  void checkLocation(SVal l, bool isLoad, const Stmt *S,
> +                     CheckerContext &C) const {
> +    ProgramStateRef State = C.getState();
> +    State = State->set<Flag>(!State->get<Flag>());
> +    C.addTransition(State);
> +  }
> +};
> +
> +void addFlagFlipperChecker(AnalysisASTConsumer &AnalysisConsumer,
> +                           AnalyzerOptions &AnOpts) {
> +  AnOpts.CheckersAndPackages = {{"test.FlipFlagOnCheckLocation", true}};
> +  AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
> +    Registry.addChecker<FlipFlagOnCheckLocation>("test.FlipFlagOnCheckLocation",
> +                                                 "Description", "");
> +  });
> +}
> +
> +TEST(ObjCTest, CheckLocationEventsShouldMaterializeInObjCForCollectionStmts) {
> +  // Previously, the `ExprEngine::hasMoreIteration` may fired an assertion
> +  // because we forgot to handle correctly the resulting nodes of the
> +  // check::Location callback for the ObjCForCollectionStmts.
> +  // This caused inconsistencies in the graph and triggering the assertion.
> +  std::string Diags;
> +  EXPECT_TRUE(runCheckerOnCodeWithArgs<addFlagFlipperChecker>(
> +      R"(
> +    @class NSArray, NSDictionary, NSString;
> +    extern void NSLog(NSString *format, ...) __attribute__((format(__NSString__, 1, 2)));
> +    void entrypoint(NSArray *bowl) {
> +      for (NSString *fruit in bowl) { // no-crash
> +        NSLog(@"Fruit: %@", fruit);
> +      }
> +    })",
> +      {"-x", "objective-c"}, Diags));
> +}
> +
> +} // namespace
> ```
> 
> Feel free to tweak it, especially the comment as you know the most of this context. I checked and without your fix the test would crash, and after your fix it would work.

I wish I could see your reply earlier!  It took me a while to reduce the downstream reproducer and come up a minimal test.  This way of testing looks great.  Maybe I should add this test anyway.  Thank you!

https://github.com/llvm/llvm-project/pull/124477