[PATCH] D124932: [clang][dataflow] Track `optional` contents in `optional` model.

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.


================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:170
 /// Returns the symbolic value that represents the "has_value" property of the
-/// optional value `Val`. Returns null if `Val` is null.
-BoolValue *getHasValue(Value *Val) {
+/// optional value `Val`, creating a fresh one if none is set. Returns null if
+/// `Val` is null.
----------------
Why is this necessary? Do we really need to create a new value on each `getHasValue` call? Can we centralize initialization to avoid this? I'm worried that "has_value" and "value" initialization isn't coordinated.


================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:219
+/// its location. Returns nullptr if the value can't be represented.
+StorageLocation *initializeUnwrappedValue(const Expr &Expr,
+                                          StructValue &OptionalVal,
----------------
Perhaps call this `maybeInitializeOptionalValueMember`?


================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:219
+/// its location. Returns nullptr if the value can't be represented.
+StorageLocation *initializeUnwrappedValue(const Expr &Expr,
+                                          StructValue &OptionalVal,
----------------
sgatev wrote:
> Perhaps call this `maybeInitializeOptionalValueMember`?
Let's pass the type instead of `Expr`, if that's the only thing we need.


================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:222-224
+  // The "value" property represents a synthetic field. As such, it needs
+  // `StorageLocation`, like normal fields (and other variables). So, we model
+  // it with a `ReferenceValue`, since that includes a storage location.  Once
----------------
It's unclear to me why we "need" a `StorageLocation`. Representing the value as a `ReferenceValue` seems good to me, but there are other options. What issues are we running into if we remove the indirection?


================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:231-236
+      // The property was previously set, but the value has been lost. This can
+      // happen, for example, because of an environment merge (where the two
+      // environments mapped the property to different values, which resulted in
+      // them both being discarded), or when two blocks in the CFG, with neither
+      // a dominator of the other, visit the same optional value, or even when a
+      // block is revisited during testing to collect per-statement state.
----------------
So we don't lose the `ReferenceValue`, but we do lose its pointee? Is that the behavior we want when merging?


================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:281
+
+    auto *Prop = OptionalVal->getProperty("has_value");
+    if (auto *HasValueVal = cast_or_null<BoolValue>(Prop)) {
----------------
Shouldn't we use `getHasValue` here?


================
Comment at: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp:2188-2260
+  // `reset` changes the state.
+  ExpectLatticeChecksFor(
+      R"(
+    #include "unchecked_optional_access_test.h"
+
+    struct Foo {
+      $ns::$optional<std::string> opt;
----------------
Do we really need tests for all members that check/access/reset the content? These are already covered in tests above.


================
Comment at: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp:2280
+    void target() {
+      Bar bar = createBar();
+      bar.member->opt = "a";
----------------



================
Comment at: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp:2287
+
+  // Resetting the nested optional.
+  ExpectLatticeChecksFor(
----------------
Do we really need this one or is it already covered by one of the tests in this file?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124932/new/

https://reviews.llvm.org/D124932