Conversation
This comment was marked as outdated.
This comment was marked as outdated.
d223a25 to
0e5fba6
Compare
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
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.
| .last() | ||
| .map(|f| f.span) | ||
| .unwrap_or(ecx.tcx.span); | ||
| ErrorHandled::TooGeneric(span) |
There was a problem hiding this comment.
This change here...
| 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:8:15 | ||
| | | ||
| LL | struct Z<const X: usize> { | ||
| LL | // Ensure that proper context is shown in lint. | ||
| LL | multiline_field: | ||
| LL | () | ||
| LL | = { | ||
| | _______________^ | ||
| LL | | f::<X>(); | ||
| | | -------- this can't be const-evaluated until use | ||
| LL | | panic!(); | ||
| LL | | }, | ||
| | |_____________^ unevaluated default value |
There was a problem hiding this comment.
...is so that we can get this, where we don't just point at the whole default field value, but also to the exact place in the const that stopped it from being eagerly computed.
| if let Some(def_id) = field.value { | ||
| if let Err(ErrorHandled::TooGeneric(span)) = tcx.const_eval_poly(def_id) |
There was a problem hiding this comment.
Need feedback on whether just doing this is reasonable.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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... 🤔
|
Got concerned that not evaluating the const would cause arbitrary expressions through, but that is not the case: |
This comment has been minimized.
This comment has been minimized.
f9acb05 to
c6ed74c
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| let variants = adt_def.variants(); | ||
| let packed = adt_def.repr().packed(); | ||
| let own_params_require_monomorphization = | ||
| LazyCell::new(|| tcx.generics_of(item).own_requires_monomorphization()); |
There was a problem hiding this comment.
Could you remove the LazyCell and perf it?
There was a problem hiding this comment.
The changes in this file are no longer necessary under the new approach, right? Could you drop them again?
There was a problem hiding this comment.
I feel like we should still be improving all const errors with the more targeted spans on the call that caused a whole const to fail, but reverting the change in this PR.
| pub ban: u8 = panic!("asdf"), | ||
| // ^ If we run `const_eval_poly` without restricting const params, this would be | ||
| // evaluation panicked: asdf | ||
| // FIXME: This whould WARN! |
There was a problem hiding this comment.
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.
| //~^ ERROR attempt to compute `130_u8 + 130_u8`, which would overflow | ||
| } | ||
|
|
||
| pub struct Baz<const C: u8> { |
There was a problem hiding this comment.
If we deem it to spammy later on we can consider linting the type instead...
There was a problem hiding this comment.
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.
| struct Z<const X: usize> { | ||
| post_mono: usize = X / 0, | ||
| post_mono: usize = X / 0, //~ WARN | ||
| //~^ ERROR attempt to divide `1_usize` by zero |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| () | ||
| = { //~ WARN default value | ||
| f::<X>(); | ||
| panic!(); //~ ERROR: explicit panic |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Wait, why isn't it correct for this panic to trigger at const eval when using Z { .. }?
…eric
When trying to evaluate constants, if they reference const generics they will not be evaluated. When encountering this in default field values, 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:8:15
|
LL | struct Z<const X: usize> {
LL | multiline_field:
LL | ()
LL | = {
| _______________^
LL | | f::<X>();
| | -------- this can't be const-evaluated until use
LL | | panic!();
LL | | },
| |_____________^ unevaluated default value
|
= 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 + #[allow(unevaluated_default_field_value)]
LL | struct Z<const X: usize> {
|
```
Try spans better for `TooGeneric` errors.
Support `Span` context in lints.
…resent when needed
…pes with generic params
c6ed74c to
4b53cba
Compare
|
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. |
4b53cba to
702470b
Compare
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Always try to evaluate default field values and lint if it is too generic
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (e741da4): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking 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 Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)This perf run didn't have relevant results for this metric. CyclesResults (primary -2.6%, secondary 4.9%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 496.563s -> 492.175s (-0.88%) |
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.
Support
Spancontext in lints.Fixes #146496.
Part of #132162.
Alternative to #163182.
CC @fmease @BoxyUwU