-
-
Notifications
You must be signed in to change notification settings - Fork 14.3k
Use more principled check for generics in const ops #150519
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
Conversation
|
HIR ty lowering was modified cc @fmease |
467a547 to
bb5a2ad
Compare
This comment has been minimized.
This comment has been minimized.
There was an inconsistency where in the case of array repeat expressions, we forgot to eat the `const` keyword of mgca const blocks. Thus, we ended up parsing them as an anon const containing an inline const, instead of simply an anon const as they should be. This commit fixes that issue and also makes the parsing for mgca const blocks more consistent and simpler in general. For example, we avoid doing a lookahead check to ensure there's a curly brace coming. There should always be one after a `const` keyword in an expression context, and that's handled by the mgca const block parsing function.
a43382b to
fd9687d
Compare
This comment has been minimized.
This comment has been minimized.
a9260e0 to
f8ae75a
Compare
tests/ui/const-generics/min_const_generics/self-ty-in-const-2.stderr
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
f8ae75a to
65bba87
Compare
|
This PR changes a file inside |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Instead of using a visitor in typeck, we just check, whenever lowering a use of a param, whether the parent item is an MCG anon const during hir ty lowering (and instantiate_value_path). If so, we report an error since MCG anon consts should never be able to use generics. All other kinds of anon consts are at least syntactically allowed to use generic params. We use a `TypeFolder` to accomplish this; this way, we look at the fully explicit semantic representation of the type/const/whatever and don't miss subtle cases like `Self` type aliases.
Now that we avoid a fatal error, they can all be one test.
94e79a8 to
f71e938
Compare
|
☀️ Test successful - checks-actions |
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 056908d (parent) -> a47f4df (this PR) Test differencesShow 59 test diffsStage 1
Stage 2
Additionally, 31 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 a47f4dfd7f496ed51a383c540497899f9e3f91b4 --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 (a47f4df): comparison URL. Overall result: ❌ regressions - please read the text belowOur benchmarks found a performance regression caused by this PR. Next Steps:
@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 (primary -1.9%, secondary -2.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 7.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (secondary -0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 474.6s -> 473.624s (-0.21%) |
|
Perf regression is unfortunate. I'm not sure what we can do about it 🤔 Doing more work here feels somewhat unavoidable and also necessary to fix the ICEs and generally correctly handle forbidding uses of generic parameters introduced after name resolution |
Following rust-lang#150519, the `forbid_generic` field in `Res::SelfTyAlias` is no longer needed. The check for generic `Self` types in anonymous constants is now handled by `check_param_uses_if_mcg` in HIR type lowering, making this field redundant. This removes: - The `forbid_generic` field from `Res::SelfTyAlias` - The hack in `rustc_resolve` that set `forbid_generic: true` when encountering `Self` in constant items - Related pattern matching and field propagation code
…neric-field, r=camelid,BoxyUwU Remove unneeded `forbid_generic` field from `Res::SelfTyAlias` Following rust-lang#150519, the `forbid_generic` field in `Res::SelfTyAlias` is no longer needed and can be removed. - Remove the `forbid_generic: bool` field from `Res::SelfTyAlias` - Simplify the ConstantItem rib handling in `rustc_resolve` - no longer need to mutate res to set `forbid_generic: true` - Update all pattern matches and constructors of `SelfTyAlias` Closes rust-lang#150579 r? `@camelid`
…neric-field, r=camelid,BoxyUwU Remove unneeded `forbid_generic` field from `Res::SelfTyAlias` Following rust-lang#150519, the `forbid_generic` field in `Res::SelfTyAlias` is no longer needed and can be removed. - Remove the `forbid_generic: bool` field from `Res::SelfTyAlias` - Simplify the ConstantItem rib handling in `rustc_resolve` - no longer need to mutate res to set `forbid_generic: true` - Update all pattern matches and constructors of `SelfTyAlias` Closes rust-lang#150579 r? ``@camelid``
Rollup merge of #150589 - AprilNEA:remove-unneeded-forbid-generic-field, r=camelid,BoxyUwU Remove unneeded `forbid_generic` field from `Res::SelfTyAlias` Following #150519, the `forbid_generic` field in `Res::SelfTyAlias` is no longer needed and can be removed. - Remove the `forbid_generic: bool` field from `Res::SelfTyAlias` - Simplify the ConstantItem rib handling in `rustc_resolve` - no longer need to mutate res to set `forbid_generic: true` - Update all pattern matches and constructors of `SelfTyAlias` Closes #150579 r? ``@camelid``
|
It's a tiny regression on diesel, I think that we can eat this one. @rustbot label: +perf-regression-triaged |
Fixes #144547.
Fixes #140891.
In the future, we likely want to make the check less likely to be missed by reducing the number of external entry points to HIR type lowering.
Note: If this causes pass->error regressions (not truly regressions because those cases were mistakenly accepted), they can easily be avoided for the time being by only running the check if
is_self_aliasis true or mgca is enabled.