Remove `PartialOrd`, `Ord` from `DefId` #90749

pierwill · 2021-11-10T01:32:14Z

Part of work on #90317.

rust-highfive · 2021-11-10T01:32:17Z

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

pierwill · 2021-11-10T01:32:33Z

bors · 2021-11-11T09:29:52Z

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

bors · 2021-12-12T14:49:15Z

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

See rust-lang/rust#90317 and rust-lang/rust#90749.

pierwill · 2021-12-15T01:53:12Z

@rustbot author

bors · 2021-12-20T13:53:43Z

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

pierwill · 2021-12-30T22:26:13Z

@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 · 2022-01-02T11:08:25Z

compiler/rustc_middle/src/ty/fold.rs

@@ -744,12 +743,12 @@ impl<'tcx> TyCtxt<'tcx> {
        self,
        value: Binder<'tcx, T>,
        mut fld_r: F,
-    ) -> (T, BTreeMap<ty::BoundRegion, ty::Region<'tcx>>)
+    ) -> (T, FxHashMap<ty::BoundRegion, ty::Region<'tcx>>)


Is this FxHashMap used anyhere?

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 · 2022-01-02T11:10:14Z

compiler/rustc_middle/src/ty/fold.rs

@@ -796,14 +795,14 @@ impl<'tcx> TyCtxt<'tcx> {
        mut fld_r: F,
        fld_t: G,
        fld_c: H,
-    ) -> (T, BTreeMap<ty::BoundRegion, ty::Region<'tcx>>)
+    ) -> (T, FxHashMap<ty::BoundRegion, ty::Region<'tcx>>)


Does any use site rely on the iteration order?

Maybe here?

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

Line 1589 in 181e915

for &late_bound_region in map.values() {

cjgillot · 2022-01-02T11:12:49Z

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

@@ -946,7 +945,10 @@ pub trait PrettyPrinter<'tcx>:
        &mut self,
        trait_ref: ty::PolyTraitRef<'tcx>,
        proj_ty: Option<(DefId, ty::Binder<'tcx, Ty<'tcx>>)>,
-        traits: &mut FxHashMap<ty::PolyTraitRef<'tcx>, BTreeMap<DefId, ty::Binder<'tcx, Ty<'tcx>>>>,
+        traits: &mut FxHashMap<


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

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 · 2022-01-02T11:13:02Z

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

-        traits: &mut FxHashMap<ty::PolyTraitRef<'tcx>, BTreeMap<DefId, ty::Binder<'tcx, Ty<'tcx>>>>,
+        traits: &mut FxHashMap<
+            ty::PolyTraitRef<'tcx>,
+            FxHashMap<DefId, ty::Binder<'tcx, Ty<'tcx>>>,


Same question?

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

compiler/rustc_middle/src/ty/subst.rs

cjgillot · 2022-01-02T11:20:38Z

compiler/rustc_infer/src/infer/region_constraints/mod.rs

@@ -90,7 +89,7 @@ pub type VarInfos = IndexVec<RegionVid, RegionVariableInfo>;
 pub struct RegionConstraintData<'tcx> {
    /// Constraints of the form `A <= B`, where either `A` or `B` can
    /// be a region variable (or neither, as it happens).
-    pub constraints: BTreeMap<Constraint<'tcx>, SubregionOrigin<'tcx>>,
+    pub constraints: FxHashMap<Constraint<'tcx>, SubregionOrigin<'tcx>>,


Does any code depend on the iteration order?

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 · 2022-01-02T11:21:14Z

compiler/rustc_trait_selection/src/traits/project.rs

-    mapped_consts: BTreeMap<ty::PlaceholderConst<'tcx>, ty::BoundVar>,
+    mapped_regions: FxHashMap<ty::PlaceholderRegion, ty::BoundRegion>,
+    mapped_types: FxHashMap<ty::PlaceholderType, ty::BoundTy>,
+    mapped_consts: FxHashMap<ty::PlaceholderConst<'tcx>, ty::BoundVar>,


Does any code depend on the iteration order?

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 · 2022-01-02T11:24:41Z

compiler/rustc_typeck/src/check/method/suggest.rs

-        let lhs = (other.def_id.krate, other.def_id);
-        let rhs = (self.def_id.krate, self.def_id);
+        let (lhs, _) = (other.def_id.krate, other.def_id);
+        let (rhs, _) = (self.def_id.krate, self.def_id);
        lhs.cmp(&rhs)


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?

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

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

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

cjgillot · 2022-01-02T11:25:08Z

compiler/rustc_typeck/src/astconv/mod.rs

@@ -1338,7 +1337,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
        }

        // Use a `BTreeSet` to keep output in a more consistent order.
-        let mut associated_types: FxHashMap<Span, BTreeSet<DefId>> = FxHashMap::default();
+        let mut associated_types: FxHashMap<Span, FxHashSet<DefId>> = FxHashMap::default();


Do we still rely on the iteration order?

Yes:

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

Line 1403 in 181e915

for def_ids in associated_types.values_mut() {

cjgillot · 2022-01-02T11:25:27Z

compiler/rustc_typeck/src/outlives/utils.rs


 /// Tracks the `T: 'a` or `'a: 'a` predicates that we have inferred
 /// must be added to the struct header.
 pub type RequiredPredicates<'tcx> =
-    BTreeMap<ty::OutlivesPredicate<GenericArg<'tcx>, ty::Region<'tcx>>, Span>;
+    FxHashMap<ty::OutlivesPredicate<GenericArg<'tcx>, ty::Region<'tcx>>, Span>;


Do we rely somewhere on the iteration order?

Yes:

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

Line 31 in 181e915

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

bors · 2022-01-14T06:38:52Z

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

bors · 2022-01-16T14:34:48Z

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

rust-log-analyzer · 2022-01-18T16:27:43Z

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...

Remove some unused `Ord` derives based on `Span` Remove some `Ord`, `PartialOrd` derivations that rely on underlying ordering of `Span`. These ordering traits appear to be unused right now. If we're going to attempt to remove ordering traits from `Span` as suggested in rust-lang#90317 (comment), we might want to slowly remove code that depends on this ordering (as opposed to the all-at-once approach in rust-lang#90749 and rust-lang#90408). cc `@tmiasko` `@cjgillot`

pierwill · 2022-01-18T23:12:02Z

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

…gillot Remove some unused ordering derivations based on `DefId` Like rust-lang#93018, this removes some unused/unneeded ordering derivations as part of ongoing work on rust-lang#90317. Here, these changes are aimed at making rust-lang#90749 easier to review, test, and merge. r? `@cjgillot`

rust-highfive assigned petrochenkov Nov 10, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 10, 2021

pierwill marked this pull request as draft November 10, 2021 01:32

rust-highfive assigned cjgillot and unassigned petrochenkov Nov 10, 2021