Skip to content

Make const_lit_matches_ty check literal suffixes for exact type match#152906

Open
lapla-cogito wants to merge 1 commit intorust-lang:mainfrom
lapla-cogito:issue_152653
Open

Make const_lit_matches_ty check literal suffixes for exact type match#152906
lapla-cogito wants to merge 1 commit intorust-lang:mainfrom
lapla-cogito:issue_152653

Conversation

@lapla-cogito
Copy link
Contributor

@lapla-cogito lapla-cogito commented Feb 20, 2026

const_lit_matches_ty ignored literal suffixes. This let the try_lower_anon_const_lit fast path produce a mistyped ty::Const::Value, bypassing the type mismatch error that typeck would otherwise report.

@rustbot
Copy link
Collaborator

rustbot commented Feb 20, 2026

HIR ty lowering was modified

cc @fmease

@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 Feb 20, 2026
@rustbot
Copy link
Collaborator

rustbot commented Feb 20, 2026

r? @wesleywiser

rustbot has assigned @wesleywiser.
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

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: compiler
  • compiler expanded to 68 candidates
  • Random selection from 15 candidates

@@ -0,0 +1,10 @@
//@ compile-flags: --emit=link
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was required to perform MIR analysis.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean forcing codegen? If so check out the build-fail directive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean forcing codegen?

No, this ICE occurs when rustc reaches the MIR analysis stage despite the presence of type errors. I think the build-fail directive is inappropriate here because, IIUC, it first requires the check pass to succeed. However, this test contains type errors that cause the check pass to fail, so compiletest aborts the test before ever reaching the full build where the ICE would occur.

Comment on lines 1 to 18
error: the constant `5` is not of type `u32`
--> $DIR/type_const-mismatched-types.rs:4:1
|
LL | type const FREE: u32 = 5_usize;
| ^^^^^^^^^^^^^^^^^^^^ expected `u32`, found `usize`

error: the constant `5` is not of type `isize`
--> $DIR/type_const-mismatched-types.rs:8:1
|
LL | type const FREE2: isize = FREE;
| ^^^^^^^^^^^^^^^^^^^^^^^ expected `isize`, found `usize`

error[E0308]: mismatched types
--> $DIR/type_const-mismatched-types.rs:16:27
|
LL | type const N: usize = false;
| ^^^^^ expected `usize`, found `bool`

Copy link
Contributor Author

@lapla-cogito lapla-cogito Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix, as mentioned above, fixes the ICE while simultaneously reverting the seemingly redundant error output introduced in #152001 (in fact, this ICE was identified as originating from this PR in the original issue).

@Kivooeo
Copy link
Member

Kivooeo commented Feb 21, 2026

r? BoxyUwU

@rustbot rustbot assigned BoxyUwU and unassigned wesleywiser Feb 21, 2026
@rustbot
Copy link
Collaborator

rustbot commented Feb 21, 2026

BoxyUwU is currently at their maximum review capacity.
They may take a while to respond.

@lapla-cogito
Copy link
Contributor Author

lapla-cogito commented Feb 22, 2026

This change also fixes the newly reported issue #152962, so I’ve added a test for it.

@BoxyUwU
Copy link
Member

BoxyUwU commented Feb 24, 2026

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 24, 2026
@rustbot
Copy link
Collaborator

rustbot commented Feb 24, 2026

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@lapla-cogito
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 25, 2026
@BoxyUwU
Copy link
Member

BoxyUwU commented Feb 25, 2026

this looks good to me though im unsure how this overlaps with #153075 as that will change what type const FOO: usize = 10_u8; lowers to which might affect the tests or introduce similar ICEs as what you're fixing here 🤔

I would expect that there's a more fundamental problem here that some part of the compiler isnt expecting for a type const to normalize to something of an incorrect type. I don't think the right way to fix that is in HIR ty lowering instead we probably need to be doing error tainting better somewhere but I don't fully understand what's actually going wrong and where 🤔

We certainly want const_lit_matches_ty to behave correctly though- (which it was not before this PR)

@lapla-cogito
Copy link
Contributor Author

lapla-cogito commented Feb 25, 2026

Looking at #153075, I understand that literals will be handled as ConstArgKind::Literal (MgcaDisambiguation::Direct) instead of AnonConst, which means they will go through lower_const_arg_literal() rather than const_lit_matches_ty() via try_lower_anon_const_lit(). This means the same ICE could occur through a different path. Given that, I think there are the following options (though I'm not sure how much should be addressed in this PR 😵 ):

  • Add a const_lit_matches_ty() check in lower_const_arg_literal as well
  • Have lit_to_const() itself detect the mismatch between the suffixed literal's type and the expected type — this would also fix the ICE
  • Taint type-mismatched consts as errors, as Boxy suggests

I'd also be happy to merge the current fix as-is (with an updated commit message and with the tests added in this PR removed, since it would no longer prevent the ICE once #153075 lands) and address the deeper issue in a follow-up PR.

@rust-bors

This comment has been minimized.

@BoxyUwU
Copy link
Member

BoxyUwU commented Feb 28, 2026

Can you update your tests which used literals by themselves to actually be const { literal } which should force it to be an anon const (as it was before #153116). That should retain the ICE -> not ICE testing. I think it's fine to land this even if this ICE is still possible through other paths since your change does improve diagnostics which was the point of the check and so still has user facing impact

@BoxyUwU
Copy link
Member

BoxyUwU commented Feb 28, 2026

Taint type-mismatched consts as errors, as Boxy suggests

are you using AI? I am boxy you're talking to me

@lapla-cogito
Copy link
Contributor Author

lapla-cogito commented Feb 28, 2026

@BoxyUwU That is because I wanted to approach discussion from a more neutral perspective for two reasons: first, because multiple people are participating in this thread (though of course you're currently the primary reviewer :)); and second, because the discussion may be revisited later. For example, if I mention you but writes it that way, it would be unnatural - but that's not the case here. However, I don’t deny that I do use them. Since I’m not a native English speaker, I sometimes use them for translation purposes and to help me better understand codebases.

@rustbot
Copy link
Collaborator

rustbot commented Feb 28, 2026

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@lapla-cogito lapla-cogito changed the title Fix ICE when a suffixed literal's type doesn't match the expected const arg type Make const_lit_matches_ty check literal suffixes for exact type match Feb 28, 2026
@rust-log-analyzer

This comment has been minimized.

@lapla-cogito lapla-cogito force-pushed the issue_152653 branch 3 times, most recently from de1659f to 30c2d6b Compare February 28, 2026 21:34
@BoxyUwU
Copy link
Member

BoxyUwU commented Feb 28, 2026

@lapla-cogito ah okay I see :) no worries that makes sense

@rust-log-analyzer

This comment has been minimized.

@lapla-cogito
Copy link
Contributor Author

lapla-cogito commented Mar 1, 2026

Only x86_64-gnu-llvm-20 produces different errors that cause CI to fail, but TBH, I don't really understand the cause... 🤷

edit: While the root cause isn't entirely clear for me, the only plausible suspect is the --emit=link option added for the new test. Since this option became unnecessary for the final fix, I've decided to remove it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants