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

Filter and test predicates using `normalize_and_test_predicates` for const-prop #68297

Merged
merged 4 commits into from Jan 21, 2020

Conversation

@Aaron1011
Copy link
Contributor

Aaron1011 commented Jan 17, 2020

Fixes #68264

Previously, I attempted to use
substitute_normalize_and_test_predicates to detect unsatisfiable
bounds. Unfortunately, since const-prop runs in a generic environment
(we don't have any of the function's generic parameters substituted),
this could lead to cycle errors when attempting to normalize predicates.

This check is replaced with a more precise check. We now only call
normalize_and_test_predicates on predicates that have the possibility
of being proved unsatisfiable - that is, predicates that don't depend
on anything local to the function (e.g. generic parameters). This
ensures that we don't hit cycle errors when we normalize said
predicates, while still ensuring that we detect unsatisfiable
predicates.

I haven't been able to come up with a minimization of the Diesel issue - however, I've verified that it compiles successfully.

Aaron1011 added 2 commits Jan 16, 2020
…const-prop

Fixes #68264

Previously, I attempted to use
`substitute_normalize_and_test_predicates` to detect unsatisfiable
bounds. Unfortunately, since const-prop runs in a generic environment
(we don't have any of the function's generic parameters substituted),
this could lead to cycle errors when attempting to normalize predicates.

This check is replaced with a more precise check. We now only call
`normalize_and_test_predicates` on predicates that have the possibility
of being proved unsatisfiable - that is, predicates that don't depend
on anything local to the function (e.g. generic parameters). This
ensures that we don't hit cycle errors when we normalize said
predicates, while still ensuring that we detect unsatisfiable
predicates.
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 17, 2020

r? @petrochenkov

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

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 17, 2020

Not my area of expertise.
r? @oli-obk because const-prop, feel free to reassign.

src/librustc_mir/transform/const_prop.rs Outdated Show resolved Hide resolved
src/librustc_mir/transform/const_prop.rs Show resolved Hide resolved
.iter()
.filter_map(|(p, _)| if p.is_global() { Some(*p) } else { None })
.collect();
if !traits::normalize_and_test_predicates(

This comment has been minimized.

Copy link
@oli-obk

oli-obk Jan 17, 2020

Contributor

why call the function and not the query here?

This comment has been minimized.

Copy link
@Aaron1011

Aaron1011 Jan 17, 2020

Author Contributor

I didn't realize this was a query, lol.

EDIT: This isn't a query. Did you want me to query-ify this?

This comment has been minimized.

Copy link
@oli-obk

oli-obk Jan 17, 2020

Contributor

well there's substitute_normalize_and_test_predicates which you used before.

This comment has been minimized.

Copy link
@Aaron1011

Aaron1011 Jan 17, 2020

Author Contributor

@oli-obk: That would use all of the predicates, which I needed to filter beforehand.

This comment has been minimized.

Copy link
@oli-obk

oli-obk Jan 17, 2020

Contributor

Oh... sorry for not reading the docs properly. You're totally right. Let's merge this PR for now and experiment with making it a query separately. Such a query could share a lot of evaluation time between the predicate elaborations of completely independent definitions if they have the same predicate list.

Fix typo
Co-Authored-By: Oliver Scherer <github35764891676564198441@oli-obk.de>
@weiznich

This comment has been minimized.

Copy link
Contributor

weiznich commented Jan 17, 2020

It probably makes sense to add the test case I've provided in #68264 as regression test?

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

Aaron1011 commented Jan 17, 2020

@weiznich: Added

}
}

fn main() {}

This comment has been minimized.

Copy link
@weiznich

weiznich Jan 17, 2020

Contributor

The overflow only happens if the current crate is a library. It does not happen if it's a binary crate (containing a main function). That means this test tests something different.

This comment has been minimized.

Copy link
@Aaron1011

Aaron1011 Jan 17, 2020

Author Contributor

Passing --emit=mir causes the issue to occur for a binary crate, since it forces optimized_mir (and therefore const-prop) to get run on the 'bad' function.

You can verify this locally with rustc +nightly src/test/ui/consts/issue-68264-overflow.rs --emit=mir

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

Aaron1011 commented Jan 19, 2020

@oli-obk: Are there any other changes that you'd like me to make?

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Jan 20, 2020

Nope, this just fell of my radar. Thanks for the ping

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 20, 2020

📌 Commit 3fef3d8 has been approved by oli-obk

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jan 20, 2020
…, r=oli-obk

 Filter and test predicates using `normalize_and_test_predicates` for const-prop

Fixes rust-lang#68264

Previously, I attempted to use
`substitute_normalize_and_test_predicates` to detect unsatisfiable
bounds. Unfortunately, since const-prop runs in a generic environment
(we don't have any of the function's generic parameters substituted),
this could lead to cycle errors when attempting to normalize predicates.

This check is replaced with a more precise check. We now only call
`normalize_and_test_predicates` on predicates that have the possibility
of being proved unsatisfiable - that is, predicates that don't depend
on anything local to the function (e.g. generic parameters). This
ensures that we don't hit cycle errors when we normalize said
predicates, while still ensuring that we detect unsatisfiable
predicates.

I haven't been able to come up with a minimization of the Diesel issue - however, I've verified that it compiles successfully.
bors added a commit that referenced this pull request Jan 20, 2020
Rollup of 8 pull requests

Successful merges:

 - #67734 (Remove appendix from Apache license)
 - #67795 (Cleanup formatting code)
 - #68290 (Fix some tests failing in `--pass check` mode)
 - #68297 ( Filter and test predicates using `normalize_and_test_predicates` for const-prop)
 - #68302 (Fix #[track_caller] and function pointers)
 - #68339 (Add `riscv64gc-unknown-linux-gnu` into target list in build-manifest)
 - #68381 (Added minor clarification to specification of GlobalAlloc::realloc.)
 - #68397 (rustdoc: Correct order of `async` and `unsafe` in `async unsafe fn`s)

Failed merges:

r? @ghost
@bors bors merged commit 3fef3d8 into rust-lang:master Jan 21, 2020
4 checks passed
4 checks passed
pr Build #20200117.25 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.