Handle stashing of delayed bugs
By just emitting them immediately, because it does happen in practice, when errors are downgraded to delayed bugs.
We already had one case in `lint.rs` where we handled this at the callsite. This commit changes things so it's handled within `stash_diagnostic` instead, because #121812 identified a second case, and it's possible there are more.
Fixes#121812.
r? ````@oli-obk````
By just emitting them immediately, because it does happen in practice,
when errors are downgraded to delayed bugs.
We already had one case in `lint.rs` where we handled this at the
callsite. This commit changes things so it's handled within
`stash_diagnostic` instead, because #121812 identified a second case,
and it's possible there are more.
Fixes#121812.
change equate for binders to not rely on subtyping
*summary by `@spastorino` and `@lcnr*`
### Context
The following code:
```rust
type One = for<'a> fn(&'a (), &'a ());
type Two = for<'a, 'b> fn(&'a (), &'b ());
mod my_api {
use std::any::Any;
use std::marker::PhantomData;
pub struct Foo<T: 'static> {
a: &'static dyn Any,
_p: PhantomData<*mut T>, // invariant, the type of the `dyn Any`
}
impl<T: 'static> Foo<T> {
pub fn deref(&self) -> &'static T {
match self.a.downcast_ref::<T>() {
None => unsafe { std::hint::unreachable_unchecked() },
Some(a) => a,
}
}
pub fn new(a: T) -> Foo<T> {
Foo::<T> {
a: Box::leak(Box::new(a)),
_p: PhantomData,
}
}
}
}
use my_api::*;
fn main() {
let foo = Foo::<One>::new((|_, _| ()) as One);
foo.deref();
let foo: Foo<Two> = foo;
foo.deref();
}
```
has UB from hitting the `unreachable_unchecked`. This happens because `TypeId::of::<One>()` is not the same as `TypeId::of::<Two>()` despite them being considered the same types by the type checker.
Currently the type checker considers binders to be equal if subtyping succeeds in both directions: `for<'a> T<'a> eq for<'b> U<'b>` holds if `for<'a> exists<'b> T<'b> <: T'<a> AND for<'b> exists<'a> T<'a> <: T<'b>` holds. This results in `for<'a> fn(&'a (), &'a ())` and `for<'a, 'b> fn(&'a (), &'b ())` being equal in the type system.
`TypeId` is computed by looking at the *structure* of a type. Even though these types are semantically equal, they have a different *structure* resulting in them having different `TypeId`. This can break invariants of unsafe code at runtime and is unsound when happening at compile time, e.g. when using const generics.
So as seen in `main`, we can assign a value of type `Foo::<One>` to a binding of type `Foo<Two>` given those are considered the same type but then when we call `deref`, it calls `downcast_ref` that relies on `TypeId` and we would hit the `None` arm as these have different `TypeId`s.
As stated in https://github.com/rust-lang/rust/issues/97156#issuecomment-1879030033, this causes the API of existing crates to be unsound.
## What should we do about this
The same type resulting in different `TypeId`s is a significant footgun, breaking a very reasonable assumptions by authors of unsafe code. It will also be unsound by itself once they are usable in generic contexts with const generics.
There are two options going forward here:
- change how the *structure* of a type is computed before relying on it. i.e. continue considering `for<'a> fn(&'a (), &'a ())` and `for<'a, 'b> fn(&'a (), &'b ())` to be equal, but normalize them to a common representation so that their `TypeId` are also the same.
- change how the semantic equality of binders to match the way we compute the structure of types. i.e. `for<'a> fn(&'a (), &'a ())` and `for<'a, 'b> fn(&'a (), &'b ())` still have different `TypeId`s but are now also considered to not be semantically equal.
---
Advantages of the first approach:
- with the second approach some higher ranked types stop being equal, even though they are subtypes of each other
General thoughts:
- changing the approach in the future will be breaking
- going from first to second may break ordinary type checking, as types which were previously equal are now distinct
- going from second to first may break coherence, because previously disjoint impls overlap as the used types are now equal
- both of these are quite unlikely. This PR did not result in any crater failures, so this should not matter too much
Advantages of the second approach:
- the soundness of the first approach requires more non-local reasoning. We have to make sure that changes to subtyping do not cause the representative computation to diverge from semantic equality
- e.g. we intend to consider higher ranked implied bounds when subtyping to [fix] https://github.com/rust-lang/rust/issues/25860, I don't know how this will interact and don't feel confident making any prediction here.
- computing a representative type is non-trivial and soundness critical, therefore adding complexity to the "core type system"
---
This PR goes with the second approach. A crater run did not result in any regressions. I am personally very hesitant about trying the first approach due to the above reasons. It feels like there are more unknowns when going that route.
### Changing the way we equate binders
Relating bound variables from different depths already results in a universe error in equate. We therefore only need to make sure that there is 1-to-1 correspondence between bound variables when relating binders. This results in concrete types being structurally equal after anonymizing their bound variables.
We implement this by instantiating one of the binder with placeholders and the other with inference variables and then equating the instantiated types. We do so in both directions.
More formally, we change the typing rules as follows:
```
for<'r0, .., 'rn> exists<'l0, .., 'ln> LHS<'l0, .., 'ln> <: RHS<'r0, .., 'rn>
for<'l0, .., 'ln> exists<'r0, .., 'rn> RHS<'r0, .., 'rn> <: LHS<'l0, .., 'ln>
--------------------------------------------------------------------------
for<'l0, .., 'ln> LHS<'l0, .., 'ln> eq for<'r0, .., 'rn> RHS<'r0, .., 'rn>
```
to
```
for<'r0, .., 'rn> exists<'l0, .., 'ln> LHS<'l0, .., 'ln> eq RHS<'r0, .., 'rn>
for<'l0, .., 'ln> exists<'r0, .., 'rn> RHS<'r0, .., 'rn> eq LHS<'l0, .., 'ln>
--------------------------------------------------------------------------
for<'l0, .., 'ln> LHS<'l0, .., 'ln> eq for<'r0, .., 'rn> RHS<'r0, .., 'rn>
```
---
Fixes#97156
r? `@lcnr`
Count stashed errors again
Stashed diagnostics are such a pain. Their "might be emitted, might not" semantics messes with lots of things.
#120828 and #121206 made some big changes to how they work, improving some things, but still leaving some problems, as seen by the issues caused by #121206. This PR aims to fix all of them by restricting them in a way that eliminates the "might be emitted, might not" semantics while still allowing 98% of their benefit. Details in the individual commit logs.
r? `@oli-obk`
Deeply normalize obligations in `refining_impl_trait`
We somewhat awkwardly use semantic comparison when checking the `refining_impl_trait` lint. This relies on us being able to normalize bounds eagerly to avoid cases where an unnormalized alias is not considered equal to a normalized alias. Since `normalize` in the new solver is a noop, let's use `deeply_normalize` instead.
r? lcnr
cc ``@tmandry,`` this should fix your bug lol
Stashed errors used to be counted as errors, but could then be
cancelled, leading to `ErrorGuaranteed` soundness holes. #120828 changed
that, closing the soundness hole. But it introduced other difficulties
because you sometimes have to account for pending stashed errors when
making decisions about whether errors have occured/will occur and it's
easy to overlook these.
This commit aims for a middle ground.
- Stashed errors (not warnings) are counted immediately as emitted
errors, avoiding the possibility of forgetting to consider them.
- The ability to cancel (or downgrade) stashed errors is eliminated, by
disallowing the use of `steal_diagnostic` with errors, and introducing
the more restrictive methods `try_steal_{modify,replace}_and_emit_err`
that can be used instead.
Other things:
- `DiagnosticBuilder::stash` and `DiagCtxt::stash_diagnostic` now both
return `Option<ErrorGuaranteed>`, which enables the removal of two
`delayed_bug` calls and one `Ty::new_error_with_message` call. This is
possible because we store error guarantees in
`DiagCtxt::stashed_diagnostics`.
- Storing the guarantees also saves us having to maintain a counter.
- Calls to the `stashed_err_count` method are no longer necessary
alongside calls to `has_errors`, which is a nice simplification, and
eliminates two more `span_delayed_bug` calls and one FIXME comment.
- Tests are added for three of the four fixed PRs mentioned below.
- `issue-121108.rs`'s output improved slightly, omitting a non-useful
error message.
Fixes#121451.
Fixes#121477.
Fixes#121504.
Fixes#121508.
Diagnostic renaming
Renaming various diagnostic types from `Diagnostic*` to `Diag*`. Part of https://github.com/rust-lang/compiler-team/issues/722. There are more to do but this is enough for one PR.
r? `@davidtwco`
Split rustc_type_ir to avoid rustc_ast from depending on it
unblocks #121576
and resolves a FIXME in `rustc_ast`'s `Cargo.toml`
The new crate is tiny, but it will get bigger in #121576
Delayed bug audit
I went through all the calls to `delayed_bug` and `span_delayed_bug` and found a few places where they could be avoided.
r? `@compiler-errors`
rename 'try' intrinsic to 'catch_unwind'
The intrinsic has nothing to do with `try` blocks, and corresponds to the stable `catch_unwind` function, so this makes a lot more sense IMO.
Also rename Miri's special function while we are at it, to reflect the level of abstraction it works on: it's an unwinding mechanism, on which Rust implements panics.
Fix more #121208 fallout (round 3)
#121208 converted lots of delayed bugs to bugs. Unsurprisingly, there were a few invalid conversion found via fuzzing.
r? `@lcnr`
Prevent cycle in implied predicates computation
Makes #65913 from hang -> fail. I believe fail is the correct state for this test to remain for the long term.
Add newtypes for bool fields/params/return types
Fixed all the cases of this found with some simple searches for `*/ bool` and `bool /*`; probably many more
Don't ICE on anonymous struct in enum variant
Fixes#121446
Computing `adt_def` for the anon struct calls `adt_def` on the parent to find its repr. If the parent is a non-item (e.g. an enum variant) we should have already emitted at least one error, so we just use the repr of the anonymous struct to avoid an ICE.
cc ``@frank-king``
Fix more #121208 fallout
#121208 converted lots of delayed bugs to bugs. Unsurprisingly, there were a few invalid conversion found via fuzzing.
r? `@lcnr`
Add "algebraic" fast-math intrinsics, based on fast-math ops that cannot return poison
Setting all of LLVM's fast-math flags makes our fast-math intrinsics very dangerous, because some inputs are UB. This set of flags permits common algebraic transformations, but according to the [LangRef](https://llvm.org/docs/LangRef.html#fastmath), only the flags `nnan` (no nans) and `ninf` (no infs) can produce poison.
And this uses the algebraic float ops to fix https://github.com/rust-lang/rust/issues/120720
cc `@orlp`
Convert `delayed_bug`s to `bug`s.
I have a suspicion that quite a few delayed bug paths are impossible to reach, so I did an experiment.
I converted every `delayed_bug` to a `bug`, ran the full test suite, then converted back every `bug` that was hit. A surprising number were never hit.
This is too dangerous to merge. Increased coverage (fuzzing or a crater run) would likely hit more cases. But it might be useful for people to look at and think about which paths are genuinely unreachable.
r? `@ghost`
I have a suspicion that quite a few delayed bug paths are impossible to
reach, so I did an experiment.
I converted every `delayed_bug` to a `bug`, ran the full test suite,
then converted back every `bug` that was hit. A surprising number were
never hit.
The next commit will convert some more back, based on human judgment.
