[clang][dataflow] Relax assert on existence of `this` pointee storage

[li.zhe.hua]

Summary

Support for unions is incomplete (per 99f7d55e) and the this pointee
storage location is not set for unions. The assert in
VisitCXXThisExpr is then guaranteed to trigger when analyzing member
functions of a union.

This commit changes the assert to an early-return. Any expression may
be undefined, and so having a value for the CXXThisExpr is not a
postcondition of the transfer function.

[ymandel]

Please wait for at least one more "accept". thanks!

[sgatev]

clang/lib/Analysis/FlowSensitive/Transfer.cpp

282–285

Please document when that could happen.

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

3181–3190

[li.zhe.hua]

Address reviewer comments.

[li.zhe.hua]

Accidentally a word...

clang/lib/Analysis/FlowSensitive/Transfer.cpp

void VisitUnaryOperator(const UnaryOperator *S) {
     }
     default:
       break;
     }
   }
 
   void VisitCXXThisExpr(const CXXThisExpr *S) {
     auto *ThisPointeeLoc = Env.getThisPointeeStorageLocation();
-    assert(ThisPointeeLoc != nullptr);
+    if (ThisPointeeLoc == nullptr)
+      // Unions are supported yet, and will not have a location for the
+      // expression's pointee.
+      return;
 
     auto &Loc = Env.createStorageLocation(*S);
     Env.setStorageLocation(*S, Loc);
     Env.setValue(Loc, Env.takeOwnership(
                           std::make_unique<PointerValue>(*ThisPointeeLoc)));
   }
 
   void VisitMemberExpr(const MemberExpr *S) {

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

 using ::testing::Pair;
 using ::testing::SizeIs;
 
 class TransferTest : public ::testing::Test {
 protected:
   template <typename Matcher>
   void runDataflow(llvm::StringRef Code, Matcher Match,
                    LangStandard::Kind Std = LangStandard::lang_cxx17,
-                   bool ApplyBuiltinTransfer = true) {
+                   bool ApplyBuiltinTransfer = true,
+                   llvm::StringRef TargetFun = "target") {
     ASSERT_THAT_ERROR(
         test::checkDataflow<NoopAnalysis>(
-            Code, "target",
+            Code, TargetFun,
             [ApplyBuiltinTransfer](ASTContext &C, Environment &) {
               return NoopAnalysis(C, ApplyBuiltinTransfer);
             },
             [&Match](
                 llvm::ArrayRef<
                     std::pair<std::string, DataflowAnalysisState<NoopLattice>>>
                     Results,
                 ASTContext &ASTCtx) { Match(Results, ASTCtx); },
runDataflow(
 
         // The loop body may not have been executed, so we should not conclude
         // that `l` points to `val`.
         EXPECT_NE(&LVal->getPointeeLoc(),
                   OuterEnv.getStorageLocation(*ValDecl, SkipPast::Reference));
 });
 }
 
+TEST_F(TransferTest, DoesNotCrashOnUnionThisExpr) {
+  std::string Code = R"(
+    union Union {
+      int A;
+      float B;
+    };
+
+    void foo() {
+      Union A;
+      Union B;
+      A = B;
+    }
+  )";
+  // This is a crash regression test when calling the transfer function on a
+  // `CXXThisExpr` that refers to a union.
+  runDataflow(
+      Code,
+      [](llvm::ArrayRef<
+             std::pair<std::string, DataflowAnalysisState<NoopLattice>>>,
+         ASTContext &) {},
+      LangStandard::lang_cxx17, /*ApplyBuiltinTransfer=*/true, "operator=");
+}
+
 } // namespace