Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

the order of tcx.implementations_of_trait is unstable #120371

Closed
lcnr opened this issue Jan 26, 2024 · 2 comments · Fixed by #120812
Closed

the order of tcx.implementations_of_trait is unstable #120371

lcnr opened this issue Jan 26, 2024 · 2 comments · Fixed by #120812
Labels
A-metadata Area: Crate metadata C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@lcnr
Copy link
Contributor

lcnr commented Jan 26, 2024

given the following extern crate dep:

mod private {
    pub trait Hidden {}
    impl Hidden for &u128 {}
    impl Hidden for &u64 {}
    impl Hidden for &u32 {}
    impl Hidden for &u16 {}
    impl Hidden for &u8 {}
}

pub trait Sealed: private::Hidden {}

and the following root crate:

struct Local;
impl dep::Sealed for Local {} 

fn main() {}

The order in trait_impls_of depends on crate metadata of dep. The rest is only necessary to reproduce this issue.

with the following cargo.toml file

[package]
name = "test2"
version = "0.1.0"
edition = "2021"

[profile.dev]
codegen-units = <insert num codegen-units here>

[dependencies]
dep = { path = "dep" }

the output of cargo c is unstable:

with 1 codegen unit it's

error[E0277]: the trait bound `Local: dep::private::Hidden` is not satisfied
  --> src/main.rs:2:22
   |
2  | impl dep::Sealed for Local {} 
   |                      ^^^^^ the trait `dep::private::Hidden` is not implemented for `Local`
   |
   = help: the following other types implement trait `dep::private::Hidden`:
             &u128
             &u64
             &u32
             &u16
             &u8
note: required by a bound in `Sealed`
  --> /home/lcnr/test2/dep/src/lib.rs:10:19
   |
10 | pub trait Sealed: private::Hidden {}
   |                   ^^^^^^^^^^^^^^^ required by this bound in `Sealed`
   = note: `Sealed` is a "sealed trait", because to implement it you also need to implement `dep::private::Hidden`, which is not accessible; this is usually done to force you to use one of the provided types that already implement it
   = help: the following types implement the trait:
             &u32
             &u8
             &u16
             &u64
             &u128

while with 2 it is

error[E0277]: the trait bound `Local: dep::private::Hidden` is not satisfied
  --> src/main.rs:2:22
   |
2  | impl dep::Sealed for Local {} 
   |                      ^^^^^ the trait `dep::private::Hidden` is not implemented for `Local`
   |
   = help: the following other types implement trait `dep::private::Hidden`:
             &u128
             &u64
             &u32
             &u16
             &u8
note: required by a bound in `Sealed`
  --> /home/lcnr/test2/dep/src/lib.rs:10:19
   |
10 | pub trait Sealed: private::Hidden {}
   |                   ^^^^^^^^^^^^^^^ required by this bound in `Sealed`
   = note: `Sealed` is a "sealed trait", because to implement it you also need to implement `dep::private::Hidden`, which is not accessible; this is usually done to force you to use one of the provided types that already implement it
   = help: the following types implement the trait:
             &u8
             &u16
             &u64
             &u128
             &u32
@lcnr lcnr added the C-bug Category: This is a bug. label Jan 26, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jan 26, 2024
@lcnr
Copy link
Contributor Author

lcnr commented Jan 26, 2024

cc #120012

@lcnr lcnr changed the title the order implementations_of_trait is unstable the order of tcx.implementations_of_trait is unstable Jan 26, 2024
@fmease fmease added A-metadata Area: Crate metadata T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jan 26, 2024
@lcnr
Copy link
Contributor Author

lcnr commented Feb 5, 2024

discussed in https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/codegen-units.20impacts.20order.20of.20impls

root cause is

// Bring everything into deterministic order for hashing
all_impls.sort_by_cached_key(|&(trait_def_id, _)| tcx.def_path_hash(trait_def_id));
let all_impls: Vec<_> = all_impls
.into_iter()
.map(|(trait_def_id, mut impls)| {
// Bring everything into deterministic order for hashing
impls.sort_by_cached_key(|&(index, _)| {
tcx.hir().def_path_hash(LocalDefId { local_def_index: index })
});
TraitImpls {
trait_id: (trait_def_id.krate.as_u32(), trait_def_id.index),
impls: self.lazy_array(&impls),
}
})
.collect();
self.lazy_array(&all_impls)
}

@cjgillot said:

The input by calling tcx.hir().items is already in deterministic order. We can just remove the sorting.

bors added a commit to rust-lang-ci/rust that referenced this issue Jul 21, 2024
Remove unnecessary impl sorting in queries and metadata

Removes unnecessary impl sorting because queries already return their keys in HIR definition order: rust-lang#120371 (comment)

r? `@cjgillot` or `@lcnr` -- unless I totally misunderstood what was being asked for here? 😆

fixes rust-lang#120371
@bors bors closed this as completed in 0f8534e Jul 22, 2024
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Jul 24, 2024
Remove unnecessary impl sorting in queries and metadata

Removes unnecessary impl sorting because queries already return their keys in HIR definition order: rust-lang/rust#120371 (comment)

r? `@cjgillot` or `@lcnr` -- unless I totally misunderstood what was being asked for here? 😆

fixes #120371
flip1995 pushed a commit to flip1995/rust-clippy that referenced this issue Jul 25, 2024
Remove unnecessary impl sorting in queries and metadata

Removes unnecessary impl sorting because queries already return their keys in HIR definition order: rust-lang/rust#120371 (comment)

r? `@cjgillot` or `@lcnr` -- unless I totally misunderstood what was being asked for here? 😆

fixes #120371
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Jul 28, 2024
Remove unnecessary impl sorting in queries and metadata

Removes unnecessary impl sorting because queries already return their keys in HIR definition order: rust-lang/rust#120371 (comment)

r? `@cjgillot` or `@lcnr` -- unless I totally misunderstood what was being asked for here? 😆

fixes #120371
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-metadata Area: Crate metadata C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants