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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGdd38caf3b5b7: [clang][dataflow] Track `optional` contents in 
`optional` model. (authored by ymandel).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124932

Files:
  clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
  clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -2165,7 +2165,6 @@
 }
   )",
   UnorderedElementsAre(Pair("check-1", "unsafe: input.cc:9:7")));
-
   ExpectLatticeChecksFor(
   R"(
 #include "unchecked_optional_access_test.h"
@@ -2231,8 +2230,95 @@
   UnorderedElementsAre(Pair("check", "unsafe: input.cc:8:7")));
 }
 
+TEST_P(UncheckedOptionalAccessTest, OptionalValueOptional) {
+  // Basic test that nested values are populated.  We nest an optional because
+  // its easy to use in a test, but the type of the nested value shouldn't
+  // matter.
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+using Foo = $ns::$optional;
+
+void target($ns::$optional foo) {
+  if (foo && *foo) {
+foo->value();
+/*[[access]]*/
+  }
+}
+  )",
+  UnorderedElementsAre(Pair("access", "safe")));
+
+  // Mutation is supported for nested values.
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+using Foo = $ns::$optional;
+
+void target($ns::$optional foo) {
+  if (foo && *foo) {
+foo->reset();
+foo->value();
+/*[[reset]]*/
+  }
+}
+  )",
+  UnorderedElementsAre(Pair("reset", "unsafe: input.cc:9:9")));
+}
+
+// Tests that structs can be nested. We use an optional field because its easy
+// to use in a test, but the type of the field shouldn't matter.
+TEST_P(UncheckedOptionalAccessTest, OptionalValueStruct) {
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+struct Foo {
+  $ns::$optional opt;
+};
+
+void target($ns::$optional foo) {
+  if (foo && foo->opt) {
+foo->opt.value();
+/*[[access]]*/
+  }
+}
+  )",
+  UnorderedElementsAre(Pair("access", "safe")));
+}
+
+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.
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+using Foo = $ns::$optional;
+
+void target($ns::$optional 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();
+  /*[[merge]]*/
+}
+  )",
+  UnorderedElementsAre(Pair("merge", "unsafe: input.cc:19:7")));
+}
+
 // FIXME: Add support for:
 // - constructors (copy, move)
 // - assignment operators (default, copy, move)
 // - invalidation (passing optional by non-const reference/pointer)
-// - nested `optional` values
Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -179,9 +179,15 @@
 
 /// Returns the symbolic value that represents the "has_value" property of the
 /// optional value `OptionalVal`. Returns null if `OptionalVal` is null.
-BoolValue *getHasValue(Value *OptionalVal) {
-  if (OptionalVal) {
-return cast(OptionalVal->getProperty("has_value"));
+BoolValue *getHasValue(Environment , Value *OptionalVal) {
+  if (OptionalVal != nullptr) {
+auto *HasValueVal =
+cast_or_null(OptionalVal->getProperty("has_value"));
+if (HasValueVal == nullptr) {
+  HasValueVal = ();
+  OptionalVal->setProperty("has_value", *HasValueVal);
+}
+return HasValueVal;
   }
   return nullptr;
 }
@@ -218,6 +224,50 @@
  .getDesugaredType(ASTCtx));
 }
 
+/// Tries to initialize the 

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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 435537.
ymandel added a comment.

add missing blank line to test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124932

Files:
  clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
  clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -2165,7 +2165,6 @@
 }
   )",
   UnorderedElementsAre(Pair("check-1", "unsafe: input.cc:9:7")));
-
   ExpectLatticeChecksFor(
   R"(
 #include "unchecked_optional_access_test.h"
@@ -2231,8 +2230,95 @@
   UnorderedElementsAre(Pair("check", "unsafe: input.cc:8:7")));
 }
 
