[clang][dataflow] Use smart pointer caching in unchecked optional accessor (#120249)

Part 2 (and final part) following
https://github.com/llvm/llvm-project/pull/120102
Allows users to do things li

[clang][dataflow] Use smart pointer caching in unchecked optional accessor (#120249)

Part 2 (and final part) following
https://github.com/llvm/llvm-project/pull/120102
Allows users to do things like:

```
if (o->x.has_value()) {
((*o).x).value();
}
```
where the `->` and `*` are operator overload calls.

A user could instead extract the nested optional into a local variable
once instead of doing two accessor calls back to back, but currently
they are unsure why the code is flagged.

show more ...

[clang][dataflow] Cache accessors returning pointers in bugprone-unchecked-optional-access (#113922)

Previously, we covered returning refs, or copies of optional, and bools.
Now cover returning poi

[clang][dataflow] Cache accessors returning pointers in bugprone-unchecked-optional-access (#113922)

Previously, we covered returning refs, or copies of optional, and bools.
Now cover returning pointers (to any type).
This is useful for cases like operator-> of smart pointers.
Addresses more of issue llvm#58510

show more ...

[clang][dataflow] Don't clear cached field state if field is const (#113698)

... in the unchecked optional access model.

[clang][dataflow] Cache accessors for bugprone-unchecked-optional-access (#112605)

Treat calls to zero-param const methods as having stable return values
(with a cache) to address issue #58510. The

[clang][dataflow] Cache accessors for bugprone-unchecked-optional-access (#112605)

Treat calls to zero-param const methods as having stable return values
(with a cache) to address issue #58510. The cache is invalidated when
non-const methods are called. This uses the infrastructure from PR
#111006.

For now we cache methods returning:
- ref to optional
- optional by value
- booleans

We can extend that to pointers to optional in a next change.

show more ...

[clang-tidy] Add support for bsl::optional (#101450)

[clang][nullability] Remove `RecordValue`. (#89052)

This class no longer serves any purpose; see also the discussion here:
https://reviews.llvm.org/D155204#inline-1503204

A lot of existing tests in

[clang][nullability] Remove `RecordValue`. (#89052)

This class no longer serves any purpose; see also the discussion here:
https://reviews.llvm.org/D155204#inline-1503204

A lot of existing tests in TransferTest.cpp check for the existence of
`RecordValue`s. Some of these checks are now simply redundant and have
been
removed. In other cases, tests were checking for the existence of a
`RecordValue` as a way of testing whether a record has been initialized.
I have
typically changed these test to instead check whether a field of the
record has
a value.

show more ...

[clang][dataflow] Fix for value constructor in class derived from optional. (#86942)

The constructor `Derived(int)` in the newly added test
`ClassDerivedFromOptionalValueConstructor` is not a templa

[clang][dataflow] Fix for value constructor in class derived from optional. (#86942)

The constructor `Derived(int)` in the newly added test
`ClassDerivedFromOptionalValueConstructor` is not a template, and this
used to
cause an assertion failure in `valueOrConversionHasValue()` because
`F.getTemplateSpecializationArgs()` returns null.

(This is modeled after the `MaybeAlign(Align Value)` constructor, which
similarly causes an assertion failure in the analysis when assigning an
`Align`
to a `MaybeAlign`.)

To fix this, we can simply look at the type of the destination type
which we're
constructing or assigning to (instead of the function template
argument), and
this not only fixes this specific case but actually simplifies the
implementation.

I've added some additional tests for the case of assigning to a nested
optional
because we didn't have coverage for these and I wanted to make sure I
didn't
break anything.

show more ...

[clang][dataflow] Make optional checker work for types derived from optional. (#84138)

`llvm::MaybeAlign` does this, for example.

It's not an option to simply ignore these derived classes because

[clang][dataflow] Make optional checker work for types derived from optional. (#84138)

`llvm::MaybeAlign` does this, for example.

It's not an option to simply ignore these derived classes because they
get cast
back to the optional classes (for example, simply when calling the
optional
member functions), and our transfer functions will then run on those
optional
classes and therefore require them to be properly initialized.

show more ...

[clang][dataflow] Add `Environment::get<>()`. (#76027)

This template function casts the result of `getValue()` or
`getStorageLocation()` to a given subclass of `Value` or
`StorageLocation` (using

[clang][dataflow] Add `Environment::get<>()`. (#76027)

This template function casts the result of `getValue()` or
`getStorageLocation()` to a given subclass of `Value` or
`StorageLocation` (using `cast_or_null`).

It's a common pattern to do something like this:

```cxx
auto *Val = cast_or_null<PointerValue>(Env.getValue(E));
```

This can now be expressed more concisely like this:

```cxx
auto *Val = Env.get<PointerValue>(E);
```

Instead of adding a new method `get()`, I had originally considered
simply adding a template parameter to `getValue()` and
`getStorageLocation()` (with a default argument of `Value` or
`StorageLocation`), but this results in an undesirable repetition at the
callsite, e.g. `getStorageLocation<RecordStorageLocation>(...)`. The
`Value` and `StorageLocation` in the method name adds nothing of value
when the template argument already contains this information, so it
seemed best to shorten the method name to simply `get()`.

show more ...

[clang][dataflow] Add synthetic fields to `RecordStorageLocation` (#73860)

Synthetic fields are intended to model the internal state of a class
(e.g. the value stored in a `std::optional`) without

[clang][dataflow] Add synthetic fields to `RecordStorageLocation` (#73860)

Synthetic fields are intended to model the internal state of a class
(e.g. the value stored in a `std::optional`) without having to depend on
that class's implementation details.

Today, this is typically done with properties on `RecordValue`s, but
these have several drawbacks:

* Care must be taken to call `refreshRecordValue()` before modifying a
property so that the modified property values aren’t seen by other
environments that may have access to the same `RecordValue`.

* Properties aren’t associated with a storage location. If an analysis
needs to associate a location with the value stored in a property (e.g.
to model the reference returned by `std::optional::value()`), it needs
to manually add an indirection using a `PointerValue`. (See for example
the way this is done in UncheckedOptionalAccessModel.cpp, specifically
in `maybeInitializeOptionalValueMember()`.)

* Properties don’t participate in the builtin compare, join, and widen
operations. If an analysis needs to apply these operations to
properties, it needs to override the corresponding methods of
`ValueModel`.

* Longer-term, we plan to eliminate `RecordValue`, as by-value
operations on records aren’t really “a thing” in C++ (see
https://discourse.llvm.org/t/70086#changed-structvalue-api-14). This
would obviously eliminate the ability to set properties on
`RecordValue`s.

To demonstrate the advantages of synthetic fields, this patch converts
UncheckedOptionalAccessModel.cpp to synthetic fields. This greatly
simplifies the implementation of the check.

This PR is pretty big; to make it easier to review, I have broken it
down into a stack of three commits, each of which contains a set of
logically related changes. I considered submitting each of these as a
separate PR, but the commits only really make sense when taken together.

To review, I suggest first looking at the changes in
UncheckedOptionalAccessModel.cpp. This gives a flavor for how the
various API changes work together in the context of an analysis. Then,
review the rest of the changes.

show more ...

[clang][nullability] Use `proves()` and `assume()` instead of deprecated synonyms. (#70297)

[clang][dataflow]Use cast_or_null instead of cast to prevent crash (#68510)

`getStorageLocation` may return `nullptr` and this will produce crash
when use `cast`, use `dyn_cast_or_null` instead. I

[clang][dataflow]Use cast_or_null instead of cast to prevent crash (#68510)

`getStorageLocation` may return `nullptr` and this will produce crash
when use `cast`, use `dyn_cast_or_null` instead. I test it locally using
[FTXUI](https://github.com/ArthurSonzogni/FTXUI) and it may be the cause
of issue [issue](https://github.com/llvm/llvm-project/issues/68412), but
I am not sure.

Co-authored-by: huqizhi <huqizhi@836744285@qq.com>

show more ...

[clang][dataflow] Change `diagnoseFunction` to use `llvm::SmallVector` instead of `std::vector`. (#66014)

The template is agnostic as to the type used by the list, as long as it
is
compatible with `

[clang][dataflow] Change `diagnoseFunction` to use `llvm::SmallVector` instead of `std::vector`. (#66014)

The template is agnostic as to the type used by the list, as long as it
is
compatible with `llvm::move` and `std::back_inserter`. In practice,
we've
encountered analyses which use different types (`llvm::SmallVector` vs
`std::vector`), so it seems preferable to leave this open to the caller.

show more ...

[clang][dataflow] Don't associate prvalue expressions with storage locations.

Instead, map prvalue expressions directly to values in a newly introduced map `Environment::ExprToVal`.

This change int

[clang][dataflow] Don't associate prvalue expressions with storage locations.

Instead, map prvalue expressions directly to values in a newly introduced map `Environment::ExprToVal`.

This change introduces an additional member variable in `Environment` but is an overall win:

- It is more conceptually correctly, since prvalues don't have storage
locations.

- It eliminates complexity from
`Environment::setValue(const Expr &E, Value &Val)`.

- It reduces the amount of data stored in `Environment`: A prvalue now has a
single entry in `ExprToVal` instead of one in `ExprToLoc` and one in
`LocToVal`.

- Not allocating `StorageLocation`s for prvalues additionally reduces memory
usage.

This patch is the last step in the migration to strict handling of value categories (see https://discourse.llvm.org/t/70086 for details). The changes here are almost entirely internal to `Environment`.

The only externally observable change is that when associating a `RecordValue` with the location returned by `Environment::getResultObjectLocation()` for a given expression, callers additionally need to associate the `RecordValue` with the expression themselves.

Reviewed By: xazax.hun

Differential Revision: https://reviews.llvm.org/D158977

show more ...

[clang][dataflow] Rename `AggregateStorageLocation` to `RecordStorageLocation` and `StructValue` to `RecordValue`.

- Both of these constructs are used to represent structs, classes, and unions;
Cl

[clang][dataflow] Rename `AggregateStorageLocation` to `RecordStorageLocation` and `StructValue` to `RecordValue`.

- Both of these constructs are used to represent structs, classes, and unions;
Clang uses the collective term "record" for these.

- The term "aggregate" in `AggregateStorageLocation` implies that, at some
point, the intention may have been to use it also for arrays, but it don't
think it's possible to use it for arrays. Records and arrays are very
different and therefore need to be modeled differently. Records have a fixed
set of named fields, which can have different type; arrays have a variable
number of elements, but they all have the same type.

- Futhermore, "aggregate" has a very specific meaning in C++
(https://en.cppreference.com/w/cpp/language/aggregate_initialization).
Aggregates of class type may not have any user-declared or inherited
constructors, no private or protected non-static data members, no virtual
member functions, and so on, but we use `AggregateStorageLocations` to model all objects of class type.

In addition, for consistency, we also rename the following:

- `getAggregateLoc()` (in `RecordValue`, formerly known as `StructValue`) to
simply `getLoc()`.

- `refreshStructValue()` to `refreshRecordValue()`

We keep the old names around as deprecated synonyms to enable clients to be migrated to the new names.

Reviewed By: ymandel, xazax.hun

Differential Revision: https://reviews.llvm.org/D156788

show more ...

[clang][dataflow] Remove `Strict` suffix from accessors.

For the time being, we're keeping the `Strict` versions around as deprecated synonyms so that clients can be migrated, but these synonyms wil

[clang][dataflow] Remove `Strict` suffix from accessors.

For the time being, we're keeping the `Strict` versions around as deprecated synonyms so that clients can be migrated, but these synonyms will be removed soon.

Depends On D156673

Reviewed By: ymandel, xazax.hun

Differential Revision: https://reviews.llvm.org/D156674

show more ...

[clang][dataflow] Use `Strict` accessors where we weren't using them yet.

This eliminates all uses of the deprecated accessors.

Reviewed By: ymandel, xazax.hun

Differential Revision: https://revie

[clang][dataflow] Use `Strict` accessors where we weren't using them yet.

This eliminates all uses of the deprecated accessors.

Reviewed By: ymandel, xazax.hun

Differential Revision: https://reviews.llvm.org/D156672

show more ...

[clang][dataflow] Reverse course on `getValue()` deprecation.

In the [value categories RFC](https://discourse.llvm.org/t/70086), I proposed that the end state of the migration should be that `getVal

[clang][dataflow] Reverse course on `getValue()` deprecation.

In the [value categories RFC](https://discourse.llvm.org/t/70086), I proposed that the end state of the migration should be that `getValue()` should only be legal to call on prvalues.

As a stepping stone, to allow migrating off existing calls to `getValue()`, I proposed introducing `getValueStrict()`, which would already have the new semantics.

However, I've now reconsidered this. Any expression, whether prvalue or glvalue, has a value, so really there isn't any reason to forbid calling `getValue()` on glvalues. I'm therefore removing the deprecation from `getValue()` and transitioning existing `getValueStrict()` calls back to `getValue()`.

The other "strict" accessors are a different case. `setValueStrict()` should only be called on prvalues because glvalues need to have a storage location associated with them; it doesn't make sense to only set a value for them. And, of course, `getStorageLocationStrict()` and `setStorageLocationStrict()` should obviously only be called on glvalues because prvalues don't have storage locations.

Reviewed By: ymandel, xazax.hun

Differential Revision: https://reviews.llvm.org/D155921

show more ...

[clang-tidy] Update unchecked-optiona-access-check to use convenience function for diagnosing `FunctionDecl`s.

Also changes code in the underlying model to fit the type expected by the convenience f

[clang-tidy] Update unchecked-optiona-access-check to use convenience function for diagnosing `FunctionDecl`s.

Also changes code in the underlying model to fit the type expected by the convenience function.

Differential Revision: https://reviews.llvm.org/D156255

show more ...

[clang-tidy] Add folly::Optional to unchecked-optional-access

The unchecked-optional-access check identifies attempted value
unwrapping without checking if the value exists. These changes extend
tha

[clang-tidy] Add folly::Optional to unchecked-optional-access

The unchecked-optional-access check identifies attempted value
unwrapping without checking if the value exists. These changes extend
that support to checking folly::Optional.

Reviewed By: gribozavr2

Differential Revision: https://reviews.llvm.org/D155890

show more ...

[clang][dataflow] Eliminate duplication between `AggregateStorageLocation` and `StructValue`.

After this change, `StructValue` is just a wrapper for an `AggregateStorageLocation`. For the wider cont

[clang][dataflow] Eliminate duplication between `AggregateStorageLocation` and `StructValue`.

After this change, `StructValue` is just a wrapper for an `AggregateStorageLocation`. For the wider context, see https://discourse.llvm.org/t/70086.

## How to review

- Start by looking at the comments added / changed in Value.h, StorageLocation.h,
and DataflowEnvironment.h. This will give you a good overview of the semantic
changes.

- Look at the corresponding .cpp files that implement the semantic changes.

- Transfer.cpp, TypeErasedDataflowAnalysis.cpp, and RecordOps.cpp show how the
core of the framework is affected by the semantic changes.

- UncheckedOptionalAccessModel.cpp shows how this complex model is affected by
the changes.

- Many of the changes in the rest of the patch are mechanical in nature.

Reviewed By: ymandel, xazax.hun

Differential Revision: https://reviews.llvm.org/D155446

show more ...

[clang][dataflow] Simplify implementation of `transferStdForwardCall()` in optional check.

The argument and return value of `std::forward` is always a reference, so we can simply forward the storage

[clang][dataflow] Simplify implementation of `transferStdForwardCall()` in optional check.

The argument and return value of `std::forward` is always a reference, so we can simply forward the storage location.

Depends On D155075

Reviewed By: ymandel, gribozavr2, xazax.hun

Differential Revision: https://reviews.llvm.org/D155202

show more ...

[dataflow] Remove deprecated BoolValue flow condition accessors

Use the Formula versions instead, now.

Differential Revision: https://reviews.llvm.org/D155152

[clang][dataflow] Various refactorings to UncheckedOptionalAccessModel.

These are intended to ease an upcoming change that will eliminate the duplication between `AggregateStorageLocation` and `Stru

[clang][dataflow] Various refactorings to UncheckedOptionalAccessModel.

These are intended to ease an upcoming change that will eliminate the duplication between `AggregateStorageLocation` and `StructValue` (see https://discourse.llvm.org/t/70086 for details), but many of the changes also have value in their own right.

Depends On D154586

Reviewed By: ymandel, gribozavr2

Differential Revision: https://reviews.llvm.org/D154597

show more ...

[clang][dataflow][NFC] Expand comments on losing values in optional checker.

While working on the ongoing migration to strict handling of value
categories (see https://discourse.llvm.org/t/70086), I

[clang][dataflow][NFC] Expand comments on losing values in optional checker.

While working on the ongoing migration to strict handling of value
categories (see https://discourse.llvm.org/t/70086), I ran into issues related
to losing the value associated with an optional.

This issue is hinted at in the existing comments, but the issue didn't become
sufficiently clear to me from those, so I thought it would be worth capturing
more details, along with ideas for how this issue might be fixed.

Reviewed By: ymandel

Differential Revision: https://reviews.llvm.org/D152369

show more ...