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

Jump to bottom

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

merged 1 commit into from
Jun 27, 2020

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Jun 18, 2020 •
edited
Loading

Address #59326.

@rust-highfive
Copy link
Collaborator

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 18, 2020
@estebank
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Jun 18, 2020

⌛ Trying commit b7f2396 with merge c9f613bb7aaebcb256e34b959196f56f62ca74c8...

@estebank estebank mentioned this pull request Jun 18, 2020
Open
hellow554
hellow554 reviewed Jun 18, 2020
View reviewed changes
src/librustc_middle/ty/relate.rs
Comment on lines +627 to +628
a_v.sort_by(|a, b| a.stable_cmp(tcx, b));
a_v.dedup();
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.

@bors
Copy link
Contributor

bors commented Jun 18, 2020

☀️ Try build successful - checks-azure
Build commit: c9f613bb7aaebcb256e34b959196f56f62ca74c8 (c9f613bb7aaebcb256e34b959196f56f62ca74c8)

@rust-timer
Copy link
Collaborator

Queued c9f613bb7aaebcb256e34b959196f56f62ca74c8 with parent e55d3f9, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (c9f613bb7aaebcb256e34b959196f56f62ca74c8): comparison url.

@estebank estebank added beta-nominated Nominated for backporting to the compiler in the beta channel. stable-nominated Nominated for backporting to the compiler in the stable channel. labels Jun 19, 2020
@spastorino
Copy link
Member

Adding T-compiler so our tools are able to fetch it and include it on the weekly agenda.

@spastorino spastorino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 24, 2020
@estebank
Copy link
Contributor Author

r? @nikomatsakis as per https://rust-lang.zulipchat.com/#narrow/stream/238009-t-compiler.2Fmeetings/topic/.5Bweekly.20meeting.5D.202020-06-25.20.2354818/near/201977869

@rust-highfive rust-highfive assigned nikomatsakis and unassigned eddyb Jun 25, 2020
@nikomatsakis
Copy link
Contributor

@bors r+

As I said on Zulip, this doesn't seem like the "right fix" to me, but it is a fix, and I don't see it causing any fresh problems, and there's an imminent regression, so r+.

@bors
Copy link
Contributor

bors commented Jun 26, 2020

📌 Commit b7f2396 has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 26, 2020
nikomatsakis
nikomatsakis reviewed Jun 26, 2020
View reviewed changes
src/librustc_middle/ty/relate.rs
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?

@Manishearth Manishearth mentioned this pull request Jun 26, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 27, 2020
@bors
Auto merge of rust-lang#73779 - Manishearth:rollup-lwqd9jm, r=Manishe…
394e1b4 
…arth

Rollup of 12 pull requests

Successful merges:

 - rust-lang#72771 (Warn if linking to a private item)
 - rust-lang#72937 (Fortanix SGX target libunwind build process changes)
 - rust-lang#73485 (Perform obligation deduplication to avoid buggy `ExistentialMismatch`)
 - rust-lang#73529 (Add liballoc impl SpecFromElem for i8)
 - rust-lang#73579 (add missing doc links)
 - rust-lang#73627 (Shortcuts for min/max on double-ended BTreeMap/BTreeSet iterators)
 - rust-lang#73691 (Bootstrap: detect Windows based on sys.platform)
 - rust-lang#73694 (Document the Self keyword)
 - rust-lang#73718 (Document the super keyword)
 - rust-lang#73728 (Document some invariants correctly/more)
 - rust-lang#73738 (Remove irrelevant comment)
 - rust-lang#73765 (Remove blank line)

Failed merges:

r? @ghost
@bors bors merged commit f13d09a into rust-lang:master Jun 27, 2020
@lcnr lcnr mentioned this pull request Jun 27, 2020
Closed
@pnkfelix
Copy link
Member

pnkfelix commented Jul 2, 2020

discussed in T-compiler meeting. Approved for beta-backport.

@pnkfelix pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jul 2, 2020
@pnkfelix
Copy link
Member

pnkfelix commented Jul 2, 2020

stable nom was also discussed. Stable backport approved.

@pnkfelix pnkfelix added the stable-accepted Accepted for backporting to the compiler in the stable channel. label Jul 2, 2020
@Elinvynia Elinvynia mentioned this pull request Jul 6, 2020
Closed
@Mark-Simulacrum Mark-Simulacrum removed beta-nominated Nominated for backporting to the compiler in the beta channel. stable-accepted Accepted for backporting to the compiler in the stable channel. stable-nominated Nominated for backporting to the compiler in the stable channel. labels Jul 10, 2020
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Jul 10, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 13, 2020
@bors
Auto merge of rust-lang#74219 - Mark-Simulacrum:beta-next, r=Mark-Sim…
e99e642 
…ulacrum

[beta] next

Backports of:

* rustdoc: Fix doc aliases with crate filtering rust-lang#73644
* rustdoc: Rename invalid_codeblock_attribute lint to be plural rust-lang#74131
* rustc_lexer: Simplify shebang parsing once more rust-lang#73596
* Perform obligation deduplication to avoid buggy `ExistentialMismatch` rust-lang#73485
* Reorder order in which MinGW libs are linked to fix recent breakage rust-lang#73184
* Change how compiler-builtins gets many CGUs rust-lang#73136
* Fix wasm32 being broken due to a NodeJS version bump rust-lang#73885
@cuviper cuviper added this to the 1.46 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hellow554 hellow554 hellow554 left review comments

@nikomatsakis nikomatsakis nikomatsakis left review comments

Assignees

@nikomatsakis nikomatsakis

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.46
Development

Successfully merging this pull request may close these issues.