Always check ConstArgHasType even when otherwise ignoring

[khyperia]

View all comments

fixes #149774

helping @BoxyUwU finish up #150322, this is a simple tweak/finishing-up of that PR.

this is a breaking change that crater detected has some issues with in Boxy's PR, and hence needs a t-types FCP. I can go and help fix those crates if/once the break is signed off on.

[BoxyUwU]

thx for taking this on for me

[rustbot]

changes to the core type system

cc @lcnr

[rustbot]

r? @nnethercote

rustbot has assigned @nnethercote.
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 13 candidates

[khyperia]

Background Context

First, it's important to note that:

• We do not check that the RHS of free type aliases are WF: type Foo = Vec<[u32]>; is valid
• We do not check trait objects' where clauses: having trait Object<T: Sized> then let x: &dyn Object<[u32]>; is valid
• We do not check WF related things for lifetimes from a for<'a>

This PR does not fundamentally change the above.

Second, in the past, we lowered many things to anon consts. In doing so, we implicitly caused a typecheck of constants to happen, by enforcing that the body of the anon const matched with the type of the anon const.

• https://rustc-dev-guide.rust-lang.org/const-generics.html#anon-consts
• https://rustc-dev-guide.rust-lang.org/const-generics.html#checking-types-of-const-arguments

This typecheck always happend, regardless of if the usage of the anon const was inside a free type alias or trait object reference. In other words, these used to cause compiler errors:

• struct Struct<const N: usize> {}
  type Foo<const B: bool> = Struct<B>;

• trait Object<const N: usize> {}
  fn f<const B: bool>(a: &dyn Object<B>);

However, recently (a few months ago) we changed how some constants are desugared, no longer generating anon consts for them, and instead representing them immediately. This caused consts in these locations to stop being typechecked.

Actual Change

Eventually, we would like to begin wfchecking free type aliases and trait objects. Today is not that day, so instead, this PR is reintroducing typechecking for constants, to minimize the eventual breakage that will be caused by wfchecking type aliases and trait objects.

• Look at wf obligations for free type aliases, discard all of them except ConstArgHasType, and prove those.
• Look at wf obligations for trait objects, discard all of them except ConstArgHasType, and prove those.

Important to note is that ConstArgHasType cannot interact with lifetimes from a for<'a> on stable, because we do not allow generic parameters (including lifetimes) in the types of const generics, so we are free to continue ignoring them.

Crater Breakage

A few crates have already slipped in some "invalid" code, here's a crater run: #150322 (comment)

There are no crater breakages around the trait objects part of this PR.

Here's an example breakage of the free type alias part of this PR:

error: the constant `MODE` is not of type `usize`
  --> src/lfstd.rs:92:1
   |
92 | type AllocScqRing<const MODE: bool> = ScqRing<Box<[CachePadded<AtomicUsize>]>, MODE>;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `usize`, found `bool`
   |
note: required by a const generic parameter in `ScqRing`
  --> src/scq.rs:47:23
   |