+TEST_P(UncheckedOptionalAccessTest, OptionalValueOptional) {
+  // Basic test that nested values are populated.  We nest an optional because
+  // its easy to use in a test, but the type of the nested value shouldn't
+  // matter.
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+using Foo = $ns::$optional;
+
+void target($ns::$optional foo) {
+  if (foo && *foo) {
+foo->value();
+/*[[access]]*/
+  }
+}
+  )",
+  UnorderedElementsAre(Pair("access", "safe")));
+
+  // Mutation is supported for nested values.
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+using Foo = $ns::$optional;
+
+void target($ns::$optional foo) {
+  if (foo && *foo) {
+foo->reset();
+foo->value();
+/*[[reset]]*/
+  }
+}
+  )",
+  UnorderedElementsAre(Pair("reset", "unsafe: input.cc:9:9")));
+}
+
+// Tests that structs can be nested. We use an optional field because its easy
+// to use in a test, but the type of the field shouldn't matter.
+TEST_P(UncheckedOptionalAccessTest, OptionalValueStruct) {
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+struct Foo {
+  $ns::$optional opt;
+};
+
+void target($ns::$optional foo) {
+  if (foo && foo->opt) {
+foo->opt.value();
+/*[[access]]*/
+  }
+}
+  )",
+  UnorderedElementsAre(Pair("access", "safe")));
+}
+
+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.
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+using Foo = $ns::$optional;
+
+void target($ns::$optional 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();
+  /*[[merge]]*/
+}
+  )",
+  UnorderedElementsAre(Pair("merge", "unsafe: input.cc:19:7")));
+}
+
 // FIXME: Add support for:
 // - constructors (copy, move)
 // - assignment operators (default, copy, move)
 // - invalidation (passing optional by non-const reference/pointer)
-// - nested `optional` values
Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -179,9 +179,15 @@
 
 /// Returns the symbolic value that represents the "has_value" property of the
 /// optional value `OptionalVal`. Returns null if `OptionalVal` is null.
-BoolValue *getHasValue(Value *OptionalVal) {
-  if (OptionalVal) {
-return cast(OptionalVal->getProperty("has_value"));
+BoolValue *getHasValue(Environment , Value *OptionalVal) {
+  if (OptionalVal != nullptr) {
+auto *HasValueVal =
+cast_or_null(OptionalVal->getProperty("has_value"));
+if (HasValueVal == nullptr) {
+  HasValueVal = ();
+  OptionalVal->setProperty("has_value", *HasValueVal);
+}
+return HasValueVal;
   }
   return nullptr;
 }
@@ -218,6 +224,50 @@
  .getDesugaredType(ASTCtx));
 }
 
+/// Tries to initialize the `optional`'s value (that is, contents), and return
+/// its location. Returns nullptr if the value can't be represented.
+StorageLocation 

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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 435528.
ymandel added a comment.

rebase and merge


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124932

Files:
  clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
  clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -2154,7 +2154,6 @@
   ExpectLatticeChecksFor(
   R"(
 #include "unchecked_optional_access_test.h"
-
 $ns::$optional MakeOpt();
 
 void target() {
@@ -2165,7 +2164,6 @@
 }
   )",
   UnorderedElementsAre(Pair("check-1", "unsafe: input.cc:9:7")));
-
   ExpectLatticeChecksFor(
   R"(
 #include "unchecked_optional_access_test.h"
@@ -2231,8 +2229,95 @@
   UnorderedElementsAre(Pair("check", "unsafe: input.cc:8:7")));
 }
 
+TEST_P(UncheckedOptionalAccessTest, OptionalValueOptional) {
+  // Basic test that nested values are populated.  We nest an optional because
+  // its easy to use in a test, but the type of the nested value shouldn't
+  // matter.
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+using Foo = $ns::$optional;
+
+void target($ns::$optional foo) {
+  if (foo && *foo) {
+foo->value();
+/*[[access]]*/
+  }
+}
+  )",
+  UnorderedElementsAre(Pair("access", "safe")));
+
+  // Mutation is supported for nested values.
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+using Foo = $ns::$optional;
+
+void target($ns::$optional foo) {
+  if (foo && *foo) {
+foo->reset();
+foo->value();
+/*[[reset]]*/
+  }
+}
+  )",
+  UnorderedElementsAre(Pair("reset", "unsafe: input.cc:9:9")));
+}
+
+// Tests that structs can be nested. We use an optional field because its easy
+// to use in a test, but the type of the field shouldn't matter.
+TEST_P(UncheckedOptionalAccessTest, OptionalValueStruct) {
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+struct Foo {
+  $ns::$optional opt;
+};
+
+void target($ns::$optional foo) {
+  if (foo && foo->opt) {
+foo->opt.value();
+/*[[access]]*/
+  }
+}
+  )",
+  UnorderedElementsAre(Pair("access", "safe")));
+}
+
+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.
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+using Foo = $ns::$optional;
+
+void target($ns::$optional 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();
+  /*[[merge]]*/
+}
+  )",
+  UnorderedElementsAre(Pair("merge", "unsafe: input.cc:19:7")));
+}
+
 // FIXME: Add support for:
 // - constructors (copy, move)
 // - assignment operators (default, copy, move)
 // - invalidation (passing optional by non-const reference/pointer)
