Rollup of 8 pull requests #137406
Merged
Rollup of 8 pull requests #137406
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously the location of the divide-by-zero error condition would be attributed to the code in the rust standard library, eg: thread 'main' panicked at /home/user/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/time.rs:1172:31: divide by zero error when dividing duration by scalar With #[track_caller] the error is correctly attributed to the callee.
When a `?` operation requires an `Into` conversion with additional bounds (like having a concrete error but wanting to convert to a trait object), we handle it speficically and provide the same kind of information we give other `?` related errors.
```
error[E0277]: `?` couldn't convert the error: `E: std::error::Error` is not satisfied
--> $DIR/bad-question-mark-on-trait-object.rs:5:13
|
LL | fn foo() -> Result<(), Box<dyn std::error::Error>> {
| -------------------------------------- required `E: std::error::Error` because of this
LL | Ok(bar()?)
| ^ the trait `std::error::Error` is not implemented for `E`
|
= note: the question mark operation (`?`) implicitly performs a conversion on the error value using the `From` trait
= note: required for `Box<dyn std::error::Error>` to implement `From<E>`
```
Avoid talking about `FromResidual` when other more relevant information is being given, particularly from `rust_on_unimplemented`.
Instead of only having `--src-base` and `src_base` which *actually* refers to the directory containing the test suite and not the sources root. More importantly, kill off `find_rust_src_root` when we can simply pass that info from bootstrap.
```
error[E0277]: `?` couldn't convert the error: `E: std::error::Error` is not satisfied
--> $DIR/bad-question-mark-on-trait-object.rs:7:13
|
LL | fn foo() -> Result<(), Box<dyn std::error::Error>> {
| -------------------------------------- required `E: std::error::Error` because of this
LL | Ok(bar()?)
| -----^ the trait `std::error::Error` is not implemented for `E`
| |
| this has type `Result<_, E>`
|
note: `E` needs to implement `std::error::Error`
--> $DIR/bad-question-mark-on-trait-object.rs:1:1
|
LL | struct E;
| ^^^^^^^^
= note: the question mark operation (`?`) implicitly performs a conversion on the error value using the `From` trait
= note: required for `Box<dyn std::error::Error>` to implement `From<E>`
error[E0277]: `?` couldn't convert the error to `X`
--> $DIR/bad-question-mark-on-trait-object.rs:18:13
|
LL | fn bat() -> Result<(), X> {
| ------------- expected `X` because of this
LL | Ok(bar()?)
| -----^ the trait `From<E>` is not implemented for `X`
| |
| this can't be annotated with `?` because it has type `Result<_, E>`
|
note: `X` needs to implement `From<E>`
--> $DIR/bad-question-mark-on-trait-object.rs:4:1
|
LL | struct X;
| ^^^^^^^^
note: alternatively, `E` needs to implement `Into<X>`
--> $DIR/bad-question-mark-on-trait-object.rs:1:1
|
LL | struct E;
| ^^^^^^^^
= note: the question mark operation (`?`) implicitly performs a conversion on the error value using the `From` trait
```
Do not deduplicate list of associated types provided by dyn principal
## Background
The way that we handle a dyn trait type's projection bounds is very *structural* today. A dyn trait is represented as a list of `PolyExistentialPredicate`s, which in most cases will be a principal trait (like `Iterator`) and a list of projections (like `Item = u32`). Importantly, the list of projections comes from user-written associated type bounds on the type *and* from elaborating the projections from the principal's supertraits.
For example, given a set of traits like:
```rust
trait Foo<T> {
type Assoc;
}
trait Bar<A, B>: Foo<A, Assoc = A> + Foo<B, Assoc = B> {}
```
For the type `dyn Bar<i32, u32>`, the list of projections will be something like `[Foo<i32>::Assoc = i32, Foo<u32>::Assoc = u32]`. We deduplicate these projections when they're identical, so for `dyn Bar<(), ()>` would be something like `[Foo<()>::Assoc = ()]`.
## Shortcomings 1: inference
We face problems when we begin to mix this structural notion of projection bounds with inference and associated type normalization. For example, let's try calling a generic function that takes `dyn Bar<A, B>` with a value of type `dyn Bar<(), ()>`:
```rust
trait Foo<T> {
type Assoc;
}
trait Bar<A, B>: Foo<A, Assoc = A> + Foo<B, Assoc = B> {}
fn call_bar<A, B>(_: &dyn Bar<A, B>) {}
fn test(x: &dyn Bar<(), ()>) {
call_bar(x);
// ^ ERROR mismatched types
}
```
```
error[E0308]: mismatched types
--> /home/mgx/test.rs:10:14
|
10 | call_bar(x);
| -------- ^ expected trait `Bar<_, _>`, found trait `Bar<(), ()>`
```
What's going on here? Well, when calling `call_bar`, the generic signature `&dyn Bar<?A, ?B>` does not unify with `&dyn Bar<(), ()>` because the list of projections differ -- `[Foo<?A>::Assoc = ?A, Foo<?B>::Assoc = ?B]` vs `[Foo<()>::Assoc = ()]`.
A simple solution to this may be to unify the principal traits first, then attempt to deduplicate them after inference. In this case, if we constrain `?A = ?B = ()`, then we would be able to deduplicate those projections in the first list.
However, this idea is still pretty fragile, and it's not a complete solution.
## Shortcomings 2: normalization
Consider a slightly modified example:
```rust
//@ compile-flags: -Znext-solver
trait Mirror {
type Assoc;
}
impl<T> Mirror for T {
type Assoc = T;
}
fn call_bar(_: &dyn Bar<(), <() as Mirror>::Assoc>) {}
fn test(x: &dyn Bar<(), ()>) {
call_bar(x);
}
```
This fails in the new solver. In this example, we try to unify `dyn Bar<(), ()>` and `dyn Bar<(), <() as Mirror>::Assoc>`. We are faced with the same problem even though there are no inference variables, and making this work relies on eagerly and deeply normalizing all projections so that they can be structurally deduplicated.
This is incompatible with how we handle associated types in the new trait solver, and while we could perhaps support it with some major gymnastics in the new solver, it suggests more fundamental shortcomings with how we deal with projection bounds in the new solver.
## Shortcomings 3: redundant projections
Consider a final example:
```rust
trait Foo {
type Assoc;
}
trait Bar: Foo<Assoc = ()> {}
fn call_bar1(_: &dyn Bar) {}
fn call_bar2(_: &dyn Bar<Assoc = ()>) {}
fn main() {
let x: &dyn Bar<Assoc = _> = todo!();
call_bar1(x);
//~^ ERROR mismatched types
call_bar2(x);
//~^ ERROR mismatched types
}
```
In this case, we have a user-written associated type bound (`Assoc = _`) which overlaps the bound that comes from the supertrait projection of `Bar` (namely, `Foo<Assoc = ()>`). In a similar way to the two examples above, this causes us to have a projection list mismatch that the compiler is not able to deduplicate.
## Solution
### Do not deduplicate after elaborating projections when lowering `dyn` types
The root cause of this issue has to do with mismatches of the deduplicated projection list before and after substitution or inference. This PR aims to avoid these issues by *never* deduplicating the projection list after elaborating the list of projections from the *identity* substituted principal trait ref.
For example,
```rust
trait Foo<T> {
type Assoc;
}
trait Bar<A, B>: Foo<A, Assoc = A> + Foo<B, Assoc = B> {}
```
When computing the projections for `dyn Bar<(), ()>`, before this PR we'd elaborate `Bar<(), ()>` to find a (deduplicated) projection list of `[Foo<()>::Assoc = ()]`.
After this PR, we take the principal trait and use its *identity* substitutions `Bar<A, B>` during elaboration, giving us projections `[Foo<A>::Assoc = A, Foo<B>::Assoc = B]`. Only after this elaboration do we substitute `A = (), B = ()` to get `[Foo<()>::Assoc = (), Foo<()>::Assoc = ()]`. This allows the type to be unified with the projections for `dyn Bar<?A, ?B>`, which are `[Foo<?A>::Assoc = ?A, Foo<?B>::Assoc = ?B]`.
This helps us avoid shorcomings 1 noted above.
### Do not deduplicate projections when relating `dyn` types
Similarly, we also do not call deduplicate when relating dyn types. This means that the list of projections does not differ depending on if the type has been normalized or not, which should avoid shortcomings 2 noted above.
Following from the example above, when relating projection lists like `[Foo<()>::Assoc = (), Foo<()>::Assoc = ()]` and `[Foo<?A>::Assoc = ?A, Foo<?B>::Assoc = ?B]`, the latter won't be deduplicated to a list of length 1 which would immediately fail to relate to the latter which is a list of length 2.
### Implement proper precedence between supertrait and user-written projection bounds when lowering `dyn` types
```rust
trait Foo {
type Assoc;
}
trait Bar: Foo<Assoc = ()> {}
```
Given a type like `dyn Foo<Assoc = _>`, we used to previously include *both* the supertrait and user-written associated type bounds in the projection list, giving us `[Foo::Assoc = (), Foo::Assoc = _]`. This would never unify with `dyn Foo`. However, this PR implements a strategy which overwrites the supertrait associated type bound with the one provided by the user, giving us a projection list of `[Foo::Assoc = _]`.
Why is this OK? Well, if a user wrote an associated type bound that is unsatisfiable (e.g. `dyn Bar<Assoc = i32>`) then the dyn type would never implement `Bar` or `Foo` anyways. If the user wrote something that is either structurally equal or equal modulo normalization to the supertrait bound, then it should be unaffected. And if the user wrote something that needs inference guidance (e.g. `dyn Bar<Assoc = _>`), then it'll be constrained when proving `dyn Bar<Assoc = _>: Bar`.
Importantly, this differs from the strategy in rust-lang#133397, which preferred the *supertrait* bound and ignored the user-written bound. While that's also theoretically justifiable in its own way, it does lead to code which does not (and probably should not) compile either today or after this PR, like:
```rust
trait IteratorOfUnit: Iterator<Item = ()> {}
impl<T> IteratorOfUnit for T where T: Iterator<Item = ()> {}
fn main() {
let iter = [()].into_iter();
let iter: &dyn IteratorOfUnit<Item = i32> = &iter;
}
```
### Conclusion
This is a far less invasive change compared to rust-lang#133397, and doesn't necessarily necessitate the addition of new lints or any breakage of existing code. While we could (and possibly should) eventually introduce lints to warn users of redundant or mismatched associated type bounds, we don't *need* to do so as part of fixing this unsoundness, which leads me to believe this is a much safer solution.
[`compiletest`-related cleanups 3/7] Make the distinction between sources root vs test suite sources root in compiletest less confusing Reference for overall changes: rust-lang#136437 Part **3** of **7** of the *`compiletest`-related cleanups* PR series. ### Summary - Remove `--src-base` compiletest in favor of new flags `--src-root` and `--src-test-suite-root` which more accurately conveys the intent. `--src-base` previously actually meant `--src-test-suite-root` and has caused multiple confusions. - Use `--src-root` to have bootstrap directly feed source root path to compiletest, instead of doing a hacky directory parent search heuristic (`find_rust_src_root`) that somehow returns an `Option<PathBuf>`. ### Review advice Best reviewed commit-by-commit. r? bootstrap
…nonical, r=lcnr Make sure we don't overrun the stack in canonicalizer r? lcnr Addresses rust-lang/trait-system-refactor-initiative#160
Remove `lifetime_capture_rules_2024` feature Just use edition 2024 instead
…-duration-div, r=jhpratt Add #[track_caller] to Duration Div impl Previously the location of the divide-by-zero error condition would be attributed to the code in the rust standard library, eg: thread 'main' panicked at /home/user/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/time.rs:1172:31: divide by zero error when dividing duration by scalar With #[track_caller] the error is correctly attributed to the callee.
…li-obk
Tweak E0277 when predicate comes indirectly from ?
When a `?` operation requires an `Into` conversion with additional bounds (like having a concrete error but wanting to convert to a trait object), we handle it speficically and provide the same kind of information we give other `?` related errors.
```
error[E0277]: `?` couldn't convert the error: `E: std::error::Error` is not satisfied
--> $DIR/bad-question-mark-on-trait-object.rs:7:13
|
LL | fn foo() -> Result<(), Box<dyn std::error::Error>> {
| -------------------------------------- required `E: std::error::Error` because of this
LL | Ok(bar()?)
| -----^ the trait `std::error::Error` is not implemented for `E`
| |
| this has type `Result<_, E>`
|
note: `E` needs to implement `std::error::Error`
--> $DIR/bad-question-mark-on-trait-object.rs:1:1
|
LL | struct E;
| ^^^^^^^^
= note: the question mark operation (`?`) implicitly performs a conversion on the error value using the `From` trait
= note: required for `Box<dyn std::error::Error>` to implement `From<E>`
```
Avoid talking about `FromResidual` when other more relevant information is being given, particularly from `rust_on_unimplemented`.
Fix rust-lang#137238.
-----
CC rust-lang#137232, which was a smaller step related to this.
…ked-field, r=oli-obk Ignore fake borrows for packed field check We should not emit unaligned packed field reference errors for the fake borrows that we generate during match lowering. These fake borrows are there to ensure in *borrow-checking* that we don't modify the value being matched (which is why this only occurs when there's a match guard, in this case `if true`), but they are removed after the MIR is processed by `CleanupPostBorrowck`, since they're really just there to cause borrowck errors if necessary. I modified `PlaceContext::is_borrow` since that's used by the packed field check: https://github.com/rust-lang/rust/blob/17c1c329a5512d718b67ef6797538b154016cd34/compiler/rustc_mir_transform/src/check_packed_ref.rs#L40 It's only used in one other place, in the SROA optimization (by which fake borrows are removed, so it doesn't matter): https://github.com/rust-lang/rust/blob/17c1c329a5512d718b67ef6797538b154016cd34/compiler/rustc_mir_dataflow/src/value_analysis.rs#L922 Fixes rust-lang#137250
…ler-errors fix ICE in layout computation with unnormalizable const The first commit reverts half of 7a667d2, where I removed a case from `layout_of` for handling non-generic unevaluated consts in array length, that I incorrectly assumed to be unreachable. This can actually happen with the combination of `feature(generic_const_exprs)` and `feature(trivial_bounds)`, because GCE makes anon consts inherit their parent's predicates and with an impossible predicate like `u8: A` it's possible to have an array whose length is an associated const like `<u8 as A>::B` that is not generic, but also can't be normalized: ```rust #![feature(generic_const_exprs)] #![feature(trivial_bounds)] trait A { const B: usize; } // With GCE + trivial bounds this definition is not a compile error. // Computing the layout of this type shouldn't ICE. struct S([u8; <u8 as A>::B]) where u8: A; ``` --- The first commit also incidentally fixes rust-lang#137308, which also managed to get an unnormalizable assoc const into an array length: ```rust trait A { const B: usize; } impl<C: ?Sized> A for u8 { //~ ERROR: the type parameter `C` is not constrained const B: usize = 42; } // Computing the layout of this type shouldn't ICE, even with the compile error above. struct S([u8; <u8 as A>::B]); ``` This happens, because we bail out from `codegen_select_candidate` with an error if the selected impl has unconstrained params to avoid leaking infer vars out of a query. `Instance::try_resolve` will then return `Ok(None)`, which for assoc consts roughly means "this const can't be evaluated in a generic context" and is treated as such: https://github.com/rust-lang/rust/blob/71e06b9c59d6af50fdc55aed75620493d29baf98/compiler/rustc_middle/src/mir/interpret/queries.rs#L84 (and this can ICE if the const isn't generic: rust-lang#135617). However, here `<u8 as A>::B` is definitely not "too generic" and also not unresolvable due to an unsatisfiable `u8: A` bound, so I've included the second commit to change the result of `Instance::try_resolve` from `Ok(None)` to `Err(ErrorGuaranteed)` when resolving an assoc item to an impl with unconstrained generic params. This has the effect that `<u8 as A>::B` will now be normalized to `ConstKind::Error` in the example above. This properly fixes rust-lang#137308, by no longer treating `<u8 as A>::B` as unresolvable even though it clearly has a unique impl that it resolves to. It also has the effect of changing the layout error from `Unknown` ("the type may be valid but has no sensible layout") to `ReferencesError` ("a non-layout error is reported elsewhere") which seems more appropriate. r? ```@compiler-errors```
rustbot
added
A-compiletest
|
@bors r+ rollup=never p=5 |
bors
added
S-waiting-on-bors
rust-highfive
added a commit
to rust-lang-nursery/rust-toolstate
that referenced
this pull request
Feb 22, 2025
Tested on commit rust-lang/rust@b87eda7. Direct link to PR: <rust-lang/rust#137406> 💔 edition-guide on windows: test-pass → test-fail (cc @ehuss). 💔 edition-guide on linux: test-pass → test-fail (cc @ehuss).
|
☀️ Test successful - checks-actions |
|
📌 Perf builds for each rolled up PR:
previous master: dc37ff82e8 In the case of a perf regression, run the following command for each PR you suspect might be the cause: |
This was referenced Feb 22, 2025
|
Finished benchmarking commit (b87eda7): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary 1.5%, secondary -1.7%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary 3.3%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 772.814s -> 771.719s (-0.14%) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Successful merges:
compiletest-related cleanups 3/7] Make the distinction between sources root vs test suite sources root in compiletest less confusing #136474 ([compiletest-related cleanups 3/7] Make the distinction between sources root vs test suite sources root in compiletest less confusing)lifetime_capture_rules_2024feature #136787 (Removelifetime_capture_rules_2024feature)r? @ghost
@rustbot modify labels: rollup
Create a similar rollup