CTFE: avoid emitting a hard error on generic normalization failures#149501
CTFE: avoid emitting a hard error on generic normalization failures#149501bors merged 2 commits intorust-lang:mainfrom
Conversation
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
😢 |
|
@rust-lang/types would like to beta backport this as early as possible ideally. Can I get one of you to quickly look at this PR? |
In the long term I think this is the right fix 🤔 Before this PR were we getting similar kinds of ICEs due to opaque types? I was looking at the doc comments for I'm not sure I feel confident in being able to judge whether there are any weird side effects from this PR or not. I'd certainly feel better to revert #147793 and then land this "normally" or if it really will be the near future that we fix #149283 then land that and then re-land #147793 |
|
Good catch @BoxyUwU, there are apparently other "not quite a bug" reasons for normalization failures in well-formed programs. This PR causes us to also weaken CTFE errors due to param-env shadowing from a hard-error to simply returning Now, not erroring in these cases also feels desirable to me. However, this is something I did not consider when writing this PR. More thoughts on this later |
soon-ish? no it depends on whether somebody ends up working on this, it'd probably take a few months to get something working here (with mentoring) for a somewhat experienced contributor. |
|
@bors r=lqd,oli-obk |
|
💡 This pull request was already approved, no need to approve it again.
|
…, r=lqd,oli-obk CTFE: avoid emitting a hard error on generic normalization failures Fixes rust-lang#149081. The fix is quite unsatisfying and should not be necessary, cc rust-lang#149283. That change is significantly more involved. This temporary fix introduces some unnecessary complexity and may hide other type system bugs. cc `@rust-lang/types` I think we should try to fix issue rust-lang#149283 in the near future and then remove this hack again. I originally intended a more targeted fix. I wanted to skip evaluating constants in MIR opts if their body was polymorphic and the current generic arguments still reference generic parameters. Notes from looking into this: - we only fetch the MIR in the `eval_to_allocation_raw` query - figuring out which MIR to use is hard - dealing with trivial consts is annoying - need to resolve instances for associated consts - implementing this by hand is hard - inlining handles this issue by bailing on literally all normalization failures, even the ones that imply an unsoundness - `try_normalize_after_erasing_regions` generally does two things - deal with ambiguity after inlining - deal with error tainting issues (please don't, we should stop doing that) - CTFE could be changed to always silently ignore normalization failures if we're in a generic body - hides actual bugs <- went with this option r? types
Rollup of 9 pull requests Successful merges: - #147841 (Fix ICE when applying test macro to crate root) - #149501 (CTFE: avoid emitting a hard error on generic normalization failures) - #149517 (Implement blessing for tidy alphabetical check) - #149521 (Improve `io::Error::downcast`) - #149545 (fix the check for which expressions read never type) - #149549 (Regression test for system register `ttbr0_el2`) - #149579 (Motor OS: fix compile error) - #149595 (Tidying up `tests/ui/issues` tests [2/N]) - #149597 (Revert "implement and test `Iterator::{exactly_one, collect_array}`") r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 9 pull requests Successful merges: - #147841 (Fix ICE when applying test macro to crate root) - #149501 (CTFE: avoid emitting a hard error on generic normalization failures) - #149517 (Implement blessing for tidy alphabetical check) - #149521 (Improve `io::Error::downcast`) - #149545 (fix the check for which expressions read never type) - #149549 (Regression test for system register `ttbr0_el2`) - #149579 (Motor OS: fix compile error) - #149595 (Tidying up `tests/ui/issues` tests [2/N]) - #149597 (Revert "implement and test `Iterator::{exactly_one, collect_array}`") r? `@ghost` `@rustbot` modify labels: rollup
…, r=lqd,oli-obk CTFE: avoid emitting a hard error on generic normalization failures Fixes rust-lang#149081. The fix is quite unsatisfying and should not be necessary, cc rust-lang#149283. That change is significantly more involved. This temporary fix introduces some unnecessary complexity and may hide other type system bugs. cc ``@rust-lang/types`` I think we should try to fix issue rust-lang#149283 in the near future and then remove this hack again. I originally intended a more targeted fix. I wanted to skip evaluating constants in MIR opts if their body was polymorphic and the current generic arguments still reference generic parameters. Notes from looking into this: - we only fetch the MIR in the `eval_to_allocation_raw` query - figuring out which MIR to use is hard - dealing with trivial consts is annoying - need to resolve instances for associated consts - implementing this by hand is hard - inlining handles this issue by bailing on literally all normalization failures, even the ones that imply an unsoundness - `try_normalize_after_erasing_regions` generally does two things - deal with ambiguity after inlining - deal with error tainting issues (please don't, we should stop doing that) - CTFE could be changed to always silently ignore normalization failures if we're in a generic body - hides actual bugs <- went with this option r? types
…, r=lqd,oli-obk CTFE: avoid emitting a hard error on generic normalization failures Fixes rust-lang#149081. The fix is quite unsatisfying and should not be necessary, cc rust-lang#149283. That change is significantly more involved. This temporary fix introduces some unnecessary complexity and may hide other type system bugs. cc ```@rust-lang/types``` I think we should try to fix issue rust-lang#149283 in the near future and then remove this hack again. I originally intended a more targeted fix. I wanted to skip evaluating constants in MIR opts if their body was polymorphic and the current generic arguments still reference generic parameters. Notes from looking into this: - we only fetch the MIR in the `eval_to_allocation_raw` query - figuring out which MIR to use is hard - dealing with trivial consts is annoying - need to resolve instances for associated consts - implementing this by hand is hard - inlining handles this issue by bailing on literally all normalization failures, even the ones that imply an unsoundness - `try_normalize_after_erasing_regions` generally does two things - deal with ambiguity after inlining - deal with error tainting issues (please don't, we should stop doing that) - CTFE could be changed to always silently ignore normalization failures if we're in a generic body - hides actual bugs <- went with this option r? types
Rollup of 12 pull requests Successful merges: - #147841 (Fix ICE when applying test macro to crate root) - #149147 (Fix unused_assignments false positives from macros) - #149183 (Use `TypingMode::PostAnalysis` in `try_evaluate_const`) - #149456 (std: don't call `current_os_id` from signal handler) - #149501 (CTFE: avoid emitting a hard error on generic normalization failures) - #149528 (reword error for invalid range patterns) - #149539 (Additional test for uN::{gather,scatter}_bits) - #149549 (Regression test for system register `ttbr0_el2`) - #149550 (Disable native-lib for x check miri) - #149554 (build-manifest: generate MSI and MINGW arrays from rustc) - #149557 (c-variadic: bpf and spirv do not support c-variadic definitions) - #149569 (Fix mailmap issue) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #149501 - lcnr:no-hard-error-on-norm-failure, r=lqd,oli-obk CTFE: avoid emitting a hard error on generic normalization failures Fixes #149081. The fix is quite unsatisfying and should not be necessary, cc #149283. That change is significantly more involved. This temporary fix introduces some unnecessary complexity and may hide other type system bugs. cc ````@rust-lang/types```` I think we should try to fix issue #149283 in the near future and then remove this hack again. I originally intended a more targeted fix. I wanted to skip evaluating constants in MIR opts if their body was polymorphic and the current generic arguments still reference generic parameters. Notes from looking into this: - we only fetch the MIR in the `eval_to_allocation_raw` query - figuring out which MIR to use is hard - dealing with trivial consts is annoying - need to resolve instances for associated consts - implementing this by hand is hard - inlining handles this issue by bailing on literally all normalization failures, even the ones that imply an unsoundness - `try_normalize_after_erasing_regions` generally does two things - deal with ambiguity after inlining - deal with error tainting issues (please don't, we should stop doing that) - CTFE could be changed to always silently ignore normalization failures if we're in a generic body - hides actual bugs <- went with this option r? types
|
Beta backport accepted as per compiler team on Zulip. A backport PR will be authored by the release team at the end of the current development cycle. Backport labels handled by them. Note: before moving on with the backport @wesleywiser will investigate if we can revert the culprit PR first (comment). So this backport is approved but in stand-by. We will comment later on. EDIT: A revert PR has been done in #149649 and it looks a solution we feel more confident in. So I'll move the beta backport approval there. @rustbot label -beta-nominated -beta-accepted |
|
Besides everything, thank you so much @lcnr for your work on this!! |
Rollup of 12 pull requests Successful merges: - rust-lang/rust#147841 (Fix ICE when applying test macro to crate root) - rust-lang/rust#149147 (Fix unused_assignments false positives from macros) - rust-lang/rust#149183 (Use `TypingMode::PostAnalysis` in `try_evaluate_const`) - rust-lang/rust#149456 (std: don't call `current_os_id` from signal handler) - rust-lang/rust#149501 (CTFE: avoid emitting a hard error on generic normalization failures) - rust-lang/rust#149528 (reword error for invalid range patterns) - rust-lang/rust#149539 (Additional test for uN::{gather,scatter}_bits) - rust-lang/rust#149549 (Regression test for system register `ttbr0_el2`) - rust-lang/rust#149550 (Disable native-lib for x check miri) - rust-lang/rust#149554 (build-manifest: generate MSI and MINGW arrays from rustc) - rust-lang/rust#149557 (c-variadic: bpf and spirv do not support c-variadic definitions) - rust-lang/rust#149569 (Fix mailmap issue) r? `@ghost` `@rustbot` modify labels: rollup
Fixes #149081.
The fix is quite unsatisfying and should not be necessary, cc #149283. That change is significantly more involved. This temporary fix introduces some unnecessary complexity and may hide other type system bugs.
cc @rust-lang/types I think we should try to fix issue #149283 in the near future and then remove this hack again.
I originally intended a more targeted fix. I wanted to skip evaluating constants in MIR opts if their body was polymorphic and the current generic arguments still reference generic parameters. Notes from looking into this:
eval_to_allocation_rawquerytry_normalize_after_erasing_regionsgenerally does two thingsr? types