[clang][dataflow] Modify `optional` model to handle type aliases.

[ymandel]

Summary

Previously, type aliases were not handled (and resulted in an assertion
firing). This patch generalizes the model to consider aliases everywhere (a
previous patch already considered aliases for optional-returning functions).

[sgatev]

clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

2217

Let's replace "check"/"checker" with "model" here and in the commit message.

[ymandel]

address comments

[xazax.hun]

This patch looks good to me. On the other hand, I was wondering whether functions like this would be handled:

pair<optional, optional> f();
array<optional, 4> g();
MyStructWithOptionals h();

And so on. In general, I wonder if it is a better approach to be able to plug the optional creation function into the general value creation function of the engine. So whenever the engine encounters an optional type, it could create the corresponding value. It could do it lazily or eagerly, however it choses. And the user would no longer need to match all the possible ways an optional type can be mentioned somewhere.

[ymandel]

In D126972#3556230, @xazax.hun wrote:

This patch looks good to me. On the other hand, I was wondering whether functions like this would be handled:

pair<optional, optional> f();
array<optional, 4> g();
MyStructWithOptionals h();

And so on. In general, I wonder if it is a better approach to be able to plug the optional creation function into the general value creation function of the engine. So whenever the engine encounters an optional type, it could create the corresponding value. It could do it lazily or eagerly, however it choses. And the user would no longer need to match all the possible ways an optional type can be mentioned somewhere.

Thanks! Good points. Those results won't be handled directly, but they will be handled eventually, when accessed (via the various mechanisms that you've highlighted). I think you're right that something needs to change -- we shouldn't be trying to catch these cases, and perform the corresponding initialization, at each point. Personally, I prefer a lazy approach (and have a short proposal I hope to share soon), but eager or lazy there should be some single code that plugs in and handles the types of interest to a model, rather than the model explicitly "trapping" every point the author can remember to insert initialization.

clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

2217

Thanks, good point!

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

 DeclarationMatcher optionalClass() {
   return classTemplateSpecializationDecl(
       anyOf(hasName("std::optional"), hasName("std::__optional_storage_base"),
             hasName("__optional_destruct_base"), hasName("absl::optional"),
             hasName("base::Optional")),
       hasTemplateArgument(0, refersToType(type().bind("T"))));
 }
 
-auto hasOptionalType() { return hasType(optionalClass()); }
-
-auto hasOptionalOrAliasType() {
+auto optionalOrAliasType() {
   return hasUnqualifiedDesugaredType(
       recordType(hasDeclaration(optionalClass())));
 }
 
+/// Matches any of the spellings of the optional types and sugar, aliases, etc.
+auto hasOptionalType() { return hasType(optionalOrAliasType()); }
+
 auto isOptionalMemberCallWithName(
     llvm::StringRef MemberName,
     llvm::Optional<StatementMatcher> Ignorable = llvm::None) {
   auto Exception = unless(Ignorable ? expr(anyOf(*Ignorable, cxxThisExpr()))
                                     : cxxThisExpr());
   return cxxMemberCallExpr(
       on(expr(Exception)),
       callee(cxxMethodDecl(hasName(MemberName), ofClass(optionalClass()))));
auto isValueOrNotEqX() {
   // nodes, which would let us generalize this to any `X`.
   return binaryOperation(hasOperatorName("!="),
                          anyOf(ComparesToSame(cxxNullPtrLiteralExpr()),
                                ComparesToSame(stringLiteral(hasSize(0))),
                                ComparesToSame(integerLiteral(equals(0)))));
 }
 
 auto isCallReturningOptional() {
-  return callExpr(callee(functionDecl(
-      returns(anyOf(hasOptionalOrAliasType(),
-                    referenceType(pointee(hasOptionalOrAliasType())))))));
+  return callExpr(callee(functionDecl(returns(anyOf(
+      optionalOrAliasType(), referenceType(pointee(optionalOrAliasType())))))));
 }
 
 /// Creates a symbolic value for an `optional` value using `HasValueVal` as the
 /// symbolic value of its "has_value" property.
 StructValue &createOptionalValue(Environment &Env, BoolValue &HasValueVal) {
   auto OptionalVal = std::make_unique<StructValue>();
   OptionalVal->setProperty("has_value", HasValueVal);
   return Env.takeOwnership(std::move(OptionalVal));
auto buildTransferMatchSwitch(
     const UncheckedOptionalAccessModelOptions &Options) {
   // FIXME: Evaluate the efficiency of matchers. If using matchers results in a
   // lot of duplicated work (e.g. string comparisons), consider providing APIs
   // that avoid it through memoization.
   auto IgnorableOptional = ignorableOptional(Options);
   return MatchSwitchBuilder<LatticeTransferState>()
       // Attach a symbolic "has_value" state to optional values that we see for
       // the first time.
-      .CaseOf<Expr>(expr(anyOf(declRefExpr(), memberExpr()), hasOptionalType()),
-                    initializeOptionalReference)
+      .CaseOf<Expr>(
+          expr(anyOf(declRefExpr(), memberExpr()), hasOptionalType()),
 
       // make_optional
       .CaseOf<CallExpr>(isMakeOptionalCall(), transferMakeOptionalCall)
 
       // optional::optional
       .CaseOf<CXXConstructExpr>(
           isOptionalInPlaceConstructor(),
           [](const CXXConstructExpr *E, const MatchFinder::MatchResult &,

clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

void target() {
       opt = MakeOpt();
       opt.value();
       /*[[check-4]]*/
     }
   )",
       UnorderedElementsAre(Pair("check-4", "unsafe: input.cc:10:7")));
 }
 
+// Verifies that the model sees through aliases.
+TEST_P(UncheckedOptionalAccessTest, WithAlias) {
+  ExpectLatticeChecksFor(
+      R"(
+    #include "unchecked_optional_access_test.h"
+
+    template <typename T>
+    using MyOptional = $ns::$optional<T>;
+
+    void target(MyOptional<int> opt) {
+      opt.value();
+      /*[[check]]*/
+    }
+  )",
+      UnorderedElementsAre(Pair("check", "unsafe: input.cc:8:7")));
+}
+
 // FIXME: Add support for:
 // - constructors (copy, move)
 // - assignment operators (default, copy, move)
 // - invalidation (passing optional by non-const reference/pointer)
 // - nested `optional` values