[NLL] Be more permissive when checking access due to Match
Partially addresses #53114. notably, we should now have parity with AST borrowck. Matching on uninitialized values is still forbidden.
* ~~Give fake borrows for match their own `BorrowKind`~~
* ~~Allow borrows with this kind to happen on values that are already mutably borrowed.~~
* ~~Track borrows with this type even behind shared reference dereferences and consider all accesses to be deep when checking for conflicts with this borrow type. See [src/test/ui/issues/issue-27282-mutate-before-diverging-arm-3.rs](cb5c989598 (diff-a2126cd3263a1f5342e2ecd5e699fbc6)) for an example soundness issue this fixes (a case of #27282 that wasn't handled correctly).~~
* Create a new `BorrowKind`: `Shallow` (name can be bike-shed)
* `Shallow` borrows differ from shared borrows in that
* When we check for access we treat them as a `Shallow(Some(_))` read
* When we check for conflicts with them, if the borrow place is a strict prefix of the access place then we don't consider that a conflict.
* For example, a `Shallow` borrow of `x` does not conflict with any access or borrow of `x.0` or `*x`
* Remove the current fake borrow in matches.
* When building matches, we take a `Shallow` borrow of any `Place` that we switch on or bind in a match, and any prefix of those places. (There are some optimizations where we do fewer borrows, but this shouldn't change semantics)
* `match x { &Some(1) => (), _ => (), }` would `Shallow` borrow `x`, `*x` and `(*x as Some).0` (the `*x` borrow is unnecessary, but I'm not sure how easy it would be to remove.)
* Replace the fake discriminant read with a `ReadForMatch`.
* Change ReadForMatch to only check for initializedness (to prevent `let x: !; match x {}`), but not conflicting borrows. It is still considered a use for liveness and `unsafe` checking.
* Give special cased error messages for this kind of borrow.
Table from the above issue after this PR
| Thing | AST | MIR | Want | Example |
| --- | --- | --- | --- |---|
| `let _ = <unsafe-field>` | 💚 | 💚 | ❌ | [playground](https://play.rust-lang.org/?gist=bb7843e42fa5318c1043d04bd72abfe4&version=nightly&mode=debug&edition=2015) |
| `match <unsafe_field> { _ => () }` | ❌ | ❌ | ❌ | [playground](https://play.rust-lang.org/?gist=3e3af05fbf1fae28fab2aaf9412fb2ea&version=nightly&mode=debug&edition=2015) |
| `let _ = <moved>` | 💚 | 💚 | 💚 | [playground](https://play.rust-lang.org/?gist=91a6efde8288558e584aaeee0a50558b&version=nightly&mode=debug&edition=2015) |
| `match <moved> { _ => () }` | ❌ | ❌ | 💚 | [playground](https://play.rust-lang.org/?gist=804f8185040b2fe131f2c4a64b3048ca&version=nightly&mode=debug&edition=2015) |
| `let _ = <borrowed>` | 💚 | 💚 | 💚 | [playground](https://play.rust-lang.org/?gist=0e487c2893b89cb772ec2f2b7c5da876&version=nightly&mode=debug&edition=2015) |
| `match <borrowed> { _ => () }` | 💚 | 💚 | 💚 | [playground](https://play.rust-lang.org/?gist=0e487c2893b89cb772ec2f2b7c5da876&version=nightly&mode=debug&edition=2015) |
r? @nikomatsakis
Previously, we would split the drop access into multiple checks for each
field of a struct/tuple/closure and through `Box` dereferences. This
changes this to check if the borrow is accessed by the drop in
places_conflict.
This also allows us to handle enums in a simpler way, since we don't
have to construct any new places.
When dropping a self-borrowing struct we shouldn't add a "values in a
scope are dropped in the opposite order they are defined" message,
since there is only one value being dropped.
move CTFE engine snapshot state out of miri engine into CTFE machine instance
It still lives in the `interpret` module as it needs access to all sorts of private stuff. Also rename a thing to make @eddyb happy :D
The goal was not to change any behavior.
Error now correctly checks whether the borrow that does not live
long enough is being returned before annotating the error with the
arguments and return type from the signature - as this would not be
relevant if the borrow was not being returned.
Enhances annotation logic to properly consider named lifetimes where
lifetime elision rules that were previously implemented would not apply.
Further, adds new help and note messages to diagnostics and highlights
only lifetime when dealing with named lifetimes.
This error can only occur within a function when a borrow of data owned
within the function is returned; and when there are arguments that could
have been returned instead. Therefore, it is always applicable to add a
specific note that links to the relevant rust documentation about
dangling references.
For cases where there are references in the parameters and in the the
outputs that do not match, and where no closures are involved, this
commit introduces an improved error that mentions (or synthesizes)
a name for the regions involved to better illustrate why the borrow
does not live long enough.
Previously, region naming would always highlight the source of the
region name it found. Now, region naming returns the name as part
of a larger structure that encodes the source of the region naming
such that a region name can be optionally added to the diagnostic.
Previously, explain_borrow would emit an error with the explanation of
the a borrow. Now, it returns a enum with what the explanation for the
borrow is and any relevant spans or information such that the calling
code can choose to emit the same note/suggestion as before by calling
the emit method on the new enum.
Report when borrow could cause `&mut` aliasing during Drop
We were already issuing an error for the cases where this cropped up, so this is not fixing any soundness holes. The previous diagnostic just wasn't accurately describing the problem in the user's code.
Fix#52059
[NLL] Record more infomation on free region constraints in typeck
Changes:
* Makes the span of the MIR return place point to the return type
* Don't try to use a path to a type alias as a path to the adt it aliases (fixes an ICE)
* Don't claim that `self` is declared outside of the function. [see this test](f2995d5b1a (diff-0c9e6b1b204f42129b481df9ce459d44))
* Remove boring/interesting distinction and instead add a `ConstraintCategory` to the constraint.
* Add categories for implicit `Sized` and `Copy` requirements, for closure bounds, for user type annotations and `impl Trait`.
* Don't use the span of the first statement for Locations::All bounds (even if it happens to work on the tests we have)
Future work:
* Fine tuning the heuristic used to choose the place the report the error.
* Reporting multiple places (behind a flag)
* Better closure bounds reporting. This probably requires some discussion.
r? @nikomatsakis
Implement `MaybeUninit`
This PR:
- Adds `MaybeUninit` (see #53491) to `{core,std}::mem`.
- Makes `mem::{uninitialized,zeroed}` panic when they are used to instantiate an uninhabited type.
- Does *not* deprecate `mem::{uninitialized,zeroed}` just yet. As per https://github.com/rust-lang/rust/issues/53491#issuecomment-414147666, we should not deprecate them until `MaybeUninit` is stabilized.
- It replaces uses of `mem::{uninitialized,zeroed}` in core and alloc with `MaybeUninit`.
There are still several instances of `mem::{uninitialized,zeroed}` in `std` that *this* PR doesn't address.
r? @RalfJung
cc @eddyb you may want to look at the new panicking logic
NLL: disallow creation of immediately unusable variables
Fix#53695
Original description follows
----
This WIP PR is for discussing the impact of fixing #53695 by injecting a fake read in let patterns.
(Travis will fail, at least the `mir-opt` suite is failing in its current state)
Improve handling of type bounds in `bit_set.rs`.
Currently, `BitSet` doesn't actually know its own domain size; it just
knows how many words it contains. We can make it better.
Remove usages of span_suggestion without Applicability
Use `Applicability::Unspecified` for all of them instead.
Shall deprecations for the non-`_with_applicability` functions be added?
Shall clippy be addressed somehow?
r? @estebank
Currently, `BitSet` doesn't actually know its own domain size; it just
knows how many words it contains. To improve things, this commit makes
the following changes.
- It changes `BitSet` and `SparseBitSet` to store their own domain size,
and do more precise bounds and same-size checks with it. It also
changes the signature of `BitSet::to_string()` (and puts it within
`impl ToString`) now that the domain size need not be passed in from
outside.
- It uses `derive(RustcDecodable, RustcEncodable)` for `BitSet`. This
required adding code to handle `PhantomData` in `libserialize`.
- As a result, it removes the domain size from `HybridBitSet`, making a
lot of that code nicer.
- Both set_up_to() and clear_above() were overly general, working with
arbitrary sizes when they are only needed for the domain size. The
commit removes the former, degeneralizes the latter, and removes the
(overly general) tests.
- Changes `GrowableBitSet::grow()` to `ensure()`, fixing a bug where a
(1-based) domain size was confused with a (0-based) element index.
- Changes `BitMatrix` to store its row count, and do more precise bounds
checks with it.
- Changes `ty_params` in `select.rs` from a `BitSet` to a
`GrowableBitSet` because it repeatedly failed the new, more precise
bounds checks. (Changing the type was simpler than computing an
accurate domain size.)
- Various other minor improvements.
The MIR/NLL type checker is in a much better position to classify
constraints and already has to classify into boring and interesting.
Adds spans to Locations::All for error reporting
Adds more constraint categories
error[E0106]: missing lifetime specifier
--> /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md:11424:23
|
9 | fn demo(s: &mut S) -> &mut String { let p = &mut *(*s).data; p }
| ^ expected lifetime parameter
|
= help: this function's return type contains a borrowed value, but the signature does not say which one of `s`'s 2 lifetimes it is borrowed from
Use `HybridBitSet` in `SparseBitMatrix`.
This fixes most of the remaining NLL memory regression.
r? @pnkfelix, because you reviewed #54286.
cc @nikomatsakis, because NLL
cc @Mark-Simulacrum, because this removes `array_vec.rs`
cc @lqd, because this massively improves `unic-ucd-name`, and probably other public crates
By introducing a new map that tracks the errors reported and the
`Place`s that spawned those errors against the move out that the error
was referring to, we are able to silence duplicate errors by emitting
only the error which corresponds to the most specific `Place` (that which
other `Place`s which reported errors are prefixes of).
This generally is an improvement, however there is a case -
`liveness-move-in-while` - where the output regresses.
This requires adding a few extra methods to `HybridBitSet`. (These are
tested in a new unit test.)
This commit reduces the `max-rss` for `nll-check` builds of `html5ever`
by 46%, `ucd` by 45%, `clap-rs` by 23%, `inflate` by 14%. And the
results for the `unic-ucd-name` crate are even more impressive: a 21%
reduction in instructions, a 60% reduction in wall-time, a 96%
reduction in `max-rss`, and a 97% reduction in faults!
Fixes#52028.
Merge `bitvec.rs` and `indexed_set.rs`
Because it's not good to have two separate implementations. Also, I will combine the best parts of each to improve NLL memory usage on some benchmarks significantly.
In particular:
1. Extend `WriteKind::StorageDeadOrDrop` with state to track whether
we are running a destructor or just freeing backing storage. (As
part of this, when we drop a Box<..<Box<T>..> where `T` does not
need drop, we now signal that the drop of `T` is a kind of storage
dead rather than a drop.)
2. When reporting that a value does not live long enough, check if
we're doing an "interesting" drop, i.e. we aren't just trivally
freeing the borrowed state, but rather a user-defined dtor will
run and potentially require exclusive aces to the borrowed state.
3. Added a new diagnosic to describe the scenario here.
`BitwiseOperator` is an unnecessarily low-level thing. This commit
replaces it with `BitSetOperator`, which works on `BitSet`s instead of
words. Within `bit_set.rs`, the commit eliminates `Intersect`, `Union`,
and `Subtract` by instead passing a function to `bitwise()`.
Currently we have two files implementing bitsets (and 2D bit matrices).
This commit combines them into one, taking the best features from each.
This involves renaming a lot of things. The high level changes are as
follows.
- bitvec.rs --> bit_set.rs
- indexed_set.rs --> (removed)
- BitArray + IdxSet --> BitSet (merged, see below)
- BitVector --> GrowableBitSet
- {,Sparse,Hybrid}IdxSet --> {,Sparse,Hybrid}BitSet
- BitMatrix --> BitMatrix
- SparseBitMatrix --> SparseBitMatrix
The changes within the bitset types themselves are as follows.
```
OLD OLD NEW
BitArray<C> IdxSet<T> BitSet<T>
-------- ------ ------
grow - grow
new - (remove)
new_empty new_empty new_empty
new_filled new_filled new_filled
- to_hybrid to_hybrid
clear clear clear
set_up_to set_up_to set_up_to
clear_above - clear_above
count - count
contains(T) contains(&T) contains(T)
contains_all - superset
is_empty - is_empty
insert(T) add(&T) insert(T)
insert_all - insert_all()
remove(T) remove(&T) remove(T)
words words words
words_mut words_mut words_mut
- overwrite overwrite
merge union union
- subtract subtract
- intersect intersect
iter iter iter
```
In general, when choosing names I went with:
- names that are more obvious (e.g. `BitSet` over `IdxSet`).
- names that are more like the Rust libraries (e.g. `T` over `C`,
`insert` over `add`);
- names that are more set-like (e.g. `union` over `merge`, `superset`
over `contains_all`, `domain_size` over `num_bits`).
Also, using `T` for index arguments seems more sensible than `&T` --
even though the latter is standard in Rust collection types -- because
indices are always copyable. It also results in fewer `&` and `*`
sigils in practice.
Note it requires MIR-borrowck to be enabled to actually do anything.
Note also that it implicitly turns off our AST-based check for
mutation in guards.
Make rustc::middle::region::Scope's fields public
This PR makes the following changes to `rustc::middle::region::Scope`:
- [x] Makes `region::Scope`'s fields public
- [x] Removes the `impl Scope` block with constructors (as per [this comment](https://github.com/rust-lang/rust/pull/54032#discussion_r216618208))
- [x] Updates call sites throughout the compiler
Closes#54122.
miri engine: keep around some information for dead allocations
We use it to test if a dangling ptr is aligned and non-NULL. This makes some code pass that should pass (writing a ZST to a properly aligned dangling pointer), and makes some code fail that should fail (writing a ZST to a pointer obtained via pointer arithmetic from a real location, but ouf-of-bounds -- that pointer could be NULL, so we cannot allow writing to it).
CTFE does not allow these operations; tests are added to miri with https://github.com/solson/miri/pull/453.
De-overlap the lifetimes of `flow_inits` and `flow_{un,ever_}inits`.
This reduces `max-rss` for an `nll-check` build by 27% for `keccak`, and
by 8% for `inflate`.
r? @nikomatsakis
use structured suggestion for "missing mut" label
Fixes#54133 for both NLL and non-NLL.
r? @estebank
I'm not super happy with the existing wording here, since it's now a suggestion. I wonder if the message would work better as something like "help: make binding mutable: `mut foo`"?
Also, are the `HELP` and `SUGGESTION` comments necessary?
[NLL] Suggest let binding
Closes#49821
Also adds an alternative to `explain_why_borrow_contains_point` that allows changing error messages based on the reason that will be given. This will also be useful for #51026, #51169 and maybe further changes to does not live long enough messages.
miri loop detector hashing
* fix enum hashing to also consider discriminant
* do not hash extra machine state
* standalone miri is not interested in loop detection, so let it opt-out
In the future I think we want to move the hashing logic out of the miri engine, this is CTFE-only.
r? @oli-obk
This requires the following changes.
- It moves parts of bitslice.rs into bitvec.rs: `bitwise()`,
`BitwiseOperator`, `bits_to_string()`.
- It changes `IdxSet` to just be a wrapper around `BitArray`.
- It changes `BitArray` and `BitVec` to use `usize` words instead of
`u128` words. (`BitSlice` and `IdxSet` already use `usize`.) Local
profiling showed `usize` was better.
- It moves some operations from `IdxSet` into `BitArray`:
`new_filled()`, `clear()`, `set_up_to()`, `trim_to()` (renamed
`clear_above()`), `words()` and `words_mut()`, `encode()` and
`decode(). The `IdxSet` operations now just call the `BitArray`
operations.
- It replaces `BitArray`'s iterator implementation with `IdxSet`'s,
because the latter is more concise. It also removes the buggy
`size_hint` function from `BitArray`'s iterator, which counted the
number of *words* rather than the number of *bits*. `IdxSet`'s
iterator is now just a thin wrapper around `BitArray`'s iterator.
- It moves some unit tests from `indexed_set.rs` to `bitvec.rs`.