-// - nested `optional` values
Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -179,9 +179,15 @@
 
 /// Returns the symbolic value that represents the "has_value" property of the
 /// optional value `OptionalVal`. Returns null if `OptionalVal` is null.
-BoolValue *getHasValue(Value *OptionalVal) {
-  if (OptionalVal) {
-return cast(OptionalVal->getProperty("has_value"));
+BoolValue *getHasValue(Environment , Value *OptionalVal) {
+  if (OptionalVal != nullptr) {
+auto *HasValueVal =
+cast_or_null(OptionalVal->getProperty("has_value"));
+if (HasValueVal == nullptr) {
+  HasValueVal = ();
+  OptionalVal->setProperty("has_value", *HasValueVal);
+}
+return HasValueVal;
   }
   return nullptr;
 }
@@ -218,6 +224,50 @@
  .getDesugaredType(ASTCtx));
 }
 
+/// Tries to initialize the 

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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

sgatev@ -- gentle ping.


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

https://reviews.llvm.org/D124932

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 429211.
ymandel added a comment.

remove stray FIXME from previous diff


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

https://reviews.llvm.org/D124932

Files:
  clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
  clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -2150,8 +2150,95 @@
   UnorderedElementsAre(Pair("check-1", "safe"), Pair("check-2", "safe")));
 }
 
+TEST_P(UncheckedOptionalAccessTest, OptionalValueOptional) {
+  // Basic test that nested values are populated.  We nest an optional because
+  // its easy to use in a test, but the type of the nested value shouldn't
+  // matter.
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+using Foo = $ns::$optional;
+
+void target($ns::$optional foo) {
+  if (foo && *foo) {
+foo->value();
+/*[[access]]*/
+  }
+}
+  )",
+  UnorderedElementsAre(Pair("access", "safe")));
+
+  // Mutation is supported for nested values.
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+using Foo = $ns::$optional;
+
+void target($ns::$optional foo) {
+  if (foo && *foo) {
+foo->reset();
+foo->value();
+/*[[reset]]*/
+  }
+}
+  )",
+  UnorderedElementsAre(Pair("reset", "unsafe: input.cc:9:9")));
+}
+
+// Tests that structs can be nested. We use an optional field because its easy
+// to use in a test, but the type of the field shouldn't matter.
+TEST_P(UncheckedOptionalAccessTest, OptionalValueStruct) {
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+struct Foo {
+  $ns::$optional opt;
+};
+
+void target($ns::$optional foo) {
+  if (foo && foo->opt) {
+foo->opt.value();
+/*[[access]]*/
+  }
+}
+  )",
+  UnorderedElementsAre(Pair("access", "safe")));
+}
+
+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.
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+using Foo = $ns::$optional;
+
+void target($ns::$optional 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();
+  /*[[merge]]*/
+}
+  )",
+  UnorderedElementsAre(Pair("merge", "unsafe: input.cc:19:7")));
+}
+
 // FIXME: Add support for:
 // - constructors (copy, move)
 // - assignment operators (default, copy, move)
 // - invalidation (passing optional by non-const reference/pointer)
-// - nested `optional` values
Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -167,10 +167,17 @@
 }
 
 /// 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.
+BoolValue *getHasValue(Environment , Value *Val) {
   if (auto *OptionalVal = cast_or_null(Val)) {
-return cast(OptionalVal->getProperty("has_value"));
+auto *HasValueVal =
+cast_or_null(OptionalVal->getProperty("has_value"));
+if (HasValueVal == nullptr) {
+  HasValueVal = ();
+  OptionalVal->setProperty("has_value", *HasValueVal);
+}
+return HasValueVal;
   }
   return nullptr;
 }
