Lint default field values in types with type or const parameters as they won't be evaluated pre-mono

[estebank]

View all comments

Default field values in types that have const generics are no longer being evaluated until mono. Emit a warn-by-default lint so that API designers are not caught of guard by this behavior.

warning: field `multiline_field` has a default value that is only checked when a value of `Z` is constructed
  --> $DIR/field-references-param-accurate-span.rs:9:15
   |
LL |   struct Z<const X: usize> {
LL |       multiline_field:
LL |           ()
LL |               = {
   |  _______________^
LL | |                 f::<X>();
LL | |                 panic!();
LL | |             },
   | |_____________^ this can't be const-evaluated until use
   |
   = help: structs and enums with type and const parameters only evaluate their default field values during construction, not eagerly when declared
   = note: `#[warn(unevaluated_default_field_value)]` on by default
help: if this behavior is acceptable, allow the lint and preferably write a test relying on the default value
   |
LL + #[expect(unevaluated_default_field_value)]
LL | struct Z<const X: usize> {
   |

Support Span context in lints.

Fixes #146496.
Part of #132162.
Alternative to #163182.

CC @fmease @BoxyUwU

[rustbot]

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

[estebank]

I don't know if this is the way we should go, but I feel like having this lint gets us most of the behavior we'd want. People can't get into a bad condition without warning, and it can be as unobtrusive as adding the allow on the crate root for those who really don't care. I just wouldn't want to have someone writing Struct<const T: u8> { field: u8 = const_fn() } and then changing that to Struct<const T: u8> { field: u8 = const_fn() + T } and then get a silent change in behavior.

View changes since this review

[estebank]

Need feedback on whether just doing this is reasonable.

[fmease]

Note that strictly speaking this is equally "unprincipled" in the sense that relying on when const_eval_poly returns TooGeneric "exposes" the implementation quirks of const eval.

On main, it's particularly "egregious" because it decides whether a given program is valid or not (pre-monomorphization). Under your PR it's at least a lint that can be silenced but still it means that changes to const eval that are meant to be purely internal / mere refactorings might lead to the lint getting emitted in fewer or more cases [edit: please see also #163235 (comment)] (AFAIU but I'm a layperson when it comes to const eval's internals).

Moreover, I don't know if Rust's (pre-monormorphization) semantics already depends on when const eval returns TooGeneric or not for code that may reference generic parameters (I'm specific here since TooGeneric can also be returned on certain kinds of normalization failures IIRC).

[fmease]

To give another example (apart from the one I gave in the GH issue).

This absolutely minor change makes const_eval_poly silently bail out with TooGeneric instead of evaluating & diverging with a const panic:

  #![feature(default_field_values)]
  
  struct X<T> {
      x: () = {
-         let _: T;
+         let _x: T;
          panic!()
      },
      y: T,
  }

That's exactly what I mean by the word "unprincipled". Under your PR, changes like this still determine whether to lint or not. That's … not great IMHO.

[fmease]

On main, it's particularly "egregious" because it decides whether a given program is valid or not (pre-monomorphization). Under your PR it's at least a lint that can be silenced but still it means that changes to const eval that are meant to be purely internal / mere refactorings might lead to the lint getting emitted in fewer or more cases (AFAIU but I'm a layperson when it comes to const eval's internals).

I'm still waking up, so I'm realizing now that under your PR it of course continues to be the case that Rust's (pre-monorphization) semantics (specifically what program to accept or to reject) would depend on the whether const_eval_poly returns TooGeneric! It's just that in one case we now emit a lint (which is irrelevant when talking core semantics).

[fmease]

All that to say,

Fixes #146496.

sadly your PR does in fact not address this issue. Looking at the example I gave in that issue, uncommenting that innocuous-seeming line upstream still breaks downstream!

Moreover, the lint message doesn't make that clear since it's obviously only targeted towards explaining why the default isn't evaluated now to address the first paragraph(s) of your comment #163182 (comment). But it completely sweeps under the table the SemVer implications.

[fmease]

When I read the first paragraph(s) of your comment #163182 (comment) I thought you meant "let's take fmease's approach from PR #163182 but also emit a lint" (which would indeed affect all structs with type or const params that have field defaults, so that might be a non-starter).

[estebank]

We can follow your approach with a less targeted lint. We just need some feedback. The problem with your approach is that the lint will be much more noisy. My biggest concern is that addint a type param to a struct all of a sudden causes the semantics to change. That is a pretty big foot gun.

[estebank]

I pushed the behavior from your draft, + an updated lint. The lint gets quite noisy, bordering on unusable, and we of course lose some opportunities to emit errors, which I am concerned about. I wonder if we could silence the lint if there was at least one construction of the struct with default values... 🤔

[estebank]

Got concerned that not evaluating the const would cause arbitrary expressions through, but that is not the case:

warning: field `f` has a default value that is only checked when a value of `S` is constructed
 --> x.rs:4:12
  |
3 | struct S<T> {
4 |     f: T = { foo::<T>() },
  |            ^^^^^^^^^^^^^^ this can't be const-evaluated until use
  |
  = help: structs with type and const parameters only evaluate their default field values during construction, not eagerly when declared
  = note: `#[warn(unevaluated_default_field_value)]` on by default
help: if this behavior is acceptable, allow the lint and preferably write a test relying on the default value
  |
3 + #[expect(unevaluated_default_field_value)]
4 | struct S<T> {
  |

error[E0015]: cannot call non-const function `foo::<T>` in constants
 --> x.rs:4:14
  |
4 |     f: T = { foo::<T>() },
  |              ^^^^^^^^^^
  |
note: function `foo` is not const
 --> x.rs:6:1
  |
6 | fn foo<T>()->T { panic!()}
  | ^^^^^^^^^^^^^^
  = note: calls in constants are limited to constant functions, tuple structs and tuple variants

[fmease]

Thanks! I've got a couple of comments that need addressing, then I'll approve the PR after a re-review.

Could you update the PR title+description & squash away the outdated approach?

View changes since this review

[fmease]

Why doesn't it?

[estebank]

I suspect it is because the DefId of the default actually corresponds to the panic macro's crate (so non-local), which causes us not to have a HirId to attach the lint to.

Edit: almost, it was the Span instead, pointing inside the panic!. We have to use the call site instead.

[fmease]

If we deem it to spammy later on we can consider linting the type instead...

[estebank]

I'd looked into doing that. The issue I encountered was that we'd have to do some shenanigans with the hir id associated to the lint, make it be the whole item instead of allowing individual fields to be allowed. I'm sure we could work around it, but it was too involved to be part of an unrelated PR.

[fmease]

Wait, this shouldn't get eval'ed post mono either.

The behavior should mirror our behavior for GCI:

//@ build-pass
#![feature(generic_const_items)]
const Z<const X: usize>: usize = X / 0;

You probably need to hunt down all other places in the compiler that evaluate field defaults and add the same own_requires_monomorphization checks there to achieve that.

[estebank]

Why shouldn't this error be emitted here? It triggers through indirect::<1>(); and let x: Z<1> = Z { .. };.

Side-note: we should have a better mechanism than ErrorHandled so that we can extend const errors with something akin to macro backtraces, instead of the free-floating notes we're using now.

[fmease]

My bad, I don't know why I thought Z didn't get instantiated.

Moreover, I've now confused myself several times along the way. Obviously (?), we evaluate all(*) constants in functions post-mono if they don't reference generic parameters even if the function has type &/ const params. However, I guess that's not exploitable (?) in the way I've explained it so far for various reasons.

(*): Unless they're located inside const { … } which make them only get eval'ed post-mono if there aren't in-scope type &/ const params...

#![feature(generic_const_items)]

const X<const N: usize>: usize = N / 0;

fn f<const N: usize>() { X::<1>; } // POST-MONO ERROR
fn g<const N: usize>() { const { X::<1>; } } // build-pass

#![feature(default_field_values)] // on your branch

struct X<const N: usize> { x: usize = N / 0 }

fn f<const N: usize>() { X::<1> { .. }; } // POST-MONO ERROR
fn g<const N: usize>() { const { X::<1> { .. }; } } // build-pass

[fmease]

This should only diverge post-mono since it's instantiated in main. I guess that's not the case yet (CC my other comment) but once it is, it should warrant a comment.

[estebank]

Wait, why isn't it correct for this panic to trigger at const eval when using Z { .. }?

[estebank]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: e741da4 (e741da44538220b14bec86cd7ee853c68ce544dd)
Base parent: ea13733 (ea137335b78829b4514bf1b4c16302f74fab8581)

[rust-timer]

Finished benchmarking commit (e741da4): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

@bors rollup=never rustc-perf
@rustbot label: -S-waiting-on-perf -perf-regression

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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-0.6%, -0.1%]	10
Improvements ✅ (secondary)	-0.4%	[-0.7%, -0.1%]	43
All ❌✅ (primary)	-0.2%	[-0.6%, -0.1%]	10

Max RSS (memory usage)

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

Cycles

Results (primary -2.6%, secondary 4.9%)

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)	4.9%	[3.0%, 6.4%]	8
Improvements ✅ (primary)	-2.6%	[-2.6%, -2.6%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.6%	[-2.6%, -2.6%]	1

Binary size

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

Bootstrap: 496.563s -> 492.175s (-0.88%)
Artifact size: 408.58 MiB -> 408.67 MiB (0.02%)

[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.

[fmease]

Isn't it obvious though that an expression that (syntactically) mentions const parameter T doesn't get evaluated¹? Isn't the main motivation constants that don't (syntactically) mention the parameters like idk 128u8 + 128u8, 1 / 0 or panic!()?

If the user writes

struct Type<const N: usize> {
    field: usize = {
        assert!(N != 1);
        N
    }
}

they know that the assertion won't get evaluated regardless of whether we do or don't call const_eval_poly if there are in-scope type/const parameters, so it's quite annoying & disruptive to emit the lint.

It's out of scope for this PR, of course, but I'd like that to be something we experiment with. In this case, a heuristic would be fine since it's just a lint and doesn't affect core semantics.

Visiting the HIR body (expr) looking for QPath::Resolveds of DefKind::{TyParam,ConstParam} would be a bit gnarly & possibly expensive (we're in the happy path after all) (need to account for Self type aliases, expr <-> item boundaries (includes anon consts)).

Looking at the (unevaluated) ty::Const would be ideal as we'd just need to check whether .has_non_region_params() (which leverages TypeFlags). However, getting access to the (unevaluated) ty::Const might be pretty tricky if not impossible w/o triggering more validation checks (which would thus affect core semantics). Query mir_built / hook build_mir_inner_impl is out of the question since it can emit additional errors. Oh well.

View changes since the review

Footnotes

1. Of course, there are proglangs where this isn't the case. ↩

[fmease]

Moreover I feel like if starting on things like this we should lint in all similar cases, too, otherwise it'd be quite odd.

E.g., associated constants in general (their def site doesn't get unconditionally evaluated since there's at least one in-scope type parameter: the Self type parameter).

Or inline consts (e.g., fn f<const _N: usize>() { const { panic!() } }).

Obviously, if we introduced that lint to assoc consts (without at least doing the syntactic "mentions check") we'd trigger everywhere and all hell would break loose...

Anyways, I'm happy with the new core semantics & T-lang can decide how to handle the lint situation on stabilization.

[fmease]

What's left are some fixes to the lint description.

Then r=me.

View changes since this review

[fmease]

Moreover I feel like if starting on things like this we should lint in all similar cases, too, otherwise it'd be quite odd.

E.g., associated constants in general (their def site doesn't get unconditionally evaluated since there's at least one in-scope type parameter: the Self type parameter).

Or inline consts (e.g., fn f<const _N: usize>() { const { panic!() } }).

Obviously, if we introduced that lint to assoc consts (without at least doing the syntactic "mentions check") we'd trigger everywhere and all hell would break loose...

Anyways, I'm happy with the new core semantics & T-lang can decide how to handle the lint situation on stabilization.

[fmease]

1. Regarding

   has const parameters to be evaluated

   That's not really relevant here, the defaults don't need to reference any of the type/const parameters for this lint to trigger. Esp. since your main concern was about defaults like 128u8 + 128u8.

2. Only mentions structs, not enums.

3. Regarding

   detects when a struct has

   The struct isn't really the subject here because we actually lint on struct field defaults, they are the subject.

So idk more sth like this, roughly?

Suggested change

	/// The `unevaluated_default_field_value` lint detects when a struct has a field with a default
	/// value and has const parameters to be evaluated, meaning that checking that default for
	/// correctness is delayed to *instantiation* (post-monomorphization), instead of happening
	/// eagerly.
	/// The `unevaluated_default_field_value` lint detects default field values that don't get evaluated [eagerly / at the definition site] because the [overarching / owning / corresponding / parent] struct or enum has type or const parameters, meaning [their evaluation is delayed to instantiation time / they only get evaluated at instantiation sites / they only get evaluated when the struct or enum gets constructed using that very default (if it's concrete enough) / …] (post-monormophization).