[clang][dataflow] Fix a bug in handling of `operator->` for optional checker.

[mboehme]

Summary

Prior to this patch, operator-> was being handled like operator*: It was
associating a Value of type T with the expression result (where T is the
template argument of the optional<T>). This is correct for operator*, which
returns a reference (of some flavor) to T, so that the result of the
CXXOperatorCallExpr is a glvalue of type T. However, operator* returns a
T*, so the result of the CXXOperatorCallExpr is a prvalue T*, which should
therefore be associated with PointerValue that in turn refers to a T.

I noticed this issue while working on the migration to strict handling of
value categories (see https://discourse.llvm.org/t/70086). The current behavior
also seems problematic more generally because it's plausible that the framework
may at some point introduce behavior that assumes an Expr of pointer type is
always associated with a PointerValue.

As it turns out, this patch fixes an existing FIXME in the test
OptionalValueInitialization.

Depends On D150657

[ymandel]

Nice catch!

clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp

789

this function will be left with only one caller. Maybe inline it? For that matter, should the other caller be changed in some parallel way?

[mboehme]

clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp

789

this function will be left with only one caller. Maybe inline it?

The general style in this file seems to be to pull matcher expressions out into helper functions, even if they're only used in one place. FWIW, I'm personally not convinced this is helpful because you _want_ to be able to see exactly what a CaseOfCFGStmt is matching on, so inlining makes sense IMO. But I'm not sure I want to be inconsistent with the general style of this file -- it would make more sense to do a general cleanup. WDYT?

For that matter, should the other caller be changed in some parallel way?

No, that caller (buildDiagnoseMatchSwitch()) does not have the same problem because it looks only at the optional, not at the value that is contained in it (see also D150756, which makes this explicit), and the distinction between operator* and operator-> (for our purposes) is only in the way they return the contained value.

[ymandel]

clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp

789

regarding the matcher -- it's a judgment call; I agree that having to jump back and forth to read this case "statement" is problematic, but some of the matchers are kind of complicated, so a name can be a service to the reader. This one looked sufficiently simple to consider just inlining it, but I'm also fine leaving as is.

[mboehme]

clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp

789

In that case, I think I'll leave this as-is for the time being and will revisit the question of inlining more generally.

clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp

if (auto *OptionalVal =
           getValueBehindPossiblePointer(*ObjectExpr, State.Env)) {
     if (State.Env.getStorageLocation(*UnwrapExpr, SkipPast::None) == nullptr)
       if (auto *Loc = maybeInitializeOptionalValueMember(
               UnwrapExpr->getType(), *OptionalVal, State.Env))
         State.Env.setStorageLocation(*UnwrapExpr, *Loc);
   }
 }
 
+void transferArrowOpCall(const Expr *UnwrapExpr, const Expr *ObjectExpr,
+                         LatticeTransferState &State) {
+  if (auto *OptionalVal =
+          getValueBehindPossiblePointer(*ObjectExpr, State.Env)) {
+    if (auto *Loc = maybeInitializeOptionalValueMember(
+            UnwrapExpr->getType()->getPointeeType(), *OptionalVal, State.Env)) {
+      State.Env.setValueStrict(*UnwrapExpr,
+                               State.Env.create<PointerValue>(*Loc));
+    }
+  }
+}
+
 void transferMakeOptionalCall(const CallExpr *E,
                               const MatchFinder::MatchResult &,
                               LatticeTransferState &State) {
   auto &Loc = State.Env.createStorageLocation(*E);
   State.Env.setStorageLocation(*E, Loc);
   State.Env.setValue(
       Loc, createOptionalValue(State.Env, State.Env.getBoolLiteralValue(true)));
 }
return CFGMatchSwitchBuilder<LatticeTransferState>()
       // optional::value
       .CaseOfCFGStmt<CXXMemberCallExpr>(
           valueCall(std::nullopt),
           [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
              LatticeTransferState &State) {
             transferUnwrapCall(E, E->getImplicitObjectArgument(), State);
           })
 
-      // optional::operator*, optional::operator->
-      // FIXME: This does something slightly strange for `operator->`.
-      // `transferUnwrapCall()` may create a new value of type `T` for the
-      // `optional<T>`, and it associates that value with `E`. In the case of
-      // `operator->`, `E` is a pointer. As a result, we associate an
-      // expression of pointer type with a storage location of non-pointer type
-      // `T`. This can confound other code that expects expressions of
-      // pointer type to be associated with `PointerValue`s, such as the
-      // centrally provided accessors `getImplicitObjectLocation()` and
-      // `getBaseObjectLocation()`, and this is the reason we need to use our
-      // own 'maybeSkipPointer()` and `getValueBehindPossiblePointer()` instead
-      // of these accessors.
-      .CaseOfCFGStmt<CallExpr>(valueOperatorCall(std::nullopt),
+      // optional::operator*
+      .CaseOfCFGStmt<CallExpr>(isOptionalOperatorCallWithName("*"),
                                [](const CallExpr *E,
                                   const MatchFinder::MatchResult &,
                                   LatticeTransferState &State) {
                                  transferUnwrapCall(E, E->getArg(0), State);
                                })
 
+      // optional::operator->
+      .CaseOfCFGStmt<CallExpr>(isOptionalOperatorCallWithName("->"),
+                               [](const CallExpr *E,
+                                  const MatchFinder::MatchResult &,
+                                  LatticeTransferState &State) {
+                                 transferArrowOpCall(E, E->getArg(0), State);
+                               })
+
       // optional::has_value
       .CaseOfCFGStmt<CXXMemberCallExpr>(
           isOptionalMemberCallWithName("has_value"),
           transferOptionalHasValueCall)
 
       // optional::operator bool
       .CaseOfCFGStmt<CXXMemberCallExpr>(
           isOptionalMemberCallWithName("operator bool"),

clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

void target($ns::$optional<Foo> foo) {
       if (foo && foo->opt) {
         foo->opt.value();
       }
     }
   )");
 }
 
 TEST_P(UncheckedOptionalAccessTest, OptionalValueInitialization) {
-  // FIXME: Fix when to initialize `value`. All unwrapping should be safe in
-  // this example, but `value` initialization is done multiple times during the
-  // fixpoint iterations and joining the environment won't correctly merge them.
   ExpectDiagnosticsFor(
       R"(
     #include "unchecked_optional_access_test.h"
 
     using Foo = $ns::$optional<std::string>;
 
     void target($ns::$optional<Foo> foo, bool b) {
       if (!foo.has_value()) return;
       if (b) {
         if (!foo->has_value()) return;
         // We have created `foo.value()`.
         foo->value();
       } else {
         if (!foo->has_value()) return;
         // We have created `foo.value()` again, in a different environment.
         foo->value();
       }
       // Now we merge the two values. UncheckedOptionalAccessModel::merge() will
       // throw away the "value" property.
-      foo->value(); // [[unsafe]]
+      foo->value();
     }
   )");
 }
 
 // This test is aimed at the core model, not the diagnostic. It is a regression
 // test against a crash when using non-trivial smart pointers, like
 // `std::unique_ptr`. As such, it doesn't test the access itself, which would be
 // ignored regardless because of `IgnoreSmartPointerDereference = true`, above.