Reject non-constructor self types in const-arg tuple-call lowering - #157513
Conversation
|
HIR ty lowering was modified cc @fmease |
rustbot
added
S-waiting-on-review
|
|
|
Hi, @fmease! |
|
r? BoxyUwU |
| //@ compile-flags: -Znext-solver | ||
|
|
||
| #![feature(min_generic_const_args)] | ||
| #![feature(generic_const_args)] |
There was a problem hiding this comment.
this PR will need a rebase since we've changed a lot of the rules around what is considered an anon const or not. i think to reproduce this now you'll need macroless_generic_const_args enabled.
| hir::TyKind::Path(hir::QPath::Resolved(_, path)) => path.res, | ||
| _ => Res::Err, | ||
| }; | ||
| if matches!( |
There was a problem hiding this comment.
can you move this logic into a separate fn, something like try_recover_misrepresented_function_call so that the good-path logic isn't complicated by diagnostics stuff.
|
@rustbot author |
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
706a7b5 to
cceaea0
Compare
This comment has been minimized.
This comment has been minimized.
|
@rustbot ready |
rustbot
added
S-waiting-on-review
| fn error_complex_const_arg(&self, span: Span) -> ErrorGuaranteed { | ||
| self.dcx() | ||
| .span_err(span, "complex const arguments must be placed inside of a `const` block") | ||
| } |
There was a problem hiding this comment.
Can we inline this?
There was a problem hiding this comment.
I think it's nice having it outlined so the errors don't diverge :3 though we could be using a structured diagnostic here instead of embedding strings into the source
There was a problem hiding this comment.
I went with the structured diagnostic you suggested. Both sites emit ComplexConstArg, so the message still has one source of truth even though the emissions are inline. btw imo this is a pretty clean compromise with Shourya's suggestion. ltm if you meant keeping the helper around the structured diag too.
| matches!( | ||
| self_ty_res, | ||
| Res::Def(DefKind::Struct | DefKind::Union | DefKind::ForeignTy, _) | Res::PrimTy(_) | ||
| ) | ||
| .then(|| self.error_complex_const_arg(span)) |
There was a problem hiding this comment.
With this we do not handle enum associated function calls. In the current lowering path, a call like E::len() is allowed to fall through because E::V(..) could be a valid tuple-variant constructor. If E is generic and written without its generic arguments, lowering the self type then emits a misleading error. Maybe we can handle this case here?
For example:
enum E<const N: usize> {
V,
}
impl E<0> {
const fn len() -> usize { 1 }
}
fn bad2(_: FieldName<{ E::len() }>) {}There was a problem hiding this comment.
I think it's tricky to handle enums because they do actually need to go into the codepath where we resolve len to tell whether its a tuple constructor or a function 🤔
struct/union/foreignty/primty are all nice because they will never resolve to a tuple constructor so this codepath is always an error
There was a problem hiding this comment.
we could in theory manually check the generics of the enum against the args here, but I don't want us to maintain a second codepath for trying to lower the ty in a way that doesnt emit errors if it fails
There was a problem hiding this comment.
fyi I left enum handling out after reading your follow-up. I tried the precheck locally, but it started duplicating resolver decisions just to avoid emitting the first error. imo that is not worth maintaining for this regression. idk of a clean non-emitting path we can reuse here, so I'd rather leave enums on the existing flow and handle them separately later. ltm if there is an API I missed.
|
@bors r+ |
rust-bors
Bot
added
S-waiting-on-bors
…157152, r=BoxyUwU
Reject non-constructor self types in const-arg tuple-call lowering
Turning on `min_generic_const_args` makes `tracing` stop compiling. Its logging macros expand a field name to something like `FieldName<{ FieldName::len(stringify!(field)) }>`, and under mgca a braced call in const-arg position gets lowered as a tuple constructor. So lowering tries to resolve the bare self type `FieldName` (written without its `const N`), which kicks off an `E0107 "missing generics"` cascade pointing deep into macro code. But `FieldName::len(..)` is just an associated fn, not a constructor, so lowering it like a `TupleCall` was wrong in the first place. See rust-lang#157152.
Only an enum can host a tuple-variant ctor, so for any other self type that can't (struct, union, primitive, foreign type) the call has to be an assoc fn and needs wrapping in `const { ... }`. We catch those from the self type's resolution before lowering it and emit the existing "complex const arguments must be placed inside of a `const` block" error, which is the message you'd want anyway. Enums, aliases, `Self` and type params get left alone since they might resolve to an enum. tbh the bare generic *enum* case (`Option::Some(0)`) still E0107s, and imo that's better as a follow-up since catching it needs the variant type before lowering. Tests cover struct/union/primitive/foreign plus the wrapped forms that compile, and I checked it against the real `tracing` 0.1.44 crate too.
_fwiw just the code changes and tests were implemented with AI help and I verified/reproduced/tested everything locally before sending to remote._
|
This pull request was unapproved. This PR was contained in a rollup (#161487), which was unapproved. |
A braced const argument that is a call, like `Ty<{ Ty::f() }>`, is lowered
as a tuple constructor. When the callee's self type cannot host a
tuple-variant constructor (a struct, union, primitive, or foreign type),
the call is an associated function, not a constructor, and must be wrapped
in a `const { ... }` block.
Detect that from the self type's resolution before lowering it, and emit
the existing "complex const arguments must be placed inside of a `const`
block" diagnostic. A generic struct written without its arguments, as
`tracing`'s logging macros generate, would otherwise produce a spurious
E0107 "missing generics" cascade; a primitive or foreign type would
surface an opaque "invalid base path" error.
Enums, aliases, `Self`, and type parameters are left to constructor
lowering, since each may resolve to an enum.
Add UI tests for struct, union, primitive, and foreign self types, plus
the `const { ... }`-wrapped form that compiles.
9c0d5d2 to
64ed71b
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. |
|
@JonathanBrouwer fixed the error and pushed. The rollup pulled in the recent lifetime cleanup around lower_const_arg_tuple_call. That function no longer requires its HIR inputs to live for all of 'tcx, but the helper I extracted still did. Matching the qpath gave the helper a shorter borrow, then the helper tried to stretch it to 'tcx. That is where the E0621 failure came from. I changed the helper to take &hir::Ty<'_>. It only reads the type kind and its resolution, so it does not need to keep that borrow around. The compiler suggested putting 'tcx back on qpath, but I do not think that is the right fix. It would make the whole caller stricter just because one helper had a needlessly strict signature. I ran the full fmt check, checked rustc_hir_analysis, and forced the existing const-arg UI test to rerun. All clean btw . @BoxyUwU could you take another look once the try build is green, please :)? |
|
@Dnreikronos: 🔑 Insufficient privileges: not in try users |
|
@bors r=BoxyUwU |
rust-bors
Bot
added
S-waiting-on-bors
…157152, r=BoxyUwU
Reject non-constructor self types in const-arg tuple-call lowering
Turning on `min_generic_const_args` makes `tracing` stop compiling. Its logging macros expand a field name to something like `FieldName<{ FieldName::len(stringify!(field)) }>`, and under mgca a braced call in const-arg position gets lowered as a tuple constructor. So lowering tries to resolve the bare self type `FieldName` (written without its `const N`), which kicks off an `E0107 "missing generics"` cascade pointing deep into macro code. But `FieldName::len(..)` is just an associated fn, not a constructor, so lowering it like a `TupleCall` was wrong in the first place. See rust-lang#157152.
Only an enum can host a tuple-variant ctor, so for any other self type that can't (struct, union, primitive, foreign type) the call has to be an assoc fn and needs wrapping in `const { ... }`. We catch those from the self type's resolution before lowering it and emit the existing "complex const arguments must be placed inside of a `const` block" error, which is the message you'd want anyway. Enums, aliases, `Self` and type params get left alone since they might resolve to an enum. tbh the bare generic *enum* case (`Option::Some(0)`) still E0107s, and imo that's better as a follow-up since catching it needs the variant type before lowering. Tests cover struct/union/primitive/foreign plus the wrapped forms that compile, and I checked it against the real `tracing` 0.1.44 crate too.
_fwiw just the code changes and tests were implemented with AI help and I verified/reproduced/tested everything locally before sending to remote._
…157152, r=BoxyUwU
Reject non-constructor self types in const-arg tuple-call lowering
Turning on `min_generic_const_args` makes `tracing` stop compiling. Its logging macros expand a field name to something like `FieldName<{ FieldName::len(stringify!(field)) }>`, and under mgca a braced call in const-arg position gets lowered as a tuple constructor. So lowering tries to resolve the bare self type `FieldName` (written without its `const N`), which kicks off an `E0107 "missing generics"` cascade pointing deep into macro code. But `FieldName::len(..)` is just an associated fn, not a constructor, so lowering it like a `TupleCall` was wrong in the first place. See rust-lang#157152.
Only an enum can host a tuple-variant ctor, so for any other self type that can't (struct, union, primitive, foreign type) the call has to be an assoc fn and needs wrapping in `const { ... }`. We catch those from the self type's resolution before lowering it and emit the existing "complex const arguments must be placed inside of a `const` block" error, which is the message you'd want anyway. Enums, aliases, `Self` and type params get left alone since they might resolve to an enum. tbh the bare generic *enum* case (`Option::Some(0)`) still E0107s, and imo that's better as a follow-up since catching it needs the variant type before lowering. Tests cover struct/union/primitive/foreign plus the wrapped forms that compile, and I checked it against the real `tracing` 0.1.44 crate too.
_fwiw just the code changes and tests were implemented with AI help and I verified/reproduced/tested everything locally before sending to remote._
…157152, r=BoxyUwU
Reject non-constructor self types in const-arg tuple-call lowering
Turning on `min_generic_const_args` makes `tracing` stop compiling. Its logging macros expand a field name to something like `FieldName<{ FieldName::len(stringify!(field)) }>`, and under mgca a braced call in const-arg position gets lowered as a tuple constructor. So lowering tries to resolve the bare self type `FieldName` (written without its `const N`), which kicks off an `E0107 "missing generics"` cascade pointing deep into macro code. But `FieldName::len(..)` is just an associated fn, not a constructor, so lowering it like a `TupleCall` was wrong in the first place. See rust-lang#157152.
Only an enum can host a tuple-variant ctor, so for any other self type that can't (struct, union, primitive, foreign type) the call has to be an assoc fn and needs wrapping in `const { ... }`. We catch those from the self type's resolution before lowering it and emit the existing "complex const arguments must be placed inside of a `const` block" error, which is the message you'd want anyway. Enums, aliases, `Self` and type params get left alone since they might resolve to an enum. tbh the bare generic *enum* case (`Option::Some(0)`) still E0107s, and imo that's better as a follow-up since catching it needs the variant type before lowering. Tests cover struct/union/primitive/foreign plus the wrapped forms that compile, and I checked it against the real `tracing` 0.1.44 crate too.
_fwiw just the code changes and tests were implemented with AI help and I verified/reproduced/tested everything locally before sending to remote._
…157152, r=BoxyUwU
Reject non-constructor self types in const-arg tuple-call lowering
Turning on `min_generic_const_args` makes `tracing` stop compiling. Its logging macros expand a field name to something like `FieldName<{ FieldName::len(stringify!(field)) }>`, and under mgca a braced call in const-arg position gets lowered as a tuple constructor. So lowering tries to resolve the bare self type `FieldName` (written without its `const N`), which kicks off an `E0107 "missing generics"` cascade pointing deep into macro code. But `FieldName::len(..)` is just an associated fn, not a constructor, so lowering it like a `TupleCall` was wrong in the first place. See rust-lang#157152.
Only an enum can host a tuple-variant ctor, so for any other self type that can't (struct, union, primitive, foreign type) the call has to be an assoc fn and needs wrapping in `const { ... }`. We catch those from the self type's resolution before lowering it and emit the existing "complex const arguments must be placed inside of a `const` block" error, which is the message you'd want anyway. Enums, aliases, `Self` and type params get left alone since they might resolve to an enum. tbh the bare generic *enum* case (`Option::Some(0)`) still E0107s, and imo that's better as a follow-up since catching it needs the variant type before lowering. Tests cover struct/union/primitive/foreign plus the wrapped forms that compile, and I checked it against the real `tracing` 0.1.44 crate too.
_fwiw just the code changes and tests were implemented with AI help and I verified/reproduced/tested everything locally before sending to remote._
…uwer Rollup of 7 pull requests Successful merges: - #157513 (Reject non-constructor self types in const-arg tuple-call lowering) - #159887 (compiletest: forward disable-minification from bootstrap) - #160949 (Add floating point inline ASM support for SPARC) - #156160 (feat: add symmetric PartialEq impls for Vec, &[T], &mut [T] versus Cow<'_, [T]>) - #160302 (target_features: sse (or at least avx2) is incompatible with soft-float ABI) - #160914 (Derive `GenericTypeVisitable` for `RegionConstraint` _correctly_) - #161341 (be more permissive wrt overflow and and improve diagnostics)
This comment has been minimized.
This comment has been minimized.
rust-bors
Bot
added
merged-by-bors
|
A job failed! Check out the build log: (web) (plain enhanced) (plain) Click to see the possible cause of the failure (guessed by this bot) |
7 participants
View all comments
Turning on
min_generic_const_argsmakestracingstop compiling. Its logging macros expand a field name to something likeFieldName<{ FieldName::len(stringify!(field)) }>, and under mgca a braced call in const-arg position gets lowered as a tuple constructor. So lowering tries to resolve the bare self typeFieldName(written without itsconst N), which kicks off anE0107 "missing generics"cascade pointing deep into macro code. ButFieldName::len(..)is just an associated fn, not a constructor, so lowering it like aTupleCallwas wrong in the first place. See #157152.Only an enum can host a tuple-variant ctor, so for any other self type that can't (struct, union, primitive, foreign type) the call has to be an assoc fn and needs wrapping in
const { ... }. We catch those from the self type's resolution before lowering it and emit the existing "complex const arguments must be placed inside of aconstblock" error, which is the message you'd want anyway. Enums, aliases,Selfand type params get left alone since they might resolve to an enum. tbh the bare generic enum case (Option::Some(0)) still E0107s, and imo that's better as a follow-up since catching it needs the variant type before lowering. Tests cover struct/union/primitive/foreign plus the wrapped forms that compile, and I checked it against the realtracing0.1.44 crate too.fwiw just the code changes and tests were implemented with AI help and I verified/reproduced/tested everything locally before sending to remote.