Remove PartialOrd, Ord from DefId

[pierwill]

Part of work on #90317.

[rust-highfive]

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

[pierwill]

r? @cjgillot

[bors]

☔ The latest upstream changes (presumably #89550) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #90423) made this pull request unmergeable. Please resolve the merge conflicts.

[pierwill]

@rustbot author

[bors]

☔ The latest upstream changes (presumably #91924) made this pull request unmergeable. Please resolve the merge conflicts.

[pierwill]

@rustbot ready

I suspect that many of the errors are from removing the various calls to sort in b1c47c9. However, I'm not sure how best to go about diagnosing the problems.

In an earlier iteration of this PR, I tried just replacing everything in sight with IndexMaps, but there were still tons of UI errors. It seemed like data was out of order and getting lost. A better approach might require looking at how these data types are constructed and where, in order to see if the order can be made deterministic, if not meaningful.

Update: I tested each of the calls to sort in b1c47c9. For each one, removing it from a fresh branch off of master caused UI errors, often the same ones. (I was working quickly, so I didn't run the entire UI suite to record all the errors.)

[cjgillot]

Thanks for the PR @pierwill, I expected much less use of these ordering impls.

Could you please reorder/remix your commits into commits that pass x.py check? This makes it much easier to review and to better see the consequences of each modification.
The main concern is the iteration order in each place where we replace a BTree by a hash-map.
You can use the internal lint rustc::potential_query_instability to help finding such issues. (lint has been removed)

[cjgillot]

Is this FxHashMap used anyhere?

[pierwill]

Maybe not? 👀 🤔 I'm not entirely sure how to check. The lsp-find-references command in Emacs shows several places where the returned map is ignored:

compiler/rustc_borrowck/src/universal_regions.rs
736:         let (value, _map) = self.tcx.replace_late_bound_regions(value, |br| {
compiler/rustc_middle/src/ty/fold.rs
821:         self.replace_late_bound_regions(value, |br| {
911:         self.replace_late_bound_regions(value, |_| self.lifetimes.re_erased).0
928:             .replace_late_bound_regions(sig, |_| {
compiler/rustc_middle/src/ty/print/pretty.rs
2160:             self.tcx.replace_late_bound_regions(value.clone(), |br| {
compiler/rustc_typeck/src/collect.rs
457:                                             .replace_late_bound_regions(poly_trait_ref, |_| {

[cjgillot]

Does any use site rely on the iteration order?

[pierwill]

Maybe here?

rust/compiler/rustc_borrowck/src/type_check/mod.rs

Line 1589 in 181e915

for &late_bound_region in map.values() {

[cjgillot]

Where is this FxHashMap used? Does any code depend on the iteration order?

[pierwill]

This map is used in two places:

• rust/compiler/rustc_middle/src/ty/print/pretty.rs

  Line 797 in 181e915

  self.insert_trait_and_projection(trait_ref, None, &mut traits, &mut fn_traits);

• rust/compiler/rustc_middle/src/ty/print/pretty.rs

  Lines 806 to 807 in 181e915

  	self.insert_trait_and_projection(
  	trait_ref,

[cjgillot]

Same question?

[pierwill]

Used in the same calls as above #90749 (comment).

[cjgillot]

Does any code depend on the iteration order?

[pierwill]

Yes:

• rust/compiler/rustc_infer/src/infer/lexical_region_resolve/mod.rs

  Line 189 in 181e915

  for (idx, (constraint, _)) in self.data.constraints.iter().enumerate() {

• rust/compiler/rustc_infer/src/infer/lexical_region_resolve/mod.rs

  Line 229 in 181e915

  for constraint in self.data.constraints.keys() {

• rust/compiler/rustc_infer/src/infer/lexical_region_resolve/mod.rs

  Line 449 in 181e915

  for (constraint, origin) in &self.data.constraints {

• rust/compiler/rustc_infer/src/infer/lexical_region_resolve/mod.rs

  Line 618 in 181e915

  for constraint in self.data.constraints.keys() {

• rust/compiler/rustc_trait_selection/src/traits/auto_trait.rs

  Line 515 in 181e915

  for constraint in regions.constraints.keys() {

[cjgillot]

Does any code depend on the iteration order?

[pierwill]

Possibly here?

rust/compiler/rustc_trait_selection/src/traits/project.rs

Lines 464 to 484 in f1ce0e6

	let data = data.super_fold_with(self);
	let normalized_ty = opt_normalize_projection_type(
	self.selcx,
	self.param_env,
	data,
	self.cause.clone(),
	self.depth,
	&mut self.obligations,
	)
	.ok()
	.flatten()
	.map(|normalized_ty| {
	PlaceholderReplacer::replace_placeholders(
	infcx,
	mapped_regions,
	mapped_types,
	mapped_consts,
	&self.universes,
	normalized_ty,
	)
	})

[cjgillot]

This impl is wrong and will probably produce surprising results.
We implement Eq, so we expect a real equality, not just CrateNum.
Is the PartialOrd/Ord bound used? Where? Can we live without?

[pierwill]

The ordering is used here https://github.com/rust-lang/rust/blob/088dd7cb54f57bd1c870a343cb5dc3d3d2703761/compiler/rustc_typeck/src/check/method/suggest.rs#L1651 and in the PartialEq implementation: https://github.com/rust-lang/rust/blob/088dd7cb54f57bd1c870a343cb5dc3d3d2703761/compiler/rustc_typeck/src/check/method/suggest.rs#L1906

[pierwill]

And the Eq,PartialEq are currently used for deduplication: https://github.com/rust-lang/rust/blob/088dd7cb54f57bd1c870a343cb5dc3d3d2703761/compiler/rustc_typeck/src/check/method/suggest.rs#L1652

[pierwill]

Would it work or defeat our purpose to sort the TraitInfos by DefIndex?

[cjgillot]

Do we still rely on the iteration order?

[pierwill]

Yes:

rust/compiler/rustc_typeck/src/astconv/mod.rs

Line 1403 in 181e915

for def_ids in associated_types.values_mut() {

[cjgillot]

Do we rely somewhere on the iteration order?

[pierwill]

Yes:

rust/compiler/rustc_typeck/src/outlives/explicit.rs

Line 31 in 181e915

for &(predicate, span) in predicates.predicates {

[bors]

☔ The latest upstream changes (presumably #92844) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #92805) made this pull request unmergeable. Please resolve the merge conflicts.

[rust-log-analyzer]

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

    Checking rustc_builtin_macros v0.0.0 (/checkout/compiler/rustc_builtin_macros)
error[E0308]: mismatched types
   --> compiler/rustc_middle/src/ty/print/pretty.rs:794:71
    |
794 |                     self.insert_trait_and_projection(trait_ref, None, &mut traits, &mut fn_traits);
    |                                                                       ^^^^^^^^^^^ expected struct `indexmap::map::IndexMap`, found struct `HashMap`
    |
    = note: expected mutable reference `&mut indexmap::map::IndexMap<Binder<'tcx, sty::TraitRef<'tcx>>, indexmap::map::IndexMap<rustc_span::def_id::DefId, Binder<'tcx, ty::Term<'tcx>>, BuildHasherDefault<FxHasher>>, BuildHasherDefault<FxHasher>>`
               found mutable reference `&mut HashMap<_, _, BuildHasherDefault<FxHasher>>`
error[E0308]: mismatched types
   --> compiler/rustc_middle/src/ty/print/pretty.rs:794:84
    |
    |
794 |                     self.insert_trait_and_projection(trait_ref, None, &mut traits, &mut fn_traits);
    |                                                                                    ^^^^^^^^^^^^^^ expected struct `indexmap::map::IndexMap`, found struct `HashMap`
    |
    = note: expected mutable reference `&mut indexmap::map::IndexMap<Binder<'tcx, sty::TraitRef<'tcx>>, OpaqueFnEntry<'tcx>, BuildHasherDefault<FxHasher>>`
               found mutable reference `&mut HashMap<_, _, BuildHasherDefault<FxHasher>>`
error[E0308]: mismatched types
   --> compiler/rustc_middle/src/ty/print/pretty.rs:806:25
    |
806 |                         &mut traits,
806 |                         &mut traits,
    |                         ^^^^^^^^^^^ expected struct `indexmap::map::IndexMap`, found struct `HashMap`
    |
    = note: expected mutable reference `&mut indexmap::map::IndexMap<Binder<'tcx, sty::TraitRef<'tcx>>, indexmap::map::IndexMap<rustc_span::def_id::DefId, Binder<'tcx, ty::Term<'tcx>>, BuildHasherDefault<FxHasher>>, BuildHasherDefault<FxHasher>>`
               found mutable reference `&mut HashMap<_, _, BuildHasherDefault<FxHasher>>`
error[E0308]: mismatched types
   --> compiler/rustc_middle/src/ty/print/pretty.rs:807:25
    |
807 |                         &mut fn_traits,
807 |                         &mut fn_traits,
    |                         ^^^^^^^^^^^^^^ expected struct `indexmap::map::IndexMap`, found struct `HashMap`
    |
    = note: expected mutable reference `&mut indexmap::map::IndexMap<Binder<'tcx, sty::TraitRef<'tcx>>, OpaqueFnEntry<'tcx>, BuildHasherDefault<FxHasher>>`
               found mutable reference `&mut HashMap<_, _, BuildHasherDefault<FxHasher>>`

error[E0277]: can't compare `TyS<'_>` with `TyS<'_>`
    --> compiler/rustc_middle/src/ty/mod.rs:787:8
     |
784  |   #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord, TyEncodable, TyDecodable)]
     |                                                     ---------- in this derive macro expansion
...
787  |       Ty(Ty<'tcx>),
     |          ^^^^^^^^ no implementation for `TyS<'_> < TyS<'_>` and `TyS<'_> > TyS<'_>`
    ::: /checkout/library/core/src/cmp.rs:1154:1
     |
     |
1154 | / pub macro PartialOrd($item:item) {
1156 | | }
1156 | | }
     | |_- in this expansion of `#[derive(PartialOrd)]`
     |
     = help: the trait `std::cmp::PartialOrd<TyS<'_>>` is not implemented for `TyS<'_>`
     = note: required because of the requirements on the impl of `std::cmp::PartialOrd<&TyS<'_>>` for `&TyS<'_>`

error[E0277]: can't compare `ty::consts::Const<'_>` with `ty::consts::Const<'_>`
    --> compiler/rustc_middle/src/ty/mod.rs:788:11
     |
784  |   #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord, TyEncodable, TyDecodable)]
     |                                                     ---------- in this derive macro expansion
...
788  |       Const(&'tcx Const<'tcx>),
     |             ^^^^^^^^^^^^^^^^^ no implementation for `ty::consts::Const<'_> < ty::consts::Const<'_>` and `ty::consts::Const<'_> > ty::consts::Const<'_>`
    ::: /checkout/library/core/src/cmp.rs:1154:1
     |
     |
1154 | / pub macro PartialOrd($item:item) {
1156 | | }
1156 | | }
     | |_- in this expansion of `#[derive(PartialOrd)]`
     |
     = help: the trait `std::cmp::PartialOrd<ty::consts::Const<'_>>` is not implemented for `ty::consts::Const<'_>`
     = note: required because of the requirements on the impl of `std::cmp::PartialOrd<&ty::consts::Const<'_>>` for `&ty::consts::Const<'_>`

error[E0277]: the trait bound `TyS<'_>: Ord` is not satisfied
   --> compiler/rustc_middle/src/ty/mod.rs:787:8
    |
784 |   #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord, TyEncodable, TyDecodable)]
    |                                                                 --- in this derive macro expansion
...
787 |       Ty(Ty<'tcx>),
    |          ^^^^^^^^ the trait `Ord` is not implemented for `TyS<'_>`
   ::: /checkout/library/core/src/cmp.rs:849:1
    |
    |
849 | / pub macro Ord($item:item) {
851 | | }
851 | | }
    | |_- in this expansion of `#[derive(Ord)]`
    |
    = note: required because of the requirements on the impl of `Ord` for `&TyS<'_>`

error[E0277]: the trait bound `ty::consts::Const<'_>: Ord` is not satisfied
   --> compiler/rustc_middle/src/ty/mod.rs:788:11
    |
784 |   #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord, TyEncodable, TyDecodable)]
    |                                                                 --- in this derive macro expansion
...
788 |       Const(&'tcx Const<'tcx>),
    |             ^^^^^^^^^^^^^^^^^ the trait `Ord` is not implemented for `ty::consts::Const<'_>`
   ::: /checkout/library/core/src/cmp.rs:849:1
    |
    |
849 | / pub macro Ord($item:item) {
851 | | }
851 | | }
    | |_- in this expansion of `#[derive(Ord)]`
    |
    = note: required because of the requirements on the impl of `Ord` for `&ty::consts::Const<'_>`
Some errors have detailed explanations: E0277, E0308.
For more information about an error, try `rustc --explain E0277`.
error: could not compile `rustc_middle` due to 8 previous errors
warning: build failed, waiting for other jobs to finish...

[pierwill]

Closing this (for now), as I'm looking at making this change in a series of smaller PRs.