Skip to content

Stop sorting via DefIds in region resolution #122824

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

Merged
merged 1 commit into from
Mar 22, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions compiler/rustc_infer/src/infer/lexical_region_resolve/mod.rs
Original file line number Diff line number Diff line change
@@ -13,6 +13,7 @@ use rustc_data_structures::graph::implementation::{
Direction, Graph, NodeIndex, INCOMING, OUTGOING,
};
use rustc_data_structures::intern::Interned;
use rustc_data_structures::unord::UnordSet;
use rustc_index::{IndexSlice, IndexVec};
use rustc_middle::ty::fold::TypeFoldable;
use rustc_middle::ty::{self, Ty, TyCtxt};
@@ -139,8 +140,8 @@ impl<'cx, 'tcx> LexicalResolver<'cx, 'tcx> {
let mut var_data = self.construct_var_data();

// Deduplicating constraints is shown to have a positive perf impact.
self.data.constraints.sort_by_key(|(constraint, _)| *constraint);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind using an UnordSet here? It is a drop-in replacement in this case but guards against accidentally relying on its iteration order.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't that have the same insertion issues as #118824 (comment) (expensive to insert)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or do you mean for the dedup set? sure, that doesn't matter

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I meant the seen set. The comment ended up in an ambiguous place 🙂

self.data.constraints.dedup_by_key(|(constraint, _)| *constraint);
let mut seen = UnordSet::default();
self.data.constraints.retain(|(constraint, _)| seen.insert(*constraint));

if cfg!(debug_assertions) {
self.dump_constraints();
@@ -888,12 +889,14 @@ impl<'cx, 'tcx> LexicalResolver<'cx, 'tcx> {
}

Constraint::RegSubVar(region, _) | Constraint::VarSubReg(_, region) => {
let constraint_idx =
this.constraints.binary_search_by(|(c, _)| c.cmp(&edge.data)).unwrap();
state.result.push(RegionAndOrigin {
region,
origin: this.constraints[constraint_idx].1.clone(),
});
let origin = this
.constraints
.iter()
.find(|(c, _)| *c == edge.data)
.unwrap()
.1
.clone();
state.result.push(RegionAndOrigin { region, origin });
}

Constraint::RegSubReg(..) => panic!(
2 changes: 1 addition & 1 deletion compiler/rustc_infer/src/infer/region_constraints/mod.rs
Original file line number Diff line number Diff line change
@@ -104,7 +104,7 @@ pub struct RegionConstraintData<'tcx> {
}

/// Represents a constraint that influences the inference process.
#[derive(Clone, Copy, PartialEq, Eq, Debug, PartialOrd, Ord)]
#[derive(Clone, Copy, PartialEq, Eq, Debug, Hash)]
pub enum Constraint<'tcx> {
/// A region variable is a subregion of another.
VarSubVar(RegionVid, RegionVid),
24 changes: 12 additions & 12 deletions tests/ui/issues/issue-27942.stderr
Original file line number Diff line number Diff line change
@@ -6,16 +6,16 @@ LL | fn select(&self) -> BufferViewHandle<R>;
|
= note: expected trait `Resources<'_>`
found trait `Resources<'a>`
note: the anonymous lifetime defined here...
--> $DIR/issue-27942.rs:5:15
|
LL | fn select(&self) -> BufferViewHandle<R>;
| ^^^^^
note: ...does not necessarily outlive the lifetime `'a` as defined here
note: the lifetime `'a` as defined here...
--> $DIR/issue-27942.rs:3:18
|
LL | pub trait Buffer<'a, R: Resources<'a>> {
| ^^
note: ...does not necessarily outlive the anonymous lifetime defined here
--> $DIR/issue-27942.rs:5:15
|
LL | fn select(&self) -> BufferViewHandle<R>;
| ^^^^^

error[E0308]: mismatched types
--> $DIR/issue-27942.rs:5:25
@@ -25,16 +25,16 @@ LL | fn select(&self) -> BufferViewHandle<R>;
|
= note: expected trait `Resources<'_>`
found trait `Resources<'a>`
note: the lifetime `'a` as defined here...
--> $DIR/issue-27942.rs:3:18
|
LL | pub trait Buffer<'a, R: Resources<'a>> {
| ^^
note: ...does not necessarily outlive the anonymous lifetime defined here
note: the anonymous lifetime defined here...
--> $DIR/issue-27942.rs:5:15
|
LL | fn select(&self) -> BufferViewHandle<R>;
| ^^^^^
note: ...does not necessarily outlive the lifetime `'a` as defined here
--> $DIR/issue-27942.rs:3:18
|
LL | pub trait Buffer<'a, R: Resources<'a>> {
| ^^

error: aborting due to 2 previous errors

Original file line number Diff line number Diff line change
@@ -4,16 +4,16 @@ error[E0495]: cannot infer an appropriate lifetime for lifetime parameter `'b` d
LL | impl<'a,'b> T2<'a, 'b> for S<'a, 'b> {
| ^^^^^^^^^
|
note: first, the lifetime cannot outlive the lifetime `'a` as defined here...
--> $DIR/impl-of-supertrait-has-wrong-lifetime-parameters.rs:24:6
|
LL | impl<'a,'b> T2<'a, 'b> for S<'a, 'b> {
| ^^
note: ...but the lifetime must also be valid for the lifetime `'b` as defined here...
note: first, the lifetime cannot outlive the lifetime `'b` as defined here...
--> $DIR/impl-of-supertrait-has-wrong-lifetime-parameters.rs:24:9
|
LL | impl<'a,'b> T2<'a, 'b> for S<'a, 'b> {
| ^^
note: ...but the lifetime must also be valid for the lifetime `'a` as defined here...
--> $DIR/impl-of-supertrait-has-wrong-lifetime-parameters.rs:24:6
|
LL | impl<'a,'b> T2<'a, 'b> for S<'a, 'b> {
| ^^
note: ...so that the types are compatible
--> $DIR/impl-of-supertrait-has-wrong-lifetime-parameters.rs:24:28
|
24 changes: 12 additions & 12 deletions tests/ui/traits/matching-lifetimes.stderr
Original file line number Diff line number Diff line change
@@ -6,16 +6,16 @@ LL | fn foo(x: Foo<'b,'a>) {
|
= note: expected signature `fn(Foo<'a, 'b>)`
found signature `fn(Foo<'b, 'a>)`
note: the lifetime `'b` as defined here...
--> $DIR/matching-lifetimes.rs:13:9
|
LL | impl<'a,'b> Tr for Foo<'a,'b> {
| ^^
note: ...does not necessarily outlive the lifetime `'a` as defined here
note: the lifetime `'a` as defined here...
--> $DIR/matching-lifetimes.rs:13:6
|
LL | impl<'a,'b> Tr for Foo<'a,'b> {
| ^^
note: ...does not necessarily outlive the lifetime `'b` as defined here
--> $DIR/matching-lifetimes.rs:13:9
|
LL | impl<'a,'b> Tr for Foo<'a,'b> {
| ^^

error[E0308]: method not compatible with trait
--> $DIR/matching-lifetimes.rs:14:5
@@ -25,16 +25,16 @@ LL | fn foo(x: Foo<'b,'a>) {
|
= note: expected signature `fn(Foo<'a, 'b>)`
found signature `fn(Foo<'b, 'a>)`
note: the lifetime `'a` as defined here...
--> $DIR/matching-lifetimes.rs:13:6
|
LL | impl<'a,'b> Tr for Foo<'a,'b> {
| ^^
note: ...does not necessarily outlive the lifetime `'b` as defined here
note: the lifetime `'b` as defined here...
--> $DIR/matching-lifetimes.rs:13:9
|
LL | impl<'a,'b> Tr for Foo<'a,'b> {
| ^^
note: ...does not necessarily outlive the lifetime `'a` as defined here
--> $DIR/matching-lifetimes.rs:13:6
|
LL | impl<'a,'b> Tr for Foo<'a,'b> {
| ^^

error: aborting due to 2 previous errors