Skip to content

Fix ICE on invalid const param types#124394

Merged
bors merged 1 commit intorust-lang:masterfrom
gurry:123863-ice-unexpected-region
Apr 27, 2024
Merged

Fix ICE on invalid const param types#124394
bors merged 1 commit intorust-lang:masterfrom
gurry:123863-ice-unexpected-region

Conversation

@gurry
Copy link
Contributor

@gurry gurry commented Apr 26, 2024 •
edited
Loading

Fixes ICE #123863 which occurs because the const param has a type which is not a bool, char or an integral type.

The ICEing code path begins here in typeck_with_fallback:

rust/compiler/rustc_hir_typeck/src/lib.rs

Line 167 in cb3752d

let expected_type = expected_type.unwrap_or_else(fallback);

The fallback invokes the type_of query and that eventually ends up calling ct_infer from the lowering code over here:

rust/compiler/rustc_hir_analysis/src/hir_ty_lowering/mod.rs

Line 561 in cb3752d

self.lowerer.ct_infer(ty, Some(param), self.span).into()
and ct_infer ICEs at this location:

rust/compiler/rustc_hir_analysis/src/collect.rs

Line 392 in cb3752d

r => bug!("unexpected region: {r:?}"),

To fix the ICE it I'm triggering a span_delayed_bug before we hit ct_infer if the type of the const param is not one of the supported types

Edit

On @lcnr's suggestion I've changed the approach to not let ReStatic region hit the bug! in ct_infer instead of triggering a span_delayed_bug.

@rustbot
Copy link
Collaborator

rustbot commented Apr 26, 2024

r? @Nadrieril

rustbot has assigned @Nadrieril.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 26, 2024
@gurry
Copy link
Contributor Author

gurry commented Apr 26, 2024 •
edited
Loading

I need help with this fix.

It currently does not cater to the case when the feature adt_const_params is enabled because I am not sure how I should handle that. When we do a similar const param type check during WF we handle adt_const_param by running a proper WF check logic over here:

rust/compiler/rustc_hir_analysis/src/check/wfcheck.rs

Lines 938 to 954 in cb3752d

if tcx.features().adt_const_params {
enter_wf_checking_ctxt(tcx, hir_ty.span, param.def_id, |wfcx| {
let trait_def_id =
tcx.require_lang_item(LangItem::ConstParamTy, Some(hir_ty.span));
wfcx.register_bound(
ObligationCause::new(
hir_ty.span,
param.def_id,
ObligationCauseCode::ConstParam(ty),
),
wfcx.param_env,
ty,
trait_def_id,
);
Ok(())
})
} else {

But we can't call WF from the location in this fix because that will lead to cycles (typeck -> type_of -> fix location -> wf -> typeck).

@Nadrieril Can you please suggest a solution to this conundrum?

@Nadrieril
Copy link
Member

Nadrieril commented Apr 26, 2024

I'm afraid I know very little about typeck. Let's see if someone else can help

r? compiler

@rustbot rustbot assigned lcnr and unassigned Nadrieril Apr 26, 2024
@lcnr
Copy link
Contributor

lcnr commented Apr 26, 2024

please add the affected test to you PR

@lcnr
Copy link
Contributor

lcnr commented Apr 26, 2024

i think the correct fix is to change

rust/compiler/rustc_hir_analysis/src/collect.rs

Lines 389 to 392 in 9adafa7

// This is never reached in practice. If it ever is reached,
// `ReErased` should be changed to `ReStatic`, and any other region
// left alone.
r => bug!("unexpected region: {r:?}"),
to keep 'static as is and not bug! there. Please try thatr

@gurry gurry force-pushed the 123863-ice-unexpected-region branch from b5ef239 to c62bc31 Compare April 27, 2024 04:21
@gurry gurry marked this pull request as ready for review April 27, 2024 05:45
@gurry
Copy link
Contributor Author

gurry commented Apr 27, 2024

i think the correct fix is to change

rust/compiler/rustc_hir_analysis/src/collect.rs

Lines 389 to 392 in 9adafa7

// This is never reached in practice. If it ever is reached,
// `ReErased` should be changed to `ReStatic`, and any other region
// left alone.
r => bug!("unexpected region: {r:?}"),

to keep 'static as is and not bug! there. Please try thatr

Thanks @lcnr. I've implemented your suggestion and it has fixed the ICE.

@lcnr
Copy link
Contributor

lcnr commented Apr 27, 2024

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Apr 27, 2024

📌 Commit c62bc31 has been approved by lcnr

It is now in the queue for this repository.

@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 Apr 27, 2024
@matthiaskrgr matthiaskrgr mentioned this pull request Apr 27, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 27, 2024
@bors
Auto merge of rust-lang#124448 - matthiaskrgr:rollup-iloy9vz, r=matth…
aed2187 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#124370 (Fix substitution parts having a shifted underline in some cases)
 - rust-lang#124394 (Fix ICE on invalid const param types)
 - rust-lang#124425 (Do not ICE on invalid consts when walking mono-reachable blocks)
 - rust-lang#124434 (Remove lazycell and once_cell from compiletest dependencies)
 - rust-lang#124437 (doc: Make the `mod.rs` in the comment point to the correct location)
 - rust-lang#124443 (Elaborate in comment about `statx` probe)
 - rust-lang#124445 (bootstrap: Change `global(true)` to `global = true` for flags for consistency)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit aeb4c04 into rust-lang:master Apr 27, 2024
@rustbot rustbot added this to the 1.79.0 milestone Apr 27, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 27, 2024
@rust-timer
Unrolled build for rust-lang#124394
3140d7f 
Rollup merge of rust-lang#124394 - gurry:123863-ice-unexpected-region, r=lcnr

Fix ICE on invalid const param types

Fixes ICE rust-lang#123863 which occurs because the const param has a type which is not a `bool`, `char` or an integral type.

The ICEing code path begins here in `typeck_with_fallback`: https://github.com/rust-lang/rust/blob/cb3752d20e0f5d24348062211102a08d46fbecff/compiler/rustc_hir_typeck/src/lib.rs#L167

The `fallback` invokes the `type_of` query and that eventually ends up calling `ct_infer` from the lowering code over here:
https://github.com/rust-lang/rust/blob/cb3752d20e0f5d24348062211102a08d46fbecff/compiler/rustc_hir_analysis/src/hir_ty_lowering/mod.rs#L561 and `ct_infer` ICEs at this location: https://github.com/rust-lang/rust/blob/cb3752d20e0f5d24348062211102a08d46fbecff/compiler/rustc_hir_analysis/src/collect.rs#L392

To fix the ICE it I'm triggering a `span_delayed_bug` before we hit `ct_infer` if the type of the const param is not one of the supported types

### Edit
On `@lcnr's` suggestion I've changed the approach to not let `ReStatic` region hit the `bug!` in `ct_infer` instead of triggering a `span_delayed_bug`.
@gurry gurry deleted the 123863-ice-unexpected-region branch April 28, 2024 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@lcnr lcnr

Labels

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.79.0

Development

Successfully merging this pull request may close these issues.

5 participants

@gurry @rustbot @Nadrieril @lcnr @bors