Skip to content

Defaulted const generic parameters no longer have the const arg's type checked #139643

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

Closed
BoxyUwU opened this issue Apr 10, 2025 · 5 comments · Fixed by #139646
Closed

Defaulted const generic parameters no longer have the const arg's type checked #139643

BoxyUwU opened this issue Apr 10, 2025 · 5 comments · Fixed by #139646
Assignees
Labels
A-const-generics Area: const generics (parameters and arguments) C-bug Category: This is a bug. F-const_generics_defaults `#![feature(const_generics_defaults)]` P-low Low priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-types Relevant to the types team, which will review and decide on the PR/issue.

Comments

@BoxyUwU
Copy link
Member

BoxyUwU commented Apr 10, 2025

Code

I tried this code:

struct Foo<const N: u32, const M: u64 = N>;

I expected to see this happen: It fail to compile like it uised to

Instead, this happened: it compile :>

Probably regressed in #125915 where we started lowering const arguments to ConstArgKind::Path instead of ConstArgKind::Anon so there is no longer implicitly an anon const with an expected type of u64 with an expr typed as u32.

We could probably special case handling of types of defaulted const generics in wfcheck to be done even when generics are involved. Or just accept this as working code which is consistent with things like struct Foo<T, U = <T as Iterator>::Item>(T, U); 🤷‍♀ would need a types fcp for that tho

I don't believe this to be a soundness bug

@BoxyUwU BoxyUwU added A-const-generics Area: const generics (parameters and arguments) C-bug Category: This is a bug. F-const_generics_defaults `#![feature(const_generics_defaults)]` regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-types Relevant to the types team, which will review and decide on the PR/issue. labels Apr 10, 2025
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Apr 10, 2025
@BoxyUwU BoxyUwU added P-low Low priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Apr 10, 2025
@BoxyUwU
Copy link
Member Author

BoxyUwU commented Apr 10, 2025

P-low because it's not going to regress anyone, and its not unsound, and is potentially just a bug fix and is in line with how defaulted generic parameters work in general. cc @rust-lang/types to see if anyone feels strongly about not just accepting this code

@compiler-errors
Copy link
Member

Or just accept this as working code which is consistent with things like struct Foo<T, U = <T as Iterator>::Item>(T, U);

Yeah, we already don't check the well-formedness of default types, so I think not checking the well-formedness of default consts is inevitable here.

@lcnr
Copy link
Contributor

lcnr commented Apr 10, 2025

I feel somewhat strongly about it.

We currently have the approach of: if the default is fully concrete, check its bounds. I would like to extend this to if the type of the default is fully concrete, check ConstArgHasType for it

@compiler-errors
Copy link
Member

I would like to extend this to if the type of the default is fully concrete, check ConstArgHasType for it

We already do that, I think?

struct Foo<const N: u32, const M: u64 = "">;
error[E0308]: mismatched types
 --> src/lib.rs:1:41
  |
1 | struct Foo<const N: u32, const M: u64 = "">;
  |                                         ^^ expected `u64`, found `&str`

For more information about this error, try `rustc --explain E0308`.

@lcnr
Copy link
Contributor

lcnr commented Apr 10, 2025

No, we check ConstArgHasType if the const itself is concrete. This issue is that for generic constants with concrete types, we don't check ConstArgHasType for that concrete type

@lcnr lcnr self-assigned this Apr 10, 2025
bors added a commit to rust-lang-ci/rust that referenced this issue Apr 11, 2025
check types of const param defaults

fixes rust-lang#139643 by checking that the type of a const parameter default matches the type of the parameter as long as both types are fully concrete

r? `@BoxyUwU`
tgross35 added a commit to tgross35/rust that referenced this issue Apr 27, 2025
…BoxyUwU

check types of const param defaults

fixes rust-lang#139643 by checking that the type of a const parameter default matches the type of the parameter as long as both types are fully concrete

r? `@BoxyUwU`
bors added a commit to rust-lang-ci/rust that referenced this issue Apr 27, 2025
…xyUwU

check types of const param defaults

fixes rust-lang#139643 by checking that the type of a const parameter default matches the type of the parameter as long as both types are fully concrete

r? `@BoxyUwU`
tgross35 added a commit to tgross35/rust that referenced this issue Apr 27, 2025
…BoxyUwU

check types of const param defaults

fixes rust-lang#139643 by checking that the type of a const parameter default matches the type of the parameter as long as both types are fully concrete

r? `@BoxyUwU`
@bors bors closed this as completed in 496145b Apr 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-generics Area: const generics (parameters and arguments) C-bug Category: This is a bug. F-const_generics_defaults `#![feature(const_generics_defaults)]` P-low Low priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants