diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -399,6 +399,18 @@ } } +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(*Loc)); + } + } +} + void transferMakeOptionalCall(const CallExpr *E, const MatchFinder::MatchResult &, LatticeTransferState &State) { @@ -774,25 +786,22 @@ 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`, 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(valueOperatorCall(std::nullopt), + // optional::operator* + .CaseOfCFGStmt(isOptionalOperatorCallWithName("*"), [](const CallExpr *E, const MatchFinder::MatchResult &, LatticeTransferState &State) { transferUnwrapCall(E, E->getArg(0), State); }) + // optional::operator-> + .CaseOfCFGStmt(isOptionalOperatorCallWithName("->"), + [](const CallExpr *E, + const MatchFinder::MatchResult &, + LatticeTransferState &State) { + transferArrowOpCall(E, E->getArg(0), State); + }) + // optional::has_value .CaseOfCFGStmt( isOptionalMemberCallWithName("has_value"), diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp --- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp @@ -2764,9 +2764,6 @@ } 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" @@ -2786,7 +2783,7 @@ } // Now we merge the two values. UncheckedOptionalAccessModel::merge() will // throw away the "value" property. - foo->value(); // [[unsafe]] + foo->value(); } )"); }