Fix - const parameters rejected when identical by Jamesbarford · Pull Request #162908 · rust-lang/rust

Jamesbarford · 2026-09-17T14:56:25Z

This keeps a series of cheaper checks before doing a more heavyweight comparison. Used the example in the issue as the test case.

r? @khyperia

Fixes #162897

khyperia · 2026-09-18T09:38:47Z

Hmm, this is definitely a good step in the correct direction, but I think it might be nice to fully resolve this underlying issue rather than just doing the first step. In particular, I think the equality check here ought to be a full trait solver equality comparison - stuff like this ought to compile (currently, it does not):

const FOO: usize = core::direct_const_arg!(5); // I thiiink a regular const ought to work as well, maybe(?)
const BAR: usize = core::direct_const_arg!(5);

pub trait Trait {
    fn method<const A: [usize; FOO]>();
}

pub struct Foo;

impl Trait for Foo {
    fn method<const A: [usize; BAR]>() {}
}

(and then more complex scenarios involving type aliases, const aliases, generic aliases, and the like, too)

Other places in this file create a new ObligationCtxt and do things with them, should be able to base things off that to be able to eventually do something like a ObligationCtxt::eq.

Also, I don't think perf stuff is super necessary to do here - should be fine to just unconditionally create a new ObligationCtxt and whatnot (ObligationCtxt::eq probably already has a fast path at some point for passing in the same type, or something).

Also, minor code style nit thing, probably good to just remove the if guard from the match statement, and handle all the const stuff inside this (Const { .. }, Const { .. }) branch

Feel free to poke me on zulip if you have questions!

@rustbot author

rustbot · 2026-09-18T09:39:06Z

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

Jamesbarford · 2026-09-18T15:05:33Z

I've added that as another test case (happy to add more if there are some obvious ones 😃 ). I've phased the check so hopefully some fast paths will still be hit. This heavily borrows ideas from the surrounding code in other functions.

Rather than try and split bits of what I've done into other functions or refactor the surrounding code so I could call helpers, it's deliberately a fairly hefty lambda that I think reads fairly well from top to bottom.

Though I'm happy to gut some of the borrowed logic into separate functions if you think that a good idea in this PR?

@rustbot ready

khyperia

This is turning into a bit of a mess. I'm wondering if it might be good to split this into two functions: compare_generic_param_kinds compares the, well, kinds of the generic params (only checks if types are matched with types, consts are matched with consts, etc.) and then add another compare_generic_const_param_types (called in whatever method after compare_generic_param_kinds is called) that actually drills into the types of the consts.

View changes since this review

khyperia · 2026-09-24T07:12:42Z

+
+                match ocx.eq(&cause, param_env, trait_ty, impl_ty) {
+                    Ok(_) => {
+                        let errors = ocx.evaluate_obligations_error_on_ambiguity();


If the ocx is created outside the loop, evaluate_obligations_error_on_ambiguity probably could/should be called at the end of the loop, not after every eq. (It's technically correct, but unnecessary, to do it here)

khyperia · 2026-09-24T07:13:00Z

+                        infcx.err_ctxt().note_type_err(
+                            &mut diag,
+                            &cause,
+                            Some((param_impl_span, Cow::from("type in trait"), false)),


Should this be param_trait_span, not param_impl_span?

Jamesbarford · 2026-09-24T14:08:49Z

This is turning into a bit of a mess. I'm wondering if it might be good to split this into two functions: compare_generic_param_kinds compares the, well, kinds of the generic params (only checks if types are matched with types, consts are matched with consts, etc.) and then add another compare_generic_const_param_types (called in whatever method after compare_generic_param_kinds is called) that actually drills into the types of the consts.

View changes since this review

I've tried splitting this into separate functions a couple of times, but the extracted const-param logic ends up needing quite a lot of surrounding context. For example, the helper ends up looking roughly like:

fn const_helper_fn(
    ocx: &ObligationCtxt,
    tcx: TyCtxt<'tcx>,
    impl_item: ty::AssocItem,
    impl_trait: ty::AssocItem,
    trait_to_impl_args: &ty::GenericArgs,
    param_impl: &GenericParamDef,
    param_trait: &GenericParamDef,
) -> Result<(), ErrorGuaranteed> {
    // Roughly the current `(Const { .. }, Const { .. }) => {}` branch.
}

The alternative I considered was putting ocx, tcx, impl_item, impl_trait, and trait_to_impl_args into an intermediary context struct, leaving the two params as arguments.

Neither option felt like it made the code substantially easier to follow; it mostly seemed to move the complexity elsewhere.

A fair amount of the logic here is also heavily based on similar code in other functions. I'm slightly wary of extracting helpers that are only useful at this one call site when there may instead be an opportunity to factor out something genuinely reusable across those implementations.

Would you be OK with keeping this together for this PR, and doing a follow-up that looks at extracting reusable utilities and reducing the duplicated logic?

khyperia · 2026-09-25T05:29:22Z

For example, the helper ends up looking roughly like

Apologies for not being clear! That's not quite what I meant.

There are two things being checked here in a single function right now:

checking that the kinds of generic parameters match up between the trait and the impl
checking that the types of const generic parameters match up

I was suggesting these two checks be two different functions instead of being combined into one. The one that checks the kinds of generic parameters (i.e. whether the GenericParamDefKind variant matches up for each parameter) could be called compare_generic_param_kinds. The second could be called compare_generic_const_param_types or something.

The diff in this PR to compare_generic_param_kinds would be a very basic deletion of the check for the types. Then, an additional function would be added, with this signature:

fn compare_generic_const_param_types<'tcx>(
    tcx: TyCtxt<'tcx>,
    impl_item: ty::AssocItem,
    trait_item: ty::AssocItem,
    impl_trait_ref: ty::TraitRef<'tcx>,
    delay: bool,
) -> Result<(), ErrorGuaranteed>;

This new function would be called in the same places compare_generic_param_kinds is called (presumably after, so that it can assume that the generic param kinds have already been checked and match up).

Conflating these two operations (checking the kinds of generic parameters, and checking the types of const generic parameters) seems to be producing very messy code, as evidenced by the current PR being quite difficult and headache-inducing to follow.

Would you be OK with keeping this together for this PR, and doing a follow-up that looks at extracting reusable utilities and reducing the duplicated logic?

Hm, I'm not quite sure I follow - I'm definitely not suggesting extracting helpers to deduplicate code, this suggestion indeed creates a bit more code duplication. However, my guess would be that code is significantly easier to follow.

In any case, sure, it was a suggestion that I'm not sure about, I don't feel particularly strongly about it.

khyperia

heck yeah, thanks for bearing with me to get this PR into shape! ❤️ - looking super good, sorry for the bunch of nits! biggest thing is that it'd be cute IMO to use ty_span rather than def_span for the error message.

View changes since this review

khyperia · 2026-09-30T05:50:44Z

+                    tcx.dcx(),
+                    param_impl_span,
+                    E0053,
+                    "{} `{}` has an incompatible generic parameter for trait `{}`",


nit: I would prefer this to specifically talk about the type of const generic parameters, rather than reusing the message from compare_generic_param_kinds. perhaps just adding the word const:

"{} {} has an incompatible const generic parameter for trait {}"

khyperia · 2026-09-30T06:00:06Z

+            Ok(_) => {}
+            Err(terr) => {
+                let param_impl_span = tcx.def_span(param_impl.def_id);
+                let param_trait_span = tcx.def_span(param_trait.def_id);


I think it would be nice if we use ty_span here instead of def_span. Here's my thoughts:

extract out let param_impl_ty_span = tcx.ty_span(param_impl.def_id.expect_local()); to the top of this for-loop

use it for the span in ObligationCause::new

also use it here

for the trait side, it is not necessarily local, so do let param_trait_ty_span = param_trait.def_id.as_local().map(|def_id| tcx.ty_span(def_id));

pass in None to note_type_err's secondary_span if the trait is nonlocal, i.e. param_trait_ty_span.map(|span| (span, Cow::from("type in trait"), false))

(I'm basing this a bit off of what compare_const_clause_entailment does)

perhaps we want a test with an err in an impl for a nonlocal trait too, if one doesn't already exist. also I'm unsure of the behavior of ty_span when the type is malformed, e.g. <const N: > or <const N> so maybe a test for that too? but we might not even get to this point if the type is borked, unsure.

khyperia · 2026-09-30T06:05:22Z

+            tcx.type_of(param_trait.def_id).instantiate(tcx, trait_to_impl_args),
+        );
+
+        match ocx.eq(&cause, param_env, trait_ty, impl_ty) {


@BoxyUwU I just realized that compare_const_clause_entailment does an ocx.sup here, not ocx.eq. Do we want ocx.eq here or ocx.sub? (for future thoughts of const generic references and whatnot - I suppose we could start with eq and expand later if desired)

relatedly, compare_const_clause_entailment does both evaluate_obligations_error_on_ambiguity and resolve_regions_and_report_errors at the end, this PR does only evaluate_obligations_error_on_ambiguity. forgive me for being a bit lazy and just asking whether we want to do so as well, instead of investigating myself (guess is nah if we do eq, but probably yes if we do sub)

khyperia · 2026-09-30T06:10:13Z

+    let ty_const_of = |def_id| {
+        tcx.generics_of(def_id).own_params.iter().filter(|param| matches!(param.kind, Const { .. }))
+    };


nit: my guess is you copied and edited the name of this from compare_generic_param_kinds's ty_const_params_of. That variable is called that because it is the "type and const parameters of". This variable here is just consts, i.e. is the "const parameters of", so should be called const_params_of, not ty_const_of (which is a bit nonsensical~)

khyperia · 2026-09-30T06:13:09Z

    Ok(())
 }

+fn check_const_type_comparison<'tcx>(


Nit: This is not "checking a comparison", so the name check_const_type_comparison is a bit odd. Rather, it is comparing the types of const parameters between a trait and an impl, so perhaps compare_const_generic_param_types?

khyperia · 2026-09-30T06:14:08Z

Nit: This doc comment is now incorrect, this particular error is no longer checked here ✨ - could you update this comment? (and optionally adding a doc comment to your new method too if you want to, shrug, either way works)

khyperia · 2026-09-30T06:15:44Z

+                    "{} `{}` has an incompatible generic parameter for trait `{}`",
+                    impl_item.descr(),
+                    trait_item.name(),
+                    &tcx.def_path_str(tcx.parent(trait_item.def_id))


very nit: this & character can be removed :3

khyperia · 2026-09-30T06:16:35Z

    fn foo<const N: Self>() {}
-    //~^ ERROR cannot use `Self` in const parameter type
-    //~| ERROR associated function `foo` has an incompatible generic parameter for trait `MyTrait`
+    //~^ HELP add `#![feature(min_adt_const_params)]` to the crate attributes to enable `Self` as a const parameter type


Nit: IMO we don't need to check this HELP here, just the ERROR is enough

khyperia

Looking good! two little mistakes I noticed though, then this should be good. Would also be nice to have tests for these two things, too:

perhaps we want a test with an err in an impl for a nonlocal trait too, if one doesn't already exist. also I'm unsure of the behavior of ty_span when the type is malformed, e.g. <const N: > or <const N> so maybe a test for that too? but we might not even get to this point if the type is borked, unsure.

View changes since this review

khyperia · 2026-10-06T14:34:43Z

                param_impl_span,
                E0053,
-                "{} `{}` has an incompatible generic parameter for trait `{}`",
+                "{} `{}` has an incompatible const generic parameter for trait `{}`",


hmm, I think you did an oopsie here!

mm, still an oopsie here! this is checking generic param kinds, not const generics, so this diff should probably be reverted~

khyperia · 2026-10-06T14:36:00Z

+        iter::zip(const_params_of(impl_item.def_id), const_params_of(trait_item.def_id));
+
+    for (param_impl, param_trait) in param_iter {
+        let param_impl_ty_span = tcx.def_span(param_impl.def_id);


hmm, bit of an oopsie here too, this is not the span of the type, despite the variable name!

Jamesbarford · 2026-10-07T14:16:31Z

Thanks the nits along with new tests are covered in; 232173c and then 3d00182 corrects the message to ... has an incompatible const generic parameter for trait. I'll tidy the commits once it all looks okay

khyperia · 2026-10-08T07:10:26Z

                param_impl_span,
                E0053,
-                "{} `{}` has an incompatible generic parameter for trait `{}`",
+                "{} `{}` has an incompatible const generic parameter for trait `{}`",


mm, still an oopsie here! this is checking generic param kinds, not const generics, so this diff should probably be reverted~

khyperia · 2026-10-08T07:12:52Z

+        iter::zip(const_params_of(impl_item.def_id), const_params_of(trait_item.def_id));
+
+    for (param_impl, param_trait) in param_iter {
+        let param_impl_span = tcx.def_span(param_impl.def_id);


whoops - my earlier review was asking you to use ty_span here, but looks like you renamed the variable back to param_impl_span instead of changing it to use ty_span!

View changes since the review

rustbot · 2026-10-09T13:26:26Z

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.

khyperia · 2026-10-09T13:35:56Z

fantastic, thanks so much! ❤️

@bors r+ rollup

rust-bors · 2026-10-09T13:35:59Z

📌 Commit 901cebd has been tentatively approved by khyperia

It will be put into the queue for this repository once PR CI succeeds.

@khyperia

…, r=khyperia Fix - const parameters rejected when identical This keeps a series of cheaper checks before doing a more heavyweight comparison. Used the example in the issue as the test case. r? @khyperia Fixes rust-lang#162897

@khyperia

…, r=khyperia Fix - const parameters rejected when identical This keeps a series of cheaper checks before doing a more heavyweight comparison. Used the example in the issue as the test case. r? @khyperia Fixes rust-lang#162897

…uwer Rollup of 24 pull requests Successful merges: - #161998 ( Support type-relative assoc item paths in generic param defaults & const param types) - #162106 (Helpful suggestions for incorrect address-of mutability (2)) - #162652 (Syntactically reject leading parenthesized precise capturing lists in bare trait object types (`(use<…>)+`)) - #163337 (MIR move elimination [3/6]: PreciseLiveness) - #163938 (-Zassumptions-on-binders: rewrite alias outlives constraints more goodly) - #163939 (Better debug impls for some assumptions on binders types) - #163954 (fix(bootstrap/darwin): fix rpath for distributed LLD) - #163956 (Pass the unremapped path to the `rustc` invocation for doctests) - #164042 (Allow testing cg-gcc on any target) - #162443 (Do not retain `Normalization` goal errors in nested goals for `BestObligationVisitor:: non_trivial_candidates `) - #162908 (Fix - const parameters rejected when identical) - #163193 (cfi: mangle `f128` as `e` rather than `g` on platforms without `_Float128`) - #163634 (move overflow lint computation into decorator) - #163666 (Updates the expect message library/core/src/time.rs) - #163727 (rigid aliases to non-rigid for fully normalized check) - #163745 (replace `fully_monomorphized` with `cx.typing_env()`) - #163912 (Fix debug assert failure in `note_obligation_cause_code_inner`) - #163950 (don't treat inherited opaques as defining) - #163972 (const-eval: ICE when we hit a non-const fn) - #164000 (When mentioning that closure doesn't implement trait, point at closure) - #164007 ([rustdoc] Prefer local paths over remote ones when foreign item is locally reexported) - #164008 (properly ignore the current goal's usages) - #164017 (cg_llvm: Avoid some explicit casts to `*const c_char`) - #164025 (Less `CanonicalVarValues`)

@khyperia

Rollup merge of #162908 - Jamesbarford:fix/const-generic-bug, r=khyperia Fix - const parameters rejected when identical This keeps a series of cheaper checks before doing a more heavyweight comparison. Used the example in the issue as the test case. r? @khyperia Fixes #162897

rust-timer · 2026-10-10T05:26:58Z

Note

This PR was benchmarked as part of triage of its containing rollup: triage URL.

Finished benchmarking commit (5f8b748): comparison URL.

Overall result: ❌ regressions - please read:

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

If the regression was expected or you think it can be justified,
please write a comment with sufficient written justification, and add
@rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
If you think that you know of a way to resolve the regression, try to create
a new PR with a fix for the regression.
If you do not understand the regression or you think that it is just noise,
you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.2%, 0.2%]	3
Regressions ❌ (secondary)	0.2%	[0.2%, 0.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	[0.2%, 0.2%]	3

Max RSS (memory usage)

This perf run didn't have relevant results for this metric.

Cycles

This perf run didn't have relevant results for this metric.

Binary size

Results (primary -0.1%, secondary -0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.0%]	9
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.1%]	8
All ❌✅ (primary)	-0.1%	[-0.1%, -0.0%]	9

Artifact size: 406.49 MiB -> 406.38 MiB (-0.03%)

khyperia · 2026-10-10T06:25:08Z

A small perf regression is to be expected when compiling code that frequently implements traits with const generics, as this PR implements a more expensive, thorough, and correct check for comparing trait vs. impl const generic parameters. The bitmaps crate is indeed full of these.

rustbot assigned khyperia Sep 17, 2026

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 Sep 17, 2026

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 Sep 18, 2026

Jamesbarford force-pushed the fix/const-generic-bug branch from a666a48 to 24d3fed Compare September 18, 2026 15:00

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 Sep 18, 2026