@@ -207,6 +214,50 @@
  .getDesugaredType(ASTCtx));
 }
 
+/// Tries to initialize the `optional`'s value (that is, contents), and return
+/// its location. Returns nullptr if the value can't be represented.
+StorageLocation *maybeInitializeOptionalValueMember(QualType Q,
+StructValue ,
+ 

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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 429210.
ymandel marked 6 inline comments as done.
ymandel added a comment.

address reviewer comments and fix bug in how the nested value as _re_created -- 
now uses the loc's type rather than the expression's type (which doesn't always 
match exactly).


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

https://reviews.llvm.org/D124932

Files:
  clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
  clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -2150,8 +2150,95 @@
   UnorderedElementsAre(Pair("check-1", "safe"), Pair("check-2", "safe")));
 }
 
+TEST_P(UncheckedOptionalAccessTest, OptionalValueOptional) {
+  // Basic test that nested values are populated.  We nest an optional because
+  // its easy to use in a test, but the type of the nested value shouldn't
+  // matter.
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+using Foo = $ns::$optional;
+
+void target($ns::$optional foo) {
+  if (foo && *foo) {
+foo->value();
+/*[[access]]*/
+  }
+}
+  )",
+  UnorderedElementsAre(Pair("access", "safe")));
+
+  // Mutation is supported for nested values.
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+using Foo = $ns::$optional;
+
+void target($ns::$optional foo) {
+  if (foo && *foo) {
+foo->reset();
+foo->value();
+/*[[reset]]*/
+  }
+}
+  )",
+  UnorderedElementsAre(Pair("reset", "unsafe: input.cc:9:9")));
+}
+
+// Tests that structs can be nested. We use an optional field because its easy
+// to use in a test, but the type of the field shouldn't matter.
+TEST_P(UncheckedOptionalAccessTest, OptionalValueStruct) {
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+struct Foo {
+  $ns::$optional opt;
+};
+
+void target($ns::$optional foo) {
+  if (foo && foo->opt) {
+foo->opt.value();
+/*[[access]]*/
+  }
+}
+  )",
+  UnorderedElementsAre(Pair("access", "safe")));
+}
+
+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.
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+using Foo = $ns::$optional;
+
+void target($ns::$optional 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();
+  /*[[merge]]*/
+}
+  )",
+  UnorderedElementsAre(Pair("merge", "unsafe: input.cc:19:7")));
+}
+
 // FIXME: Add support for:
 // - constructors (copy, move)
 // - assignment operators (default, copy, move)
 // - invalidation (passing optional by non-const reference/pointer)
-// - nested `optional` values
Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -167,10 +167,17 @@
 }
 
 /// 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.
+BoolValue *getHasValue(Environment , Value *Val) {
   if (auto *OptionalVal = cast_or_null(Val)) {
-return cast(OptionalVal->getProperty("has_value"));
+auto *HasValueVal =
+cast_or_null(OptionalVal->getProperty("has_value"));
+if (HasValueVal == nullptr) {
+  HasValueVal = ();
+  OptionalVal->setProperty("has_value", *HasValueVal);
+}
+return HasValueVal;
   }
   return nullptr;
 }
@@ -207,6 +214,51 @@
  .getDesugaredType(ASTCtx));
 }
 
+/// Tries to initialize the `optional`'s value (that is, contents), and return
+/// its location. Returns nullptr if the value can't be represented.

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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked 8 inline comments as done.
ymandel added a comment.

I'm having trouble with arcanist -- `arc diff` is crashing, but wanted to 
respond in the meantime to your questions. I hope I'll be able to actually 
upload the new diff soon...




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.

sgatev wrote:
> 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.
> 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.

We don't create a new one on each call, only when none is already populated. As 
for centralizing -- we could, but I'd like to move to lazy initialization 
anyhow, so it seemed less than ideal to put effort into centralizing at this 
point.



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

sgatev wrote:
> 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?
> 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?

We can't address it. Compare with a normal field, where the corresponding 
`AggregateStorageLocation` has a child that maps to a storage location for the 
field.  We don't support properties on `AggregateStorageLocation`, so the 
location has to exist somewhere.

