Skip to content

Make in-scope non-region params inhibit the evaluation of default field values #163182

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.

Sign up for GitHub

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
fmease wants to merge 1 commit into from
Closed

Make in-scope non-region params inhibit the evaluation of default field values #163182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 5 additions & 17 deletions compiler/rustc_hir_analysis/src/check/wfcheck.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -986,28 +986,16 @@ pub(crate) fn check_type_defn<'tcx>(
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());

@fmease fmease Sep 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LazyCell probably overkill/unnecessary -- gonna perf this at some point

View changes since the review


for variant in variants.iter() {
// All field types must be well-formed.
for field in &variant.fields {
if let Some(def_id) = field.value
&& let Some(_ty) = tcx.type_of(def_id).no_bound_vars()
&& !*own_params_require_monomorphization
{
// FIXME(generic_const_exprs, default_field_values): this is a hack and needs to
// be refactored to check the instantiate-ability of the code better.
if let Some(def_id) = def_id.as_local()
&& let DefKind::AnonConst = tcx.def_kind(def_id)
&& let hir::Node::AnonConst(anon) = tcx.hir_node_by_def_id(def_id)
&& let expr = &tcx.hir_body(anon.body).value
&& let hir::ExprKind::Path(hir::QPath::Resolved(None, path)) = expr.kind
&& let Res::Def(DefKind::ConstParam, _def_id) = path.res
{
// Do not evaluate bare `const` params, as those would ICE and are only
// usable if `#![feature(generic_const_exprs)]` is enabled.
} else {
// Evaluate the constant proactively, to emit an error if the constant has
// an unconditional error. We only do so if the const has no type params.
let _ = tcx.const_eval_poly(def_id);
}
let _ = tcx.const_eval_poly(def_id);
}
let field_id = field.did.expect_local();
let span = tcx.ty_span(field_id);
Expand Down
10 changes: 4 additions & 6 deletions tests/ui/structs/default-field-values/invalid-const.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,13 @@
#![feature(default_field_values, generic_const_exprs)]
#![allow(incomplete_features)]
#![feature(default_field_values)]

pub struct Bat {
pub bax: u8 = panic!("asdf"),
//~^ ERROR evaluation panicked: asdf
pub bax: u8 = panic!("asdf"), //~ ERROR evaluation panicked: asdf
pub bat: u8 = 130 + 130, //~ ERROR attempt to compute `130_u8 + 130_u8`, which would overflow
}

pub struct Baz<const C: u8> {
pub bax: u8 = 130 + C, // ok
pub bat: u8 = 130 + 130,
//~^ ERROR attempt to compute `130_u8 + 130_u8`, which would overflow
Comment on lines -11 to -12

@estebank estebank Sep 22, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would still want us to stabilize this feature with some way this check still happens... Either way, we can probably land this change by adding a FIXME in the test and the code?

View changes since the review

@fmease fmease Sep 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather not ship this PR with a new stabilization-blocking FIXME since that wouldn't buy us anything, as we'd just kick the can down the road. I'd prefer to settle this either in this PR or in an alternative PR that would get shipped instead. After all, this topic is the only known major blocker, right?

I'm open to alternative proposals from you, T-types or anyone else (as long as they're principled :P).

@BoxyUwU BoxyUwU Sep 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know of a way to check for unconditionally panic-ing too-generic code, other than things like the unconditional panic lint which I would have expected to catch something like this? 🤔

I think the only principled way to avoid this is too not support generic default values but that seems like it would kinda screw the usability of the feature 🤔

@estebank estebank Sep 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cop-out solution: Explicit lint on structs with generics and default field values. You must explicitly allow it as a signal that "yes, I am aware that these consts are only evaluated when used". It will make the feature more annoying to use because of attribute, but at least then it will be clear.

Edit: I just checked, and unconditionally doing const_eval_poly no longer ICEs when referencing a const param directly either.

It feels to me that we can do

                if let Some(def_id) = field.value {
                    if let Err(ErrorHandled::TooGeneric(span)) = tcx.const_eval_poly(def_id) {
                        let field_span = tcx.def_span(def_id);
                        let mut multispan: MultiSpan =
                            if span.is_dummy() { field_span.into() } else { span.into() };
                        multispan.push_span_label(field_span, "unevaluated default value");
                        multispan.push_span_context(tcx.def_span(field.did));
                        multispan.push_span_context(tcx.def_span(item).shrink_to_lo());
                        tcx.dcx().span_warn( // FIXME: turn into a lint
                            multispan,
                            format!(
                                "field `{}` has a default value that is only checked when a value \
                                 of `{}` is constructed",
                                field.name,
                                tcx.def_path_str(item),
                            ),
                        );
                    }
                }

and get pretty much the behavior we'd want:

warning: field `field` has a default value that is only checked when a value of `Container` is constructed
  --> y.rs:14:9
   |
10 |   struct Container<const N: usize> {
11 |       field:
12 |       ()
13 |       = {
   |  _______-
14 | |         f::<N>();
   | |         ^^^^^^^^
15 | |         panic!();
16 | |     },
   | |_____- unevaluated default value
   = note: lint `...` enabled by default
help: if this behavior is acceptable, allow the lint and preferably write a test relying on the default value
   |
10 + #[allow(..)]
11 | struct Container<const N: usize> {
   |

What do people think?

(I'd love it if we improved TooGeneric to carry an appropriate span more often than it does today.)

@estebank estebank Sep 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/rust-lang/rust/pull/163235/changes#diff-eba11d0a83029d71c70f82e634f0d46b1bac714278f59fc1a998a872515efe67R999

pub bat: u8 = 130 + 130, // ok
pub bay: u8 = 1, // ok
}

Expand Down
6 changes: 3 additions & 3 deletions tests/ui/structs/default-field-values/invalid-const.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
error[E0080]: evaluation panicked: asdf
--> $DIR/invalid-const.rs:5:19
--> $DIR/invalid-const.rs:4:19
|
LL | pub bax: u8 = panic!("asdf"),
| ^^^^^^^^^^^^^^ evaluation of `Bat::bax::{constant#0}` failed here

error[E0080]: attempt to compute `130_u8 + 130_u8`, which would overflow
--> $DIR/invalid-const.rs:11:19
--> $DIR/invalid-const.rs:5:19
|
LL | pub bat: u8 = 130 + 130,
| ^^^^^^^^^ evaluation of `Baz::<C>::bat::{constant#0}` failed here
| ^^^^^^^^^ evaluation of `Bat::bat::{constant#0}` failed here

error: aborting due to 2 previous errors

Expand Down
Loading