[PATCH] D155204: [clang][dataflow] Add `refreshStructValue()`.

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

xazax.hun added inline comments.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:706-711
+/// This function is primarily intended for use by checks that set custom
+/// properties on `StructValue`s to model the state of these values. Such checks
+/// should avoid modifying the properties of an existing `StructValue` because
+/// these changes would be visible to other `Environment`s that share the same
+/// `StructValue`. Instead, call `refreshStructValue()`, then set the properties
+/// on the new `StructValue` that it returns. Typical usage:
----------------
mboehme wrote:
> xazax.hun wrote:
> > I think this is fine for now, but I wonder if we should come up with an API where errors like this cannot happen. One potential way would be to no longer include these properties in the `StructValue`s, but have a separate mapping `StructValue => Properties`. So, one can update the mapping in an environment without any unintended consequences in other environments. 
> > I think this is fine for now, but I wonder if we should come up with an API where errors like this cannot happen.
> 
> Thanks for bringing this up.
> 
> I agree that this needs more improvement in the future. The underlying issue is that `StructValue` isn't really a very useful concept.
> 
> `Value` works great for scalar values, where we do actually treat it as the immutable value that it's intended to be. Attaching properties to values makes a lot of sense in this context: If we're modelling a property of an immutable value, then that property is presumably itself immutable.
> 
> However, the `Value` concept has never worked well for structs / records because, when we mutate fields, we don't update the `StructValue`. We could try to change this, but I don't think this would be worth it because `StructValue` isn't a useful concept in C++ anyway. As the value categories RFC discusses, prvalues of class type are a very niche concept in C++. The only thing you can do with them is to initialize a result object -- you can't otherwise perform any operations on them (i.e. access member variables or call member functions). This has been part of the motivation for reducing their importance as part of the value categories work.
> 
> I think we should continue down that path and, ultimately, maybe eliminate `StructValue` entirely. So while introducing a mapping `StructValue => Properties` would be a good option, I think an even better one would be to start attaching properties to `AggregateStorageLocation`s instead of `StructValue`s. Because `AggregateStorageLocation`s are in essence, mutable, the mapping of `AggregateStorageLocation`s to property values would need to be reflected in the `Environment` in some form. I think a natural way to do this would be to model properties in a similar way to fields: An `AggregateStorageLocation` would have a mapping `PropertyName => StorageLocation`, and the values of the properties would then be tracked through the existing `StorageLocation => Value` mapping in the environment.
> 
> Benefits:
> 
> - As already noted, this makes properties on `AggregateStorageLocation`s very similar to fields, which is what they are typically used to model
> - If a particular analysis needs a storage location for a property, we already have one. For example, UncheckedOptionalAccessModel.cpp currently models the "value" property as a `PointerValue` so that it has a `StorageLocation` that it can use as the return value of `optional::value()`. Under the approach I'm proposing, the storage location would be available from the framework, and the model for a specific analysis wouldn't have to do anything extra.
> - We don't need any additional logic for joins / widening; the existing join / widening logic for the `StorageLocation => Value` mapping would also handle properties.
> 
> So this is where I think we can go in the future, and I agree that `refreshStructValue()` should only be a stopgap as we evolve the framework.
This sounds great, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155204