[clang][dataflow] Do not crash on missing `Value` for struct-typed variable init.

[ymandel]

Summary

Remove constraint that an initializing expression of struct type must have an
associated Value. This invariant is not and will not be guaranteed by the
framework, because of potentially uninitialized fields.

[sgatev]

clang/lib/Analysis/FlowSensitive/Transfer.cpp

184–186

The patch makes sense to me in the current state, but it's unclear why is this not something that we'd like to do in the long term.

[ymandel]

clang/lib/Analysis/FlowSensitive/Transfer.cpp

184–186

Because of uninterpreted fields. If we consider those a temporary fix, then I agree about the long term. I'd thought those were here to stay. That said, if we find a way to either interperet fields lazily or only interpret those needed in the current function scope, we may be able to avoid uninterpreted expressions.

This also begs the question as to why we insist on initializing the variable. Arguably, if the expresssion is uninterpreted, so should be the variable. So, we should at least explain why we're distinguishing. Thoughts on my adding a comment to that effect?

[xazax.hun]

clang/lib/Analysis/FlowSensitive/Transfer.cpp

184–186

Because of uninterpreted fields.

Could you elaborate on this? Do you mean fields being uninterpreted due to hitting a depth limit or being recursive? Or is this something else?

[sgatev]

clang/lib/Analysis/FlowSensitive/Transfer.cpp

184–186

Not initializing the variable in such cases also makes sense to me. I'd very much appreciate a comment. I suggest also keeping the FIXME as interpreting fields lazily (and thus relying that all fields that are accessed are initialized) is still an option.

[ymandel]

extended comments.

clang/lib/Analysis/FlowSensitive/Transfer.cpp

void VisitDeclStmt(const DeclStmt *S) {
     if (D.getType()->isReferenceType()) {
       // Initializing a reference variable - do not create a reference to
       // reference.
       if (auto *InitExprLoc =
               Env.getStorageLocation(*InitExpr, SkipPast::Reference)) {
         auto &Val =
             Env.takeOwnership(std::make_unique<ReferenceValue>(*InitExprLoc));
         Env.setValue(Loc, Val);
-      } else {
-        // FIXME: The initializer expression must always be assigned a value.
-        // Replace this with an assert when we have sufficient coverage of
-        // language features.
-        if (Value *Val = Env.createValue(D.getType()))
-          Env.setValue(Loc, *Val);
+        return;
       }
+    } else if (auto *InitExprVal = Env.getValue(*InitExpr, SkipPast::None)) {
+      Env.setValue(Loc, *InitExprVal);
       return;
     }
 
-    if (auto *InitExprVal = Env.getValue(*InitExpr, SkipPast::None)) {
-      Env.setValue(Loc, *InitExprVal);
-    } else if (!D.getType()->isStructureOrClassType()) {
-      // FIXME: The initializer expression must always be assigned a value.
-      // Replace this with an assert when we have sufficient coverage of
-      // language features.
-      if (Value *Val = Env.createValue(D.getType()))
-        Env.setValue(Loc, *Val);
-    } else {
-      llvm_unreachable("structs and classes must always be assigned values");
-    }
+    // We arrive here in (the few) cases where an expression is intentionally
+    // "uninterpreted". There are two ways to handle this situation: propagate
+    // the status, so that uninterpreted initializers result in uninterpreted
+    // variables, or provide a default value. We choose the latter so that later
+    // refinements of the variable can be used for reasoning about the
+    // surrounding code.
+    //
+    // FIXME. If and when we interpret all language cases, change this to assert
+    // that `InitExpr` is interpreted, rather than supplying a default value
+    // (assuming we don't update the environment API to return references).
   }
 
   void VisitImplicitCastExpr(const ImplicitCastExpr *S) {
     // The CFG does not contain `ParenExpr` as top-level statements in basic
     // blocks, however sub-expressions can still be of that type.
     assert(S->getSubExpr() != nullptr);
     const Expr *SubExpr = S->getSubExpr()->IgnoreParens();
     assert(SubExpr != nullptr);

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

runDataflow(
             cast<ScalarStorageLocation>(&FooLoc->getChild(*BarDecl));
 
         const auto *FooVal = cast<StructValue>(Env.getValue(*FooLoc));
         const auto *BarVal = cast<IntegerValue>(FooVal->getChild(*BarDecl));
         EXPECT_EQ(Env.getValue(*BarLoc), BarVal);
       });
 }
 
+TEST_F(TransferTest, StructVarDeclWithInit) {
+  std::string Code = R"(
+    struct A {
+      int Bar;
+    };
+
+    A Gen();
+
+    void target() {
+      A Foo = Gen();
+      // [[p]]
+    }
+  )";
+  runDataflow(
+      Code, [](llvm::ArrayRef<
+                   std::pair<std::string, DataflowAnalysisState<NoopLattice>>>
+                   Results,
+               ASTContext &ASTCtx) {
+        ASSERT_THAT(Results, ElementsAre(Pair("p", _)));
+        const Environment &Env = Results[0].second.Env;
+
+        const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+        ASSERT_THAT(FooDecl, NotNull());
+
+        ASSERT_TRUE(FooDecl->getType()->isStructureType());
+        auto FooFields = FooDecl->getType()->getAsRecordDecl()->fields();
+
+        FieldDecl *BarDecl = nullptr;
+        for (FieldDecl *Field : FooFields) {
+          if (Field->getNameAsString() == "Bar") {
+            BarDecl = Field;
+          } else {
+            FAIL() << "Unexpected field: " << Field->getNameAsString();
+          }
+        }
+        ASSERT_THAT(BarDecl, NotNull());
+
+        const auto *FooLoc = cast<AggregateStorageLocation>(
+            Env.getStorageLocation(*FooDecl, SkipPast::None));
+        const auto *BarLoc =
+            cast<ScalarStorageLocation>(&FooLoc->getChild(*BarDecl));
+
+        const auto *FooVal = cast<StructValue>(Env.getValue(*FooLoc));
+        const auto *BarVal = cast<IntegerValue>(FooVal->getChild(*BarDecl));
+        EXPECT_EQ(Env.getValue(*BarLoc), BarVal);
+      });
+}
+
 TEST_F(TransferTest, ClassVarDecl) {
   std::string Code = R"(
     class A {
       int Bar;
     };
 
     void target() {
       A Foo;