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.

Sign up for GitHub

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

Perform obligation deduplication to avoid buggy ExistentialMismatch #73485

Merged
merged 1 commit into from
Jun 27, 2020
Merged

Perform obligation deduplication to avoid buggy ExistentialMismatch #73485

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 13 additions & 3 deletions src/librustc_middle/ty/relate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -617,12 +617,22 @@ impl<'tcx> Relate<'tcx> for &'tcx ty::List<ty::ExistentialPredicate<'tcx>> {
a: &Self,
b: &Self,
) -> RelateResult<'tcx, Self> {
if a.len() != b.len() {
let tcx = relation.tcx();

// FIXME: this is wasteful, but want to do a perf run to see how slow it is.
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should open up a FIXME issue for this..? To try and prevent the duplicates in the first place and figure out where they come from?

// We need to perform this deduplication as we sometimes generate duplicate projections
// in `a`.
let mut a_v: Vec<_> = a.into_iter().collect();
let mut b_v: Vec<_> = b.into_iter().collect();
a_v.sort_by(|a, b| a.stable_cmp(tcx, b));
a_v.dedup();
Comment on lines +627 to +628
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it correct, that the first one is a sort_by, but the next one a plain dedup, instead of dedup_by?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, that is probably a potential bug. On the repro case that I posted it didn't cause problems, but I'll change it after the perf run has completed (don't want to break anything by force-pushing to this branch before then).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if dedup will cause trouble in practice and don't know whether dedup_by_key might introduce a slight slowdown. Either way, this PR seems to solve bad rejection part of the ticket.

b_v.sort_by(|a, b| a.stable_cmp(tcx, b));
b_v.dedup();
if a_v.len() != b_v.len() {
return Err(TypeError::ExistentialMismatch(expected_found(relation, a, b)));
}

let tcx = relation.tcx();
let v = a.iter().zip(b.iter()).map(|(ep_a, ep_b)| {
let v = a_v.into_iter().zip(b_v.into_iter()).map(|(ep_a, ep_b)| {
use crate::ty::ExistentialPredicate::*;
match (ep_a, ep_b) {
(Trait(ref a), Trait(ref b)) => Ok(Trait(relation.relate(a, b)?)),
Expand Down
26 changes: 26 additions & 0 deletions src/test/ui/issues/issue-59326.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// check-pass
trait Service {
type S;
}

trait Framing {
type F;
}

impl Framing for () {
type F = ();
}

trait HttpService<F: Framing>: Service<S = F::F> {}

type BoxService = Box<dyn HttpService<(), S = ()>>;

fn build_server<F: FnOnce() -> BoxService>(_: F) {}

fn make_server<F: Framing>() -> Box<dyn HttpService<F, S = F::F>> {
unimplemented!()
}

fn main() {
build_server(|| make_server())
}