-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
merge DefKind::InlineConst into AnonConst #158767
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -162,13 +162,17 @@ pub(super) fn generics_of(tcx: TyCtxt<'_>, def_id: LocalDefId) -> ty::Generics { | |
| ty::AnonConstKind::GCE => Some(parent_did), | ||
|
|
||
| // Field defaults are allowed to use generic parameters, e.g. `field: u32 = /*defid: N + 1*/` | ||
| ty::AnonConstKind::NonTypeSystem | ||
| ty::AnonConstKind::NonTypeSystemAnon | ||
| if matches!(tcx.parent_hir_node(hir_id), Node::TyPat(_) | Node::Field(_)) => | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm yeah I wouldn't expect pattern types to contain non type system anon consts? that feels weird :3 |
||
| { | ||
| Some(parent_did) | ||
| } | ||
| // Default to no generic parameters for other kinds of anon consts | ||
| ty::AnonConstKind::NonTypeSystem => None, | ||
| ty::AnonConstKind::NonTypeSystemAnon => None, | ||
| ty::AnonConstKind::NonTypeSystemInline => span_bug!( | ||
| tcx.def_span(def_id), | ||
| "a DefKind::AnonConst with a HIR parent of hir::Node::AnonConst should never be an AnonConstKind::NonTypeSystemInline" | ||
| ), | ||
| } | ||
| } | ||
| Node::ConstBlock(_) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -191,7 +191,7 @@ pub fn check_crate(tcx: TyCtxt<'_>) { | |
| // `feed_anon_const_type`. | ||
| // Also skip items for which typeck forwards to parent typeck. | ||
| if !(def_kind == DefKind::AnonConst | ||
| || def_kind == DefKind::InlineConst && tcx.is_type_system_inline_const(item_def_id) | ||
| && tcx.anon_const_kind(item_def_id) != ty::AnonConstKind::NonTypeSystemInline | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this now allows feeding
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was previously allowed! This is one of the cases where my eyebrows were raised:
With this PR, before this PR, as well as before #158375 we blindly accepted all
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh that's quite fun, I would expect we never feed non type system anon consts lol. cc @oli-obk fine to keep behaviour the same for this PR |
||
| || tcx.is_typeck_child(item_def_id.to_def_id())) | ||
| { | ||
| tcx.ensure_ok().typeck(item_def_id); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unsure about this catch-all, tbh. There's three options I see:
AnonConstKindthat actually list which specific kind of anon const this non-type-system anon const is - for example, having the AnonConstKind variants ofConstBlock,AsmConstBlock,FieldAnonConst,VariantAnonConst, and then perhaps afn AnonConstKind::is_type_system() -> boolor somethin'View changes since the review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it might make sense to make this
NonTypeSystem(NonTypeSystemAnonConstKind)(better name pending) 🤔 unsure :3 I like the option of specifying which kind of anon const it is if that makes a lot of callers simpler (and it's weird to only do it for inline vs non inline lol)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
context for others: boxy and I discussed this in DMs quite a bit, the conclusion was that this PR exposes a lot of questionable jank, and we may want to do larger refactorings in this area. However, that's out of scope for this PR, so leaving this as a catch-all for now is fine.