Rollup of 7 pull requests #128213
Merged
Rollup of 7 pull requests #128213
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
`use` declarations will be reformatted in rust-lang#125443. Very rarely, there is a desire to force a group of `use` declarations together in a way that auto-formatting will break up. E.g. when you want a single comment to apply to a group. rust-lang#126776 dealt with all of these in the codebase, ensuring that no comments intended for multiple `use` declarations would end up in the wrong place. But some people were unhappy with it. This commit uses `#[rustfmt::skip]` to create these custom `use` groups in an idiomatic way for a few of the cases changed in rust-lang#126776. This works because rustfmt treats any `use` item annotated with `#[rustfmt::skip]` as a barrier and won't reorder other `use` items around it.
The test mentioned by this comment was deleted long ago by <rust-lang#80290>.
…y-unsoundness, r=lcnr Fix supertrait associated type unsoundness ### What? Object safety allows us to name `Self::Assoc` associated types in certain positions if they come from our trait or one of our supertraits. When this check was implemented, I think it failed to consider that supertraits can have different args, and it was only checking def-id equality. This is problematic, since we can sneak different implementations in by implementing `Supertrait<NotActuallyTheSupertraitSubsts>` for a `dyn` type. This can be used to implement an unsound transmute function. See the committed test. ### How do we fix it? We consider the whole trait ref when checking for supertraits. Right now, this is implemented using equality *without* normalization. We erase regions since those don't affect trait selection. This is a limitation that could theoretically affect code that should be accepted, but doesn't matter in practice -- there are 0 crater regression. We could make this check stronger, but I would be worried about cycle issues. I assume that most people are writing `Self::Assoc` so they don't really care about the trait ref being normalized. --- ### What is up w the stacked commit This is built on top of rust-lang#122804 though that's really not related, it's just easier to make this modification with the changes to the object safety code that I did in that PR. The only thing is that PR may make this unsoundness slightly easier to abuse, since there are more positions that allow self-associated-types -- I am happy to stall that change until this PR merges. --- Fixes rust-lang#126079 r? lcnr
…arams, r=compiler-errors Graciously handle `Drop` impls introducing more generic parameters than the ADT Follow up to rust-lang#110577 Fixes rust-lang#126378 Fixes rust-lang#126889 ## Motivation A current issue with the way we check drop impls do not specialize any of their generic parameters is that when the `Drop` impl introduces *more* generic parameters than are present on the ADT, we fail to prove any bounds involving those parameters. This can be demonstrated with the following [code on stable](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=139b65e4294634d7286a3282bc61e628) which fails due to the fact that `<T as Trait>::Assoc == U` is not present in `Foo`s `ParamEnv` even though arguably there is no reason it cannot compiler: ```rust struct Foo<T: Trait>(T); trait Trait { type Assoc; } impl<T: Trait<Assoc = U>, U: ?Sized> Drop for Foo<T> { //~^ ERROR: `Drop` impl requires `<T as Trait>::Assoc == U` but the struct ... fn drop(&mut self) {} } fn main() {} ``` I think the motivation for supporting this code is somewhat lacking, it might be useful in practice for deeply nested associated types where you might want to be able to write: `where T: Trait<Assoc: Other<AnotherAssoc: MoreTrait<YetAnotherAssoc: InnerTrait<Final = U>>>>` in order to be able to just use `U` in the function body instead of writing out the whole nested associated type. Regardless I don't think there is really any reason to *not* support this code and it is relatively easy to support it. What I find slightly more compelling is the fact that when defining a const parameter `const N: u8` we desugar that to having a where clause requiring the constant `N` is typed as `u8` (`ClauseKind::ConstArgHasType`). As we *always* desugar const parameters to have these bounds, if we attempt to prove that some const parameter `N` is of type `u8` and there is no bound on `N` in the enviroment that generally indicates usage of an incorrect `ParamEnv` (this has caught a bug already). Given that, if we write the following code: ```rust #![feature(associated_const_equality)] struct Foo<T: Trait>(T); trait Trait { const ASSOC: usize; } impl<T: Trait<ASSOC = N>, const N: usize> Drop for Foo<T> { fn drop(&mut self) {} } fn main() {} ``` The `Drop` impl would have this desugared where clause about `N` being of type `usize`, and if we were to try to prove that where clause in `Foo`'s `ParamEnv` we would ICE as there would not be any `ConstArgHasType` in the environment (which generally indicates improper `ParamEnv` usage. As this is otherwise well formed code (the `T: Trait<ASSOC = N>` causes `N` to be constrained) we have to handle this *somehow* and I believe the only principled way to support this is the changes I have made to `dropck.rs` that would cause these code examples to compiler (Perhaps we could just throw out all `ConstArgHasType` where clauses from the predicates we prove but that makes me nervous even if it might actually be okay). ## The changes Currently the way `dropck.rs` works is that take the `ParamEnv` of the ADT and instantiate it with the generic arguments used on the self ty of the `impl`. We then instantiate the predicates of the drop impl with the identity params to the impl, e.g. in the original example `<T as Trait>::Assoc == U` stays as `<T as Trait>::Assoc == U`. We then attempt to prove all the where clauses in the instantiated env of the self type ADT. This PR changes us to first instantiate the impl with infer vars, then we equate the self type (with infer vars as its generic arguments) with the self type as written by the user. This causes all generic parameters on the impl that are constrained via associated type/const equality bounds to be left as inference variables while all other parameters are still `Ty`/`Const`/`Region` Finally when instantiating the predicates on the impl, instead of using the identity arguments, we use the list of inference variables of which some have been inferred to the impl parameters. In practice this means that we wind up proving `<T as Trait>::Assoc == ?x` which can succeed just fine. In the const generics example we would wind up trying to prove `ConstArgHasType(?x: usize)` instead of `ConstArgHasType(N: usize)` which avoids the ICE as it is expected to encounter goals of the form `?x: usize`. At a higher level the way I justify/think about this is that as we are proving goals in the environment of the ADT (`Foo` in the above examples), we do not expect to encounter generic parameters from a different environment so we must "deal with them" somehow. In this PR we handle them by replacing them with inference variables as they should either *actually* be unconstrained (and we will error later) or they are constrained to be equal to some associated type/const. To go along with this it would be nice if we were not instantiating the adt's env with the generic arguments to the ADT in the `Drop` impl as it would make it clearer we are proving bounds in the adt's env instead of the `Drop` impl's. Instead we would map the predicates on the drop impl to be valid in the environment of the adt. In practice this causes diagnostic regressions as all of the generic parameters in errors refer to the ones defined on the adt; attempting to map these back to the ones on the impl, while possible, is involved as writing a `TypeFolder` over `FulfillmentError` is non trivial. ## Edge cases There are some subtle interactions here: One is that we should not allow `<T as Trait>::Assoc == U` to be present on the `Drop` if `U` is constrained by the self type of the impl and the bound is not present in the ADT's environment. demonstrated with the [following code](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=af839e2c3e43e03a624825c58af84dff): ```rust trait Trait { type Assoc; } struct Foo<T: Trait, U: ?Sized>(T, U); impl<T: Trait<Assoc = U>, U: ?Sized> Drop for Foo<T, U> { //~^ ERROR: `Drop` impl requires `<T as Trait>::Assoc == U` fn drop(&mut self) {} } fn main() {} ``` This is tested at `tests/ui/dropck/constrained_by_assoc_type_equality_and_self_ty.rs`. Another weirdness is that we permit the following code to compile now: ```rust struct Foo<T>(T); impl<'a, T: 'a> Drop for Foo<T> { fn drop(&mut self) {} } ``` This is caused by the fact that we permit unconstrained lifetime parameters in trait implementations as long as they are not used in associated types (so we do not wind up erroring on this code like we perhaps ought to), combined with the fact that as we are now proving `T: '?x` instead of `T: 'a` which allows proving the bound via `'?x= 'empty` wheras previously it would have failed. This is tested as part of `tests/ui/dropck/reject-specialized-drops-8142.rs`. --- r? `@compiler-errors`
…cls, r=cuviper Use `#[rustfmt::skip]` on some `use` groups to prevent reordering. `use` declarations will be reformatted in rust-lang#125443. Very rarely, there is a desire to force a group of `use` declarations together in a way that auto-formatting will break up. E.g. when you want a single comment to apply to a group. rust-lang#126776 dealt with all of these in the codebase, ensuring that no comments intended for multiple `use` declarations would end up in the wrong place. But some people were unhappy with it. This commit uses `#[rustfmt::skip]` to create these custom `use` groups in an idiomatic way for a few of the cases changed in rust-lang#126776. This works because rustfmt treats any `use` item annotated with `#[rustfmt::skip]` as a barrier and won't reorder other `use` items around it. r? `@cuviper`
Various notes on match lowering This is an assortment of comments for things that I found unclear or confusing when I was learning how match lowering works. This PR only adds/modifies comments, so there are no functional changes. I have tried to avoid touching code that would conflict with rust-lang#127159. r? `@Nadrieril`
…s, r=workingjubilee Stop using `unsized_const_parameters` in core/std `feature(unsized_const_parameters)` is an incomplete feature and should not be used by core/std as it makes it can make it significantly harder to evolve the feature. It also just generally opens the possibility of introducing bugs on stable through std's backdoor. The only usage of this feature in std is the `simd_shuffle_intrinsic` added in rust-lang#119213. It doesn't seem to be used anywhere as far as I can tell so it is removed in this PR. All tests and codegen logic etc have been kept however. r? `@workingjubilee`
LLVM: LLVM-20.0 removes MMX types See llvm/llvm-project#98505 `@rustbot` label: +llvm-main
…ross35 fix: compilation issue w/ refactored type Fixes a compilation issue related to rust-lang#121478
rustbot
added
O-windows
|
@bors r+ rollup=never p=7 |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
📌 Perf builds for each rolled up PR:
previous master: 2f26b2a99a In the case of a perf regression, run the following command for each PR you suspect might be the cause: |
This was referenced Jul 26, 2024
Merged
|
Finished benchmarking commit (72d73ce): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 771.056s -> 771s (-0.01%) |
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:
Dropimpls introducing more generic parameters than the ADT #127220 (Graciously handleDropimpls introducing more generic parameters than the ADT)#[rustfmt::skip]on someusegroups to prevent reordering. #127950 (Use#[rustfmt::skip]on someusegroups to prevent reordering.)unsized_const_parametersin core/std #128150 (Stop usingunsized_const_parametersin core/std)change_timerefactored type #128211 (fix: compilation issue w/ refactored type)r? @ghost
@rustbot modify labels: rollup
Create a similar rollup