### Problem
`bugprone-unchecked-optional-access` produces a lot of false positives
if type inside of `bsl::optional` or `bdlb::NullableValue` is
**allocator-aware**.
This is a very common pattern, especially due to frequent use of
`bsl::string`.
[Compiler explorer example to showcase false-positives with BDE
library](https://compiler-explorer.com/z/P4zh7KbGx)
### Context
https://github.com/llvm/llvm-project/pull/101450 added support for
analysing `bsl::optional` access patterns.
However, mock `bsl::optional` type has been very simplified for testing
purposes which lead to missing false-positives related to _inheritance_
logic for this type.
[According to this
article](https://bloomberg.github.io/bde/articles/bsl_optional.html#interoperability-between-bsl-optional-and-bdlb-nullablevalue),
there are two ways of inheritance for `bsl::optional` and
`bdlb::NullableValue`:
1. C++17 non-allocator-aware type
` bdlb::NullableValue<T> -> bsl::optional<T> -> std::optional<T>`
2. C++17 **Allocator-Aware**, and pre-C++17
`bdlb::NullableValue<T> -> bsl::optional<T>`
But this is not a full picture :(
In practice, there is an additional layer in the inheritance chain:
`BloombergLP::bslstl::Optional_Base`.
Thus, the actual inheritance structure is:
1. C++17 non-allocator-aware
`bdlb::NullableValue<T> -> bsl::optional<T> ->
BloombergLP::bslstl::Optional_Base<T, false> -> std::optional<T>`
2. C++17 **Allocator-Aware**, and pre-C++17
`bdlb::NullableValue<T> -> bsl::optional<T> ->
BloombergLP::bslstl::Optional_Base<T, true>`
[Source code to show this
inheritance](f8b09a9298/groups/bsl/bslstl/bslstl_optional.h (L1851))
### Root cause
IIUC, because of this inheritance logic, function calls to
`bsl::optional::has_value()` are processed like:
1. `std::optional::has_value()` for non-allocator-aware type.
2. `BloombergLP::bslstl::Optional_Base::has_value()` for allocator-aware
type.
Obviously, similar conversion are true for other common methods like
`.value()`
**This PR tries to solve this issue by improving mocks and adding
`BloombergLP::bslstl::Optional_Base<T>` to list of supported optional
types**
This is important if the first use of a StatusOr (or Status) is in a
conditional statement, we need a stable value for `ok` from outside of
the conditional statement to make sure we don't use a different variable
in every branch.
Reviewers: jvoung, Xazax-hun
Reviewed By: jvoung
Pull Request: https://github.com/llvm/llvm-project/pull/163898
`absl::StatusOr::status` allows extraction of the status associated with
a StatusOr value. That can also be used to check whether the StatusOr
has a value or not.
This makes sure code like this is checked properly:
```cpp
int target(absl::StatusOr<int> SOR) {
if (SOR.status().ok())
return *SOR;
return 0;
}
```
This model implements a dataflow analysis for reporting instances of
unchecked use of absl::StatusOr values. It makes sure that every use
the value of a StatusOr object is dominated by a check that the
StatusOr object is ok.
This is an example of code that will be flagged by the analysis:
```cpp
int f(absl::StatusOr<int> SOR) {
return SOR.value();
}
```
This is an example of code that will not be flagged by the analysis:
```cpp
int f(absl::StatusOr<int> SOR) {
if (SOR.ok())
return SOR.value();
return 0;
}
```
This model has successfully been used by Google for some time now.
This is the initial commit that adds the simplest possible model, that
only models calls to `ok()` and checks for unsafe accesses. I will add
more fidelity to the model in follow up changes.
The test setup is notable in that it has an extra indirection. This is
because we have an internal model that extends the model we intend to
upstream, in order to model special constructs only found in our code
base. The parametrized test allows us (and anyone who chooses to do
this) to make sure our extensions do not break the base functionality.
RFC:
https://discourse.llvm.org/t/rfc-abseil-unchecked-statusor-use-check/87998
https://github.com/llvm/llvm-project/pull/101450 added support for
`BloombergLP::bdlb::NullableValue`.
However, `NullableValue::makeValue` and
`NullableValue::makeValueInplace` have been missed which impacts code
like this:
```cpp
if (opt.isNull()) {
opt.makeValue(42);
}
opt.value(); // triggers false positive warning from `bugprone-unchecked-optional-access`
```
My patch addresses this issue.
[Docs that I used for methods
mocks](https://bloomberg.github.io/bde-resources/doxygen/bde_api_prod/classbdlb_1_1NullableValue.html)
---------
Co-authored-by: Baranov Victor <bar.victor.2002@gmail.com>
This reintroduces `Type.h`, having earlier been renamed to `TypeBase.h`,
as a redirection to `TypeBase.h`, and redirects most users to include
the former instead.
This is a preparatory patch for being able to provide inline definitions
for `Type` methods which would otherwise cause a circular dependency
with `Decl{,CXX}.h`.
Doing these operations into their own NFC patch helps the git rename
detection logic work, preserving the history.
This patch makes clang just a little slower to build (~0.17%), just
because it makes more code indirectly include `DeclCXX.h`.
This is a preparatory patch, to be able to provide inline definitions
for `Type` functions which depend on `Decl{,CXX}.h`. As the latter also
depends on `Type.h`, this would not be possible without some
reorganizing.
Splitting this rename into its own patch allows git to track this as a
rename, and preserve all git history, and not force any code
reformatting.
A later NFC patch will reintroduce `Type.h` as redirection to
`TypeBase.h`, rewriting most places back to directly including `Type.h`
instead of `TypeBase.h`, leaving only a handful of places where this is
necessary.
Then yet a later patch will exploit this by making more stuff inline.
These are identified by misc-include-cleaner. I've filtered out those
that break builds. Also, I'm staying away from llvm-config.h,
config.h, and Compiler.h, which likely cause platform- or
compiler-specific build failures.
Report the range in diagnostics, in addition to the location
in case the range helps disambiguate a little in chained `->`
expressions.
```
b->a->f->x = 1;
^~~~~~~
```
instead of just:
```
b->a->f->x = 1;
^
```
As a followup we should probably also report the location/range
of an `->` if that operator is used. Like:
```
b->a->f->x = 1;
^~
```
Add test for https://github.com/llvm/llvm-project/issues/125589
The crash is actually incidentally fixed by
https://github.com/llvm/llvm-project/pull/128437 since it added a branch
for the reference case and would no longer fall through when the return
type is a reference to a pointer.
Clean up a bit as well:
- make the fallback for early returns more consistent (check if
returning optional and call transfer function for that case)
- check RecordLoc == nullptr in one place
- clean up extra spaces in test
- clean up parameterization in test of `std::` vs `$ns::$`
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.
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
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.
`getDirectCallee()` may return a null pointer if the callee is not a
`FunctionDecl` (for example when using function pointers), this requires
to use `dyn_cast_or_null` instead of `dyn_cast`.
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.
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.
`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.
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()`.
This patch replaces uses of StringRef::{starts,ends}with with
StringRef::{starts,ends}_with for consistency with
std::{string,string_view}::{starts,ends}_with in C++20.
I'm planning to deprecate and eventually remove
StringRef::{starts,ends}with.
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.
`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>
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.
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
- 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
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
