Author: Yitzhak Mandelbaum Date: 2022-12-22T14:19:49Z New Revision: 0e8d4a6df9598cf0d654c24bbd3901bfb77d91bb
URL: https://github.com/llvm/llvm-project/commit/0e8d4a6df9598cf0d654c24bbd3901bfb77d91bb DIFF: https://github.com/llvm/llvm-project/commit/0e8d4a6df9598cf0d654c24bbd3901bfb77d91bb.diff LOG: [clang][dataflow] Simplify handling of nullopt-optionals. Previously, in the case of an optional constructed from `nullopt`, we relied on the value constructed for the `nullopt`. This complicates the implementation and exposes it to bugs (indeed, one such was found), yet doesn't improve the engine. Instead, this patch constructs a fresh optional representation, rather than relying on the underlying nullopt representation. Differential Revision: https://reviews.llvm.org/D140506 Added: Modified: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp Removed: ################################################################################ diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index da3ffce1d1d8e..07ec16c9cc6ef 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -438,12 +438,11 @@ void transferCallReturningOptional(const CallExpr *E, Loc, createOptionalValue(State.Env, State.Env.makeAtomicBoolValue())); } -void assignOptionalValue(const Expr &E, LatticeTransferState &State, +void assignOptionalValue(const Expr &E, Environment &Env, BoolValue &HasValueVal) { if (auto *OptionalLoc = - State.Env.getStorageLocation(E, SkipPast::ReferenceThenPointer)) { - State.Env.setValue(*OptionalLoc, - createOptionalValue(State.Env, HasValueVal)); + Env.getStorageLocation(E, SkipPast::ReferenceThenPointer)) { + Env.setValue(*OptionalLoc, createOptionalValue(Env, HasValueVal)); } } @@ -479,7 +478,7 @@ void transferValueOrConversionConstructor( LatticeTransferState &State) { assert(E->getNumArgs() > 0); - assignOptionalValue(*E, State, + assignOptionalValue(*E, State.Env, valueOrConversionHasValue(*E->getConstructor(), *E->getArg(0), MatchRes, State)); @@ -647,35 +646,35 @@ auto buildTransferMatchSwitch() { // make_optional .CaseOfCFGStmt<CallExpr>(isMakeOptionalCall(), transferMakeOptionalCall) - // optional::optional + // optional::optional (in place) .CaseOfCFGStmt<CXXConstructExpr>( isOptionalInPlaceConstructor(), [](const CXXConstructExpr *E, const MatchFinder::MatchResult &, LatticeTransferState &State) { - assignOptionalValue(*E, State, State.Env.getBoolLiteralValue(true)); + assignOptionalValue(*E, State.Env, + State.Env.getBoolLiteralValue(true)); }) + // nullopt_t::nullopt_t .CaseOfCFGStmt<CXXConstructExpr>( isNulloptConstructor(), [](const CXXConstructExpr *E, const MatchFinder::MatchResult &, LatticeTransferState &State) { - assignOptionalValue(*E, State, + assignOptionalValue(*E, State.Env, State.Env.getBoolLiteralValue(false)); }) + // optional::optional(nullopt_t) .CaseOfCFGStmt<CXXConstructExpr>( isOptionalNulloptConstructor(), [](const CXXConstructExpr *E, const MatchFinder::MatchResult &, LatticeTransferState &State) { - // Shares a temporary with the underlying `nullopt_t` instance. - if (auto *OptionalLoc = - State.Env.getStorageLocation(*E, SkipPast::None)) { - State.Env.setValue( - *OptionalLoc, - *State.Env.getValue(*E->getArg(0), SkipPast::None)); - } + assignOptionalValue(*E, State.Env, + State.Env.getBoolLiteralValue(false)); }) + // optional::optional (value/conversion) .CaseOfCFGStmt<CXXConstructExpr>(isOptionalValueOrConversionConstructor(), transferValueOrConversionConstructor) + // optional::operator= .CaseOfCFGStmt<CXXOperatorCallExpr>( isOptionalValueOrConversionAssignment(), @@ -714,7 +713,7 @@ auto buildTransferMatchSwitch() { isOptionalMemberCallWithName("emplace"), [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &, LatticeTransferState &State) { - assignOptionalValue(*E->getImplicitObjectArgument(), State, + assignOptionalValue(*E->getImplicitObjectArgument(), State.Env, State.Env.getBoolLiteralValue(true)); }) @@ -723,7 +722,7 @@ auto buildTransferMatchSwitch() { isOptionalMemberCallWithName("reset"), [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &, LatticeTransferState &State) { - assignOptionalValue(*E->getImplicitObjectArgument(), State, + assignOptionalValue(*E->getImplicitObjectArgument(), State.Env, State.Env.getBoolLiteralValue(false)); }) diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp index 7c95119ef68d6..4d9c57f0dacd5 100644 --- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp @@ -1273,7 +1273,6 @@ class UncheckedOptionalAccessTest ExpectDiagnosticsFor(SourceCode, ast_matchers::hasName("target")); } -private: template <typename FuncDeclMatcher> void ExpectDiagnosticsFor(std::string SourceCode, FuncDeclMatcher FuncMatcher) { @@ -2939,6 +2938,38 @@ TEST_P(UncheckedOptionalAccessTest, StructuredBindingsFromTupleLikeType) { )"); } +TEST_P(UncheckedOptionalAccessTest, CtorInitializerNullopt) { + using namespace ast_matchers; + ExpectDiagnosticsFor( + R"( + #include "unchecked_optional_access_test.h" + + struct Target { + Target(): opt($ns::nullopt) { + opt.value(); // [[unsafe]] + } + $ns::$optional<int> opt; + }; + )", + cxxConstructorDecl(ofClass(hasName("Target")))); +} + +TEST_P(UncheckedOptionalAccessTest, CtorInitializerValue) { + using namespace ast_matchers; + ExpectDiagnosticsFor( + R"( + #include "unchecked_optional_access_test.h" + + struct Target { + Target(): opt(3) { + opt.value(); + } + $ns::$optional<int> opt; + }; + )", + cxxConstructorDecl(ofClass(hasName("Target")))); +} + // FIXME: Add support for: // - constructors (copy, move) // - assignment operators (default, copy, move) _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
