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

Follow up on const_eval.rs check_match call #59378

Closed
estebank opened this issue Mar 23, 2019 · 4 comments · Fixed by #59781
Closed

Follow up on const_eval.rs check_match call #59378

estebank opened this issue Mar 23, 2019 · 4 comments · Fixed by #59781
Labels
A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@estebank
Copy link
Contributor

#59199 (comment)

@estebank estebank added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) labels Mar 23, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Mar 25, 2019

Super-High-Level Instructions:

Replace https://github.com/rust-lang/rust/blob/master/src/librustc_mir/const_eval.rs#L617-L632 with

if def_id.is_local() && tcx.typeck_tables_of(def_id).tainted_by_errors {
    return Err(ErrorHandled::Reported);
}

And then follow up by making sure that the compiler doesn't ICE anywhere (e.g. during MIR building). It should probably "just work" nowadays. A crater run isn't necessary, as this will only ICE if an error wasn't handled properly. Successfully compiling code is unaffected.

@oli-obk oli-obk added the E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. label Mar 25, 2019
@whitfin
Copy link
Contributor

whitfin commented Apr 7, 2019

@oli-obk I took a look at this and all tests seem fine in librustc_mir just dropping in the code you have above as you said.

However I just wanted to check https://github.com/rust-lang/rust/blob/master/src/librustc_mir/const_eval.rs#L625-L627 is no longer needed? Pretty unfamiliar with this code so I wanted to confirm before submitting as it's not explicitly mentioned in the other thread that it should be removed.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 7, 2019

Yes that should be removed, too. It will automatically be run when fetching the MIR of the current constant.

@whitfin
Copy link
Contributor

whitfin commented Apr 7, 2019

@oli-obk ok, cool! I have filed a PR for this in #59781. Let me know if anything needs to be updated!

Centril added a commit to Centril/rust that referenced this issue Apr 12, 2019
Remove check_match from const_eval

This fixes rust-lang#59378.

It seems that the `check_match` may be unnecessary, so this removes it per instructions provided in the issue. I re-ran the tests for `librustc_mir` and everything seemed fine!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants