Fix ICE when Self is used in enum discriminant of a generic enum#153815
Conversation
|
rustbot has assigned @petrochenkov. Use Why was this reviewer chosen?The reviewer was selected based on:
|
7cbee2e to
c6e30c7
Compare
|
There's probably a way to implement this better. |
|
r? BoxyUwU |
|
Yeah this isn't quite what we want 🤔 The general structure here is supposed to be that name resolution rejects uses of generic parameters, and the things it can't do we reject during HIR ty lowering. Name resolution can't/doesn't reject uses of Instead of adding a new place where we check for illegal generic parameters can you look into why the existing checks in HIR ty lowering aren't working correctly for discriminant exprs. I expect that what happened is that the refactoring in #150519 was overly fit to const generics rather than all anon consts and so stopped validating stuff that it should. |
|
HIR ty lowering was modified cc @fmease |
This comment has been minimized.
This comment has been minimized.
eca8904 to
94a0b1d
Compare
This comment has been minimized.
This comment has been minimized.
94a0b1d to
b6afb25
Compare
This comment has been minimized.
This comment has been minimized.
93afad5 to
d09f2e8
Compare
2a868a9 to
30ddd00
Compare
|
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. |
This comment has been minimized.
This comment has been minimized.
30ddd00 to
87f3952
Compare
| @@ -0,0 +1,15 @@ | |||
| //@ check-fail | |||
| // Test that `Self` is rejected even when nested inside an inline const | |||
| // or closure within an enum discriminant. Regression test for issue #154281. | |||
There was a problem hiding this comment.
can you add the test for the closure case ✨
87f3952 to
16d1dcb
Compare
|
@bors squash |
This comment has been minimized.
This comment has been minimized.
* Fix ICE when Self is used in enum discriminant of a generic enum Move the validation into the existing `check_param_uses_if_mcg` machinery in HIR ty lowering instead of adding a new check in wfcheck. After the `AnonConstKind` refactoring, `ForbidMCGParamUsesFolder` was only gated on `AnonConstKind::MCG`, causing discriminant anon consts (`NonTypeSystem`) to bypass it entirely. Add `anon_const_forbids_generic_params()` which returns the appropriate `ForbidParamContext` for both MCG and enum discriminant contexts. Wire it into `check_param_uses_if_mcg` so that `Self` aliasing a generic type is caught before reaching `const_eval_poly`. Convert the `TooGeneric` span_bug into a proper diagnostic as a fallback for anything slipping through type-dependent path resolution. * Address review comments - Rename `ForbidMCGParamUsesFolder` to `ForbidParamUsesFolder` - Rename `MinConstGenerics` variant to `ConstArgument` with updated doc - Simplify doc comment on `anon_const_forbids_generic_params` - Make match on `AnonConstKind` exhaustive - Move `anon_const_def_id` inside the `if let` in `check_param_uses_if_mcg` - Remove now-unreachable `TooGeneric` span_err in wfcheck * Revert TooGeneric arm back to span_bug! as requested by reviewer * Use generics_of to determine if NonTypeSystem anon consts allow generic params * Also check InlineConst and Closure defs nested in enum discriminants * Simplify logic for determining anonymous constant parent in generic contexts * add test
|
🔨 7 commits were squashed into aacac7e. |
b2a6842 to
aacac7e
Compare
|
@bors r+ sorry for the long turnaround on reviews :) thank you for working on this |
…eric-self, r=BoxyUwU Fix ICE when Self is used in enum discriminant of a generic enum Fixes rust-lang#153756 Let discriminant AnonConst inherit parent generics via Node::Variant in generics_of, and emit a proper error instead of span_bug! for the TooGeneric case in wfcheck.
…eric-self, r=BoxyUwU Fix ICE when Self is used in enum discriminant of a generic enum Fixes rust-lang#153756 Let discriminant AnonConst inherit parent generics via Node::Variant in generics_of, and emit a proper error instead of span_bug! for the TooGeneric case in wfcheck.
…uwer Rollup of 9 pull requests Successful merges: - #153536 (Add `const_param_ty_unchecked` gate) - #153815 (Fix ICE when Self is used in enum discriminant of a generic enum) - #154882 (Gate tuple const params behind `min_adt_const_params` feature) - #155293 (fix arch names in cfg pretty printer) - #154765 (Clarify ascii whitespace exclusion of vertical tab in the doc) - #155172 (Some small nits for supertrait_item_shadowing, and additional testing) - #155279 (Test/lexer unicode pattern white space) - #155280 (Tests for precise-capture through RPIT and TAIT) - #155304 (remove PointeeParser)
This comment has been minimized.
This comment has been minimized.
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing a5c825c (parent) -> bd1e7c7 (this PR) Test differencesShow 8 test diffsStage 1
Stage 2
Additionally, 4 doctest diffs were found. These are ignored, as they are noisy. Job group index
Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard bd1e7c79485d6a4113bb11dd98cb8b415cd4a55e --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (bd1e7c7): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (secondary 2.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -0.7%, secondary 3.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.1%, secondary 0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 489.296s -> 495.503s (1.27%) |
View all comments
Fixes #153756
Let discriminant AnonConst inherit parent generics via Node::Variant in generics_of, and emit a proper error instead of span_bug! for the TooGeneric case in wfcheck.