Rollup of 4 pull requests #151443
Merged
Rollup of 4 pull requests #151443
+746
−372
Conversation
This file contains hidden or 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
…tible_trait], which makes the marked trait dyn-incompatible. Removes the attribute from `MetaSized` and `PointeeSized`, with a special case in the trait solvers for `MetaSized`. `dyn MetaSized` is a perfectly cromulent type, and seems to only have had #[rustc_do_not_implement_via_object] so the builtin object candidate does not overlap with the builtin MetaSized impl that all `dyn` types get. Resolves this with a special case by checking `is_sizedness_trait` where the trait solvers previously checked `implement_via_object`. `dyn PointeeSized` alone is rejected for other reasons (since `dyn PointeeSized` is considered to have no principal trait because `PointeeSized` is removed at an earlier stage of the compiler), but `(dyn PointeeSized + Send)` is valid and equivalent to `dyn Send`. Add suggestions from code review Update compiler/rustc_trait_selection/src/traits/dyn_compatibility.rs and tests Co-authored-by: lcnr <rust@lcnr.de>
The dep names needed here are statically available from `rustc_middle`.
Replace `#[rustc_do_not_implement_via_object]` with `#[rustc_dyn_incompatible_trait]` Background: `#[rustc_do_not_implement_via_object]` on a trait currently still allows `dyn Trait` to exist (if the trait is otherwise dyn-compatible), it just means that `dyn Trait` does not automatically implement `Trait` via the normal object candidate. For some traits, this means that `dyn Trait` does not implement `Trait` at all (e.g. `Unsize` and `Tuple`). For some traits, this means that `dyn Trait` implements `Trait`, but with different associated types (e.g. `Pointee`, `DiscriminantKind`). Both of these cases can can cause issues with codegen , as seen in rust-lang#148089 (and rust-lang#148089 (comment) ), because codegen assumes that if `dyn Trait` does not implement `Trait` (including if `dyn Trait<Assoc = T>` does not implement `Trait` with `Assoc == T`), then `dyn Trait` cannot be constructed, so vtable accesses on `dyn Trait` are unreachable, but this is not the case if `dyn Trait` has multiple supertraits: one which is `#[rustc_do_not_implement_via_object]`, and one which we are doing the vtable access to call a method from. This PR replaces `#[rustc_do_not_implement_via_object]` with `#[rustc_dyn_incompatible_trait]`, which makes the marked trait dyn-incompatible, making `dyn Trait` not well-formed, instead of it being well-formed but not implementing `Trait`. This resolves rust-lang#148089 by making it not compile. May fix rust-lang#148615 The traits that are currently marked `#[rustc_do_not_implement_via_object]` are: `Sized`, `MetaSized`, `PointeeSized`, `TransmuteFrom`, `Unsize`, `BikeshedGuaranteedNoDrop`, `DiscriminantKind`, `Destruct`, `Tuple`, `FnPtr`, `Pointee`. Of these: * `Sized` and `FnPtr` are already not dyn-compatible (`FnPtr: Copy`, which implies `Sized`) * `MetaSized` * Removed `#[rustc_do_not_implement_via_object]`. Still dyn-compatible after this change. (Has a special-case in the trait solvers to ignore the object candidate for `dyn MetaSized`, since it `dyn MetaSized: MetSized` comes from the sized candidate that all `dyn Trait` get.) * `PointeeSized` * Removed `#[rustc_do_not_implement_via_object]`. It doesn't seem to have been doing anything anyway ([playground](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2024&gist=a395626c8bef791b87a2d371777b7841)), since `PointeeSized` is removed before trait solving(?). * `Pointee`, `DiscriminantKind`, `Unsize`, and `Tuple` being dyn-compatible without having `dyn Trait: Trait` (with same assoc tys) can be observed to cause codegen issues (rust-lang#148089) so should be made dyn-incompatible * `Destruct`, `TransmuteFrom`, and `BikeshedGuaranteedNoDrop` I'm not sure if would be useful as object types, but they can be relaxed to being dyn-compatible later if it is determined they should be. ----- <details> <summary> resolved </summary> Questions before merge: 1. `dyn MetaSized: MetaSized` having both `SizedCandidate` and `ObjectCandidate` 1. I'm not sure if the checks in compiler/rustc_trait_selection/src/traits/project.rs and compiler/rustc_next_trait_solver/src/solve/assembly/mod.rs were "load-bearing" for `MetaSized` (which is the only trait that was previously `#[rustc_do_not_implement_via_object]` that is still dyn-compatible after this change). Is it fine to just remove them? Removing them (as I did in the second commit) doesn't change any UI test results. 3. IIUC, `dyn MetaSized` could get its `MetaSized` implementation in two ways: the object candidate (the normal `dyn Trait: Trait`) that was supressed by `#[rustc_do_not_implement_via_object]`, and the `SizedCandidate` (that all `dyn Trait` get for `dyn Trait: MetaSized`). Given that `MetaSized` has no associated types or methods, is it fine that these both exist now? Or is it better to only have the `SizedCandidate` and leave these checks in (i.e. drop the second commit, and remove the FIXMEs)? 4. Resolved: the trait solvers special-case `dyn MetaSized` to ignore the object candidate in preference to the sizedness candidate (technically the check is for any `is_sizedness_trait`, but only `MetaSized` gets this far (`Sized` is inherently dyn-incompatible, and `dyn PointeeSized` is ill-formed for other reasons) 4. Diagnostics improvements? 1. The diagnostics are kinda bad. If you have a `trait Foo: Pointee {}`, you now get a note that reads like *Foo* "opted out of dyn-compatbility", when really `Pointee` did that. 2. Resolved: can be improved later <details> <summary>diagnostic example</summary> ```rs #![feature(ptr_metadata)] trait C: std::ptr::Pointee {} fn main() { let y: &dyn C; } ``` ```rs error[E0038]: the trait `C` is not dyn compatible --> c.rs:6:17 | 6 | let y: &dyn C; | ^ `C` is not dyn compatible | note: for a trait to be dyn compatible it needs to allow building a vtable for more information, visit <https://doc.rust-lang.org/reference/items/traits.html#dyn-compatibility> --> /home/zachary/opt_mount/zachary/Programming/rust-compiler-2/library/core/src/ptr/metadata.rs:57:1 | 57 | #[rustc_dyn_incompatible_trait] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ...because it opted out of dyn-compatbility | ::: c.rs:3:7 | 3 | trait C: std::ptr::Pointee {} | - this trait is not dyn compatible... error: aborting due to 1 previous error For more information about this error, try `rustc --explain E0038`. ``` </details> </details> Still investigating "3. `compiler/rustc_hir/src/attrs/encode_cross_crate.rs`: Should `DynIncompatibleTrait` attribute be encoded cross crate?"
Move LTO to OngoingCodegen::join This will make it easier to in the future move all this code to link_binary. Follow up to rust-lang#147810 Part of rust-lang/compiler-team#908
Simplify lookup of `DepKind` names in duplicate dep node check This PR simplifies parts of the query system, by removing the `make_dep_kind_name_array!` macro, and removing much of the associated plumbing added by 68fd771. Instead, we now create a `DEP_KIND_NAMES` constant in `define_dep_nodes!`, and look up names in that instead.
Roll bootstrap reviewers for `src/tools/build-manifest` I honestly don't know who's supposed to be maintaining this tool, but maybe bootstrap? 🤷 r? @Kobzol (maybe)
rustbot
added
A-attributes
Member
Author
|
@bors r+ rollup=never p=4 |
Contributor
rust-bors
bot
added
S-waiting-on-bors
This comment has been minimized.
This comment has been minimized.
rust-bors
bot
added
merged-by-bors
Contributor
Collaborator
|
📌 Perf builds for each rolled up PR:
previous master: 88ad3d44ca In the case of a perf regression, run the following command for each PR you suspect might be the cause: |
Contributor
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 88ad3d4 (parent) -> 838db25 (this PR) Test differencesShow 102 test diffsStage 1
Stage 2
Additionally, 94 doctest diffs were found. These are ignored, as they are noisy. Job group index
Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 838db2538201a845a3694c99d9114a1acebd6e28 --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
Collaborator
|
Finished benchmarking commit (838db25): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -8.2%, secondary -3.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -1.1%, secondary 1.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 472.964s -> 472.653s (-0.07%) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
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:
#[rustc_do_not_implement_via_object]with#[rustc_dyn_incompatible_trait]#148637 (Replace#[rustc_do_not_implement_via_object]with#[rustc_dyn_incompatible_trait])DepKindnames in duplicate dep node check #151402 (Simplify lookup ofDepKindnames in duplicate dep node check)src/tools/build-manifest#151437 (Roll bootstrap reviewers forsrc/tools/build-manifest)r? @ghost
Create a similar rollup