What other options do you see?



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.

sgatev wrote:
> So we don't lose the `ReferenceValue`, but we do lose its pointee? Is that 
> the behavior we want when merging?
> So we don't lose the `ReferenceValue`, but we do lose its pointee? Is that 
> the behavior we want when merging?

Correct. No, that's not the behavior we want, hence the FIXME that follows. :)  
But, it at least accounts for the other two situations, so its a win overall.



Comment at: 
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:281
+
+auto *Prop = OptionalVal->getProperty("has_value");
+if (auto *HasValueVal = cast_or_null(Prop)) {

sgatev wrote:
> Shouldn't we use `getHasValue` here?
>Shouldn't we use getHasValue here?

Not quite:

1. its a bit a redundant since we already need to check whether the value is a 
`StructValue`, line 274-5. I tried to keep it to where its use would not cause 
unnecessary additional checks. 
2. We don't want to create a new "has_value" here, given that the access is 
illegal in that case. We want the condition to fail in that case (line 282) and 
result in recording an error.



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 opt;

sgatev wrote:
> Do we really need tests for all members that check/access/reset the content? 
> These are already covered in tests above.
Great point. I rethought the tests in general -- I'd mostly been copying them 
over, without carefully rethinking them. I've reduced the number of tests and 
added comments explaining their purposes. Essentially, we need to test that the 
synthetic value can be used like a normal field. 



Comment at: 

[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 ,
+  StructValue ,

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 ,
+  StructValue ,

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(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 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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Thanks for the clarifications!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124932

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

In D124932#3492495 , @xazax.hun wrote:

> Overall looks good to me. I am curious what will the strategy be to properly 
> support construction. Do you plan to introduce a customization point to 
> `Env.createValue` to give checks/models a way to set properties up? Or do you 
> have something else in mind?

If we want to go the eager route, than I think that's the "right" solution.  
Anything else is brittle, since it is easy to miss a construction point.  
However, I'm more inclined to move the whole framework to a lazy initialization 
process, if we can devise one.




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 ,
+  StructValue ,

xazax.hun wrote:
> I recall some problems with convergence due to creating fresh locations 
> lazily in multiple branches. I wonder if doubling down before finding a 
> proper solution could make it harder to fix this in the near future. If this 
> is just a temporary solution until construction is better supported, it might 
> be fine.
Excellent question. Yes, you're right about the convergence issue this falls 
into the same category.  That said, we intentionally reuse the same location in 
all branches, which mitigates that issue in this case -- we are at least 
assured that the two branches will have the contents value at the same 
location. In fact, the FIXME I added is wrong (and I've adjusted it 
accordingly). The property will be shared by all blocks that share the same 
value representing the `Optional` (which for the most part is done eagerly).

To your larger point, about the advisability of this approach, even for the 
short term:
1. I'm pushing on this largely because I have the data to support that it works 
in practice. We've done multiple surveys of significant size (~1000 files, 10k 
optional accesses) and found it does very well. So, this is not much of an 
issue in practice, in large part because the contents of optionals don't tend 
to further influence the outcome of the check (e.g. in the case of nested 
optionals).
2. I thought of redoing this in an eager fashion, but realized that ultimately 
we want to move everything to a lazy model, so it seemed less than ideal to 
start trying to be eager here.




Comment at: 
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:286
   // Record that this unwrap is *not* provably safe.
   State.Lattice.getSourceLocations().insert(ObjectExpr->getBeginLoc());
 }

xazax.hun wrote:
> I wonder how well this strategy works in practice. If we have many `unwrap` 
> calls to the same `optional`, we might end up emitting lots of diagnostics. 
> An alternative approach is to assume that the `optional` has a value after 
> the first unwrap. This would ensure that we only report a particular problem 
> once and can reduce the noise.
I see your point, and agree that there does seem room for significantly 
reducing warnings, even if all are correct. But, in practice, we haven't gotten 
complaints (which is a little surprising, actually). I wonder if it would be 
good to pair the report with the surrounding compound statement, and limit to 
one per compound statement. I fear that shutting it off entirely after the 
first issue might be _too_ quiet. Either way, this is definitely something that 
we'll be actively tweaking.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124932

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 427296.
ymandel added a comment.

Fixed the FIXME


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124932

Files:
  clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
  clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -2150,8 +2150,195 @@
   UnorderedElementsAre(Pair("check-1", "safe"), Pair("check-2", "safe")));
 }
 