47 | pub struct ScqRing<I, const MODE: usize> {
   |                       ^^^^^^^^^^^^^^^^^ required by this const generic parameter in `ScqRing`

Note that even though these type aliases are clearly incorrect, they are still useable by downstream code - this code compiles successfully before this PR, but fails after it:

struct Struct<const N: usize> {}
type Foo<const B: bool> = Struct<B>;
type Bar<const N: usize> = Foo<N>;
let x: Bar<1_usize>;

The breakage is small enough that one person (me) could easily help fix up broken crates. However, the longer we wait, the more breakage will creep in. We've waited a few months already so it's not desperately urgent, but still.

[BoxyUwU]

r? BoxyUwU

[BoxyUwU]

cc @rust-lang/types
@rfcbot merge types

See: #152931 (comment)

[rust-rfcbot]

Team member @BoxyUwU has proposed to merge this. The next step is review by the rest of the tagged team members:

• @BoxyUwU
• @jackh726
• @lcnr
• @oli-obk
• @spastorino

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[lcnr]

reintroducing typechecking for constants,

This PR checks that the type of constant arguments matches the generic parameter instantiated by them when checking that the containing item is well-formed. We do not go back to calling HIR typeck for them. Is that correct?

[khyperia]

This PR checks that the type of constant arguments matches the generic parameter instantiated by them when checking that the containing item is well-formed. We do not go back to calling HIR typeck for them. Is that correct?

doublechecked with boxy (was only 90% sure of exact definitions of words), that is correct!

"most" constants still lower to anonconsts still, it's just very simple stuff that is now directly represented (as of a few months ago), without lowering to an anon const. So, this PR is just covering those simple things that are no longer anonconsts. AFAIK, anonconsts always have been and continue to be HIR typechecked.

[khyperia]

tests/ui/type-alias/lack-of-wfcheck.rs, introduced after this PR was made, in #153738 now fails with this PR. Hmm.

error[E0080]: evaluation panicked: explicit panic
  --> /d/rust/tests/ui/type-alias/lack-of-wfcheck.rs:14:23
   |
LL | type Diverging = [(); panic!()]; // `panic!()` diverging
   |                       ^^^^^^^^ evaluation of `Diverging::{constant#0}` failed here

error: constant evaluation is taking a long time
  --> /d/rust/tests/ui/type-alias/lack-of-wfcheck.rs:22:63
   |
LL | type Several<'a> = dyn HasGenericAssocType<Type<'a, String, { loop {} }> = [u8]>;
   |                                                               ^^^^^^^
   |
   = note: this lint makes sure the compiler doesn't get stuck due to infinite loops in const eval.
           If your compilation actually takes a long time, you can safely allow the lint
help: the constant being evaluated
  --> /d/rust/tests/ui/type-alias/lack-of-wfcheck.rs:22:61
   |
LL | type Several<'a> = dyn HasGenericAssocType<Type<'a, String, { loop {} }> = [u8]>;
   |                                                             ^^^^^^^^^^^
   = note: `#[deny(long_running_const_eval)]` on by default

error: aborting due to 2 previous errors

So the RFC comment listing breaking changes wasn't fully complete, as this causes unused diverging type aliases to now fail to compile. I didn't realize that, sorry. Maybe that's ok? Is a new RFC period needed?

[khyperia]

Ah, sorry, ignore me, it only fails with the unnormalized_obligations -> obligations change (see Boxy's review comment above). Leaving it as unnormalized_obligations does not seem to attempt to const eval the diverging things. I think.

[rustbot]

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]

As far as I can tell, there were two crates broken in the crater run - one of which, ranch, was subsequently fixed by AldaronLau/ranch@791e96a . The other, lfqueue, is fixed by the PR I just opened, linked above.

[BoxyUwU]

@bors r+ rollup=never

I don't think it makes sense to wait for the fix PR to be merged since that crate has been inactive for a while now (and of course the library author still has a couple months to merge it before this PR hits stable)

[rust-bors]

📋 This PR cannot be approved because it currently has the following label: S-waiting-on-fcp.

[BoxyUwU]

@bors r+ rollup=never

[rust-bors]

📌 Commit e9c273e has been approved by BoxyUwU

It is now in the queue for this repository.

[rust-bors]

☀️ Test successful - CI
Approved by: BoxyUwU
Duration: 3h 20m 31s
Pushing f66622c to main...

[github-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 eb9d3ca (parent) -> f66622c (this PR)

Test differences

Show 8 test diffs

Stage 1

• [ui] tests/ui/const-generics/check_const_arg_type_in_free_alias.rs#eager: [missing] -> pass (J1)
• [ui] tests/ui/const-generics/check_const_arg_type_in_free_alias.rs#lazy: [missing] -> pass (J1)
• [ui] tests/ui/const-generics/check_const_arg_type_in_trait_object.rs: [missing] -> pass (J1)

Stage 2

• [ui] tests/ui/const-generics/check_const_arg_type_in_free_alias.rs#eager: [missing] -> pass (J0)
• [ui] tests/ui/const-generics/check_const_arg_type_in_free_alias.rs#lazy: [missing] -> pass (J0)
• [ui] tests/ui/const-generics/check_const_arg_type_in_trait_object.rs: [missing] -> pass (J0)

Additionally, 2 doctest diffs were found. These are ignored, as they are noisy.

Job group index

• J0: aarch64-apple, aarch64-gnu, aarch64-gnu-llvm-21-1, aarch64-msvc-1, arm-android, armhf-gnu, dist-i586-gnu-i586-i686-musl, i686-gnu-1, i686-gnu-nopt-1, i686-msvc-1, optional-x86_64-gnu-parallel-frontend, test-various, x86_64-gnu, x86_64-gnu-debug, x86_64-gnu-gcc, x86_64-gnu-llvm-21, x86_64-gnu-llvm-21-2, x86_64-gnu-llvm-22-2, x86_64-gnu-nopt, x86_64-gnu-stable, x86_64-mingw-1, x86_64-msvc-1
• J1: x86_64-gnu-llvm-21-3, x86_64-gnu-llvm-22-3

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard f66622c7eca7fc48ccc4dac848ff911b09a4d566 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

1. dist-aarch64-llvm-mingw: 1h 53m -> 1h 28m (-21.8%)
2. dist-apple-various: 1h 51m -> 1h 33m (-16.7%)
3. dist-x86_64-apple: 2h 12m -> 2h 32m (+14.6%)
4. x86_64-gnu-llvm-22-1: 1h 15m -> 1h 5m (-13.5%)
5. x86_64-mingw-2: 3h 2m -> 2h 39m (-12.6%)
6. x86_64-gnu-llvm-21-2: 1h 28m -> 1h 39m (+12.6%)
7. i686-gnu-2: 1h 48m -> 1h 36m (-10.7%)
8. i686-msvc-2: 2h 6m -> 2h 20m (+10.5%)
9. x86_64-gnu-llvm-21-1: 1h 18m -> 1h 10m (-9.9%)
10. pr-check-2: 43m 17s -> 39m 20s (-9.1%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

[rust-timer]

Finished benchmarking commit (f66622c): comparison URL.

Overall result: ❌ regressions - please read the text below

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)	1.9%	[0.1%, 4.1%]	13
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.9%	[0.1%, 4.1%]	13

Max RSS (memory usage)

Results (primary -2.7%, secondary -6.6%)

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)	-2.7%	[-3.2%, -2.3%]	2
Improvements ✅ (secondary)	-6.6%	[-6.6%, -6.6%]	1
All ❌✅ (primary)	-2.7%	[-3.2%, -2.3%]	2

Cycles

Results (primary 3.0%, secondary 6.1%)

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

	mean	range	count
Regressions ❌ (primary)	3.0%	[1.9%, 3.7%]	3
Regressions ❌ (secondary)	6.1%	[3.5%, 12.0%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.0%	[1.9%, 3.7%]	3

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%]	51
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.0%]	23
All ❌✅ (primary)	-0.1%	[-0.1%, -0.0%]	51

Bootstrap: 484.336s -> 484.979s (0.13%)
Artifact size: 396.95 MiB -> 394.94 MiB (-0.50%)

[panstromek]

perf triage:

These regressions look real, and it seems like they were not discussed before merge.

Regressed benchmarks are both type-heavy and and the regression seems to be entirely in typeck (assumed_wf_types, check_well_formed, predicates_of and type_alias_is_lazy all show some negative changes in detailed results). This seems to match the PR changes.

I assume some regression is expected because there's more work to do, but is it this much? Is it possible to reduce the impact in some followup?

This fixes a regression so I also assume we don't want to revert this.

[lcnr]

typenum has a lot of type aliases, so doing additional work for them causing a regression seems expected to me. Looking at the code, I don't know how to avoid this regression

[panstromek]

Ok, thanks for confirmation. Let's mark this as triaged then.

@rustbot label: +perf-regression-triaged