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.

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

Add HRTB-related regression test #74889

Merged
merged 2 commits into from
Aug 6, 2020
Merged

Conversation

JohnTitor
Copy link
Member

@JohnTitor JohnTitor commented Jul 28, 2020

Closes #59311 and cc #71546
This closes the former but the test is taken from #71546 (comment) since it seems they have the same cause and it's simplified.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 28, 2020
@lcnr
Copy link
Contributor

lcnr commented Jul 28, 2020

uh, so it makes some sense why this is happening 🤔

Thanks for pinging me here. I think what's going on here is that previously for<'a> Ty<'a>: Trait always resulted in CodeSelectionError(Unimplemented) but we now first replace 'a with a placeholder and and then successfully evaluate Ty<'_0>: Trait during borrowck.

So I think that change is fine, but @nikomatsakis should also take a quick look at this.

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned lcnr Jul 28, 2020
@nikomatsakis
Copy link
Contributor

I'm a bit unsure why this changed behavior, to be honest, and the error seems incorrect to me (that is, I think ideally the code would compile; obviously an error is an improvement over an ICE). It is certainly linked to the for<'a> &'a T: 'static requirement. It's true that this requirement is unsatisfiable, but I don't think it ought to be creating an error within the function body, probably more at the call-site somewhere (which would be the one obligated to prove this).

This probably has something to do with the way we sometimes try to prove that the where-clauses hold even within the callee, which is a bit silly.

@JohnTitor
Copy link
Member Author

but I don't think it ought to be creating an error within the function body, probably more at the call-site somewhere (which would be the one obligated to prove this).

@nikomatsakis Makes sense. So, I think it's a good way here to add this test with a FIXME comment and follow-up the above point later so that we can prevent a regression at least.

@nikomatsakis
Copy link
Contributor

@JohnTitor that sounds reasonable to me, yes

@nikomatsakis
Copy link
Contributor

@JohnTitor can you add a comment to the test that says something like:

// FIXME: It's not clear that this code ought to report an error, but the regression test is here to ensure that it does not ICE. See discussion on #74889 for details.

@JohnTitor
Copy link
Member Author

@nikomatsakis Done :)

@nikomatsakis
Copy link
Contributor

@bors r+ rollup

Thanks!

@bors
Copy link
Contributor

bors commented Aug 5, 2020

📌 Commit cc36c3d 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 Aug 5, 2020
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Aug 6, 2020
Add HRTB-related regression test

Closes rust-lang#59311 and cc rust-lang#71546
This closes the former but the test is taken from rust-lang#71546 (comment) since it seems they have the same cause and it's simplified.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Aug 6, 2020
Add HRTB-related regression test

Closes rust-lang#59311 and cc rust-lang#71546
This closes the former but the test is taken from rust-lang#71546 (comment) since it seems they have the same cause and it's simplified.
@bors
Copy link
Contributor

bors commented Aug 6, 2020

⌛ Testing commit cc36c3d with merge 4b0882c...

@bors
Copy link
Contributor

bors commented Aug 6, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: nikomatsakis
Pushing 4b0882c to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 6, 2020
@bors bors merged commit 4b0882c into rust-lang:master Aug 6, 2020
@JohnTitor JohnTitor deleted the hrtb-tests branch August 6, 2020 21:36
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE with HRTB: Broken MIR in DefId [...] could not prove Binder
6 participants