+TEST_P(UncheckedOptionalAccessTest, NestedOptionalInOptional) {
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+using Foo = $ns::$optional;
+
+void target($ns::$optional foo) {
+  if (foo && *foo) {
+foo->value();
+/*[[access]]*/
+  }
+}
+  )",
+  UnorderedElementsAre(Pair("access", "safe")));
+}
+
+TEST_P(UncheckedOptionalAccessTest, NestedOptionalInOptionalStruct) {
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+struct Foo {
+  $ns::$optional opt;
+};
+
+void target(const $ns::$optional& foo) {
+  if (foo && foo->opt) {
+foo->opt.value();
+/*[[access]]*/
+  }
+}
+  )",
+  UnorderedElementsAre(Pair("access", "safe")));
+
+  // `reset` changes the state.
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+struct Foo {
+  $ns::$optional opt;
+};
+
+void target($ns::$optional& foo) {
+  if (foo && foo->opt) {
+foo->opt.reset();
+foo->opt.value();
+/*[[reset]]*/
+  }
+}
+  )",
+  UnorderedElementsAre(Pair("reset", "unsafe: input.cc:11:9")));
+
+  // `has_value` and `operator*`.
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+struct Foo {
+  $ns::$optional opt;
+};
+
+void target($ns::$optional& foo) {
+  if (foo && foo->opt.has_value()) {
+*foo->opt;
+/*[[other-ops]]*/
+  }
+}
+  )",
+  UnorderedElementsAre(Pair("other-ops", "safe")));
+
+  // `operator->`.
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+struct Foo {
+  $ns::$optional opt;
+};
+
+void target($ns::$optional& foo) {
+  if (foo && foo->opt.has_value()) {
+foo->opt->empty();
+/*[[arrow]]*/
+  }
+}
+  )",
+  UnorderedElementsAre(Pair("arrow", "safe")));
+
+  // Accessing the nested optional in the wrong branch.
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+struct Foo {
+  $ns::$optional opt;
+};
+
+void target($ns::$optional& foo) {
+  if (!foo.has_value()) return;
+  if (!foo->opt.has_value()) {
+foo->opt.value();
+/*[[not-set]]*/
+  }
+}
+  )",
+  UnorderedElementsAre(Pair("not-set", "unsafe: input.cc:11:9")));
+}
+
+TEST_P(UncheckedOptionalAccessTest, NestedOptionalInOptionalStructField) {
+  // Non-standard assignment.
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+struct Foo {
+  $ns::$optional opt;
+};
+
+struct Bar {
+  $ns::$optional member;
+};
+
+Bar createBar();
+
+void target() {
+  Bar bar = createBar();
+  bar.member->opt = "a";
+  /*[[ns-assign]]*/
+}
+  )",
+  UnorderedElementsAre(Pair("ns-assign", "unsafe: input.cc:16:7")));
+
+  // Resetting the nested optional.
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+struct Member {
+  $ns::$optional opt;
+};
+
+struct Foo {
+  $ns::$optional member;
+};
+
+Foo createFoo();
+
+void target() {
+  Foo foo = createFoo();
+  foo.member->opt.reset();
+  /*[[nested-reset]]*/
+}
+  )",
+  UnorderedElementsAre(Pair("nested-reset", "unsafe: input.cc:16:7")));
+}
+
+TEST_P(UncheckedOptionalAccessTest, NestedOptionalInitialization) {
+  // 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.
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+using Foo = $ns::$optional;
+
+void target($ns::$optional 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 

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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Overall looks good to me. I am curious what will the strategy be to properly 
support construction. Do you plan to introduce a customization point to 
`Env.createValue` to give checks/models a way to set properties up? Or do you 
have something else in mind?




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 ,
+  StructValue ,

I recall some problems with convergence due to creating fresh locations lazily 
in multiple branches. I wonder if doubling down before finding a proper 
solution could make it harder to fix this in the near future. If this is just a 
temporary solution until construction is better supported, it might be fine.



Comment at: 
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:286
   // Record that this unwrap is *not* provably safe.
   State.Lattice.getSourceLocations().insert(ObjectExpr->getBeginLoc());
 }

I wonder how well this strategy works in practice. If we have many `unwrap` 
calls to the same `optional`, we might end up emitting lots of diagnostics. An 
alternative approach is to assume that the `optional` has a value after the 
first unwrap. This would ensure that we only report a particular problem once 
and can reduce the noise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124932

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 427029.
ymandel added a comment.

remove stray newline


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124932

Files:
  clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
  clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -2150,8 +2150,195 @@
   UnorderedElementsAre(Pair("check-1", "safe"), Pair("check-2", "safe")));
 }
 
