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

Wed May 4 15:26:57 PDT 2022

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 &Expr,
+                                          StructValue &OptionalVal,
----------------
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