+TEST_P(UncheckedOptionalAccessTest, NestedOptionalInOptional) {
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+using Foo = $ns::$optional;
+
+void target($ns::$optional foo) {
+  if (foo && *foo) {
+foo->value();
+/*[[access]]*/
+  }
+}
+  )",
+  UnorderedElementsAre(Pair("access", "safe")));
+}
+
+TEST_P(UncheckedOptionalAccessTest, NestedOptionalInOptionalStruct) {
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+struct Foo {
+  $ns::$optional opt;
+};
+
+void target(const $ns::$optional& foo) {
+  if (foo && foo->opt) {
+foo->opt.value();
+/*[[access]]*/
+  }
+}
+  )",
+  UnorderedElementsAre(Pair("access", "safe")));
+
+  // `reset` changes the state.
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+struct Foo {
+  $ns::$optional opt;
+};
+
+void target($ns::$optional& foo) {
+  if (foo && foo->opt) {
+foo->opt.reset();
+foo->opt.value();
+/*[[reset]]*/
+  }
+}
+  )",
+  UnorderedElementsAre(Pair("reset", "unsafe: input.cc:11:9")));
+
+  // `has_value` and `operator*`.
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+struct Foo {
+  $ns::$optional opt;
+};
+
+void target($ns::$optional& foo) {
+  if (foo && foo->opt.has_value()) {
+*foo->opt;
+/*[[other-ops]]*/
+  }
+}
+  )",
+  UnorderedElementsAre(Pair("other-ops", "safe")));
+
+  // `operator->`.
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+struct Foo {
+  $ns::$optional opt;
+};
+
+void target($ns::$optional& foo) {
+  if (foo && foo->opt.has_value()) {
+foo->opt->empty();
+/*[[arrow]]*/
+  }
+}
+  )",
+  UnorderedElementsAre(Pair("arrow", "safe")));
+
+  // Accessing the nested optional in the wrong branch.
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+struct Foo {
+  $ns::$optional opt;
+};
+
+void target($ns::$optional& foo) {
+  if (!foo.has_value()) return;
+  if (!foo->opt.has_value()) {
+foo->opt.value();
+/*[[not-set]]*/
+  }
+}
+  )",
+  UnorderedElementsAre(Pair("not-set", "unsafe: input.cc:11:9")));
+}
+
+TEST_P(UncheckedOptionalAccessTest, NestedOptionalInOptionalStructField) {
+  // Non-standard assignment.
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+struct Foo {
+  $ns::$optional opt;
+};
+
+struct Bar {
+  $ns::$optional member;
+};
+
+Bar createBar();
+
+void target() {
+  Bar bar = createBar();
+  bar.member->opt = "a";
+  /*[[ns-assign]]*/
+}
+  )",
+  UnorderedElementsAre(Pair("ns-assign", "unsafe: input.cc:16:7")));
+
+  // Resetting the nested optional.
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+struct Member {
+  $ns::$optional opt;
+};
+
+struct Foo {
+  $ns::$optional member;
+};
+
+Foo createFoo();
+
+void target() {
+  Foo foo = createFoo();
+  foo.member->opt.reset();
+  /*[[nested-reset]]*/
+}
+  )",
+  UnorderedElementsAre(Pair("nested-reset", "unsafe: input.cc:16:7")));
+}
+
+TEST_P(UncheckedOptionalAccessTest, NestedOptionalInitialization) {
+  // 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.
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+using Foo = $ns::$optional;
+
+void target($ns::$optional 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