allow mGCA const arguments to fall back to anon consts#158617
Conversation
|
The parser was modified, potentially altering the grammar of (stable) Rust cc @fmease
cc @rust-lang/rustfmt
cc @rust-lang/clippy Some changes occurred in compiler/rustc_builtin_macros/src/autodiff.rs cc @ZuseZ4 |
|
|
| ast::TyKind::DirectConstArg(ref expr) => { | ||
| let expr = expr.rewrite_result(context, shape)?; | ||
| Ok(format!("core::direct_const_arg!({expr})")) | ||
| } |
There was a problem hiding this comment.
Just double checking that core::direct_const_arg! won't show up as a macro call in the AST. Might be better to just return Err(RewriteError::Unknown) or even Ok(context.snippet(self.span).to_owned()) to return the span unchanged.
There was a problem hiding this comment.
ah, good point, I didn't quite realize how rustfmt works, thank you! could you please double-check what I just force-pushed to make sure it's what you were thinking of?
in particular, I'm not totally sure why ast::ExprKind::Err(_) | ast::ExprKind::Dummy return Err(RewriteError::Unknown), but ast::TyKind::Dummy | ast::TyKind::Err(_) return Ok(context.snippet(self.span).to_owned()). I kept doing that for DirectConstArg, but, yeah.
There was a problem hiding this comment.
i would expect this codepath to actually be unreachable. direct_const_arg! is a macro call in the AST and rustfmt will see the unexpanded AST so we'll never encounter a DirectConstArg expr/ty 🤔 does ICEing here cause any issues?
There was a problem hiding this comment.
oh this is the:
rustfmt tries to parse macro arguments when formatting macros, so it's not
totally impossible for rustfmt to come across one of these nodes when formatting
a file
thats funny
| rhs: Some(self.parse_expr_anon_const(|_, _| MgcaDisambiguation::Direct)?), | ||
| }, | ||
| (true, true) => { | ||
| ConstItemRhsKind::TypeConst { rhs: Some(self.parse_expr_anon_const()?) } |
There was a problem hiding this comment.
we should track somewhere to stop parsing type const rhs' differently than normal const items I think :3 this goes hand in hand with i guess the stuff about lowering the rhs of type consts as if they were direct'd?
There was a problem hiding this comment.
precisely and exactly. very much on my todo list for the followup I was talking about!
| self.with_parent(parent, |this| visit::walk_anon_const(this, constant)); | ||
| } | ||
| }; | ||
| let parent = |
There was a problem hiding this comment.
hurray less def collector and parsing jank 😌
| /// it cannot. | ||
| #[instrument(level = "debug", skip(self), ret)] | ||
| fn lower_expr_to_const_arg_direct(&mut self, expr: &Expr) -> hir::ConstArg<'hir> { | ||
| fn try_lower_expr_to_const_arg_direct( |
There was a problem hiding this comment.
this fn is like slightly scuffed but I need to think a bit about why that is and what an alternative might be :3
There was a problem hiding this comment.
it's SO scuffed, and it's gonna get worse once we introduce macroful gca.
the main issue IMO is that we need a separate "check" and "actually do" phase, because we could fail several layers deep - e.g. we're inside a tuple, ((lowering, stuff), (and + then + we + error)), if we have already lowered and allocated arena memory when lowering (lowering, stuff), we cannot then bail out when we encounter the addition expr, because then all those generated IDs and arena memory and stuff would get unused.
| ExprKind::Tup(exprs) if is_mgca => { | ||
| if check_only { | ||
| for expr in exprs { | ||
| let _ = self.try_lower_expr_to_const_arg_direct(expr, None, check_only)?; |
There was a problem hiding this comment.
iirc the reason we recurse into tuple elements but not path arguments is that we can't support (N, 1 + 1) without (N, const { 1 + 1 }) because we dont want to make a defid for all tuple element exprs in advance so that we can reuse the defid here
should write that down somewhere in here as the inconsistency between paths and other exprs feels slightly weird :3
There was a problem hiding this comment.
yeah. if we generated defids for everything, we wouldn't need this weird separate "check" and "actually do" phases, because we could always bail to representing things as anon consts if we fail to lower some later tuple element after successfully lowering previous tuple elements, and (N, 1 + 1) would sorta implicitly have a const { 1 + 1 } block.
... but generating defids for everything is, Not Great
| }, | ||
| _ => false, | ||
| } | ||
| && matches!(tcx.hir_node_by_def_id(local_id), hir::Node::ConstArg(_)) |
There was a problem hiding this comment.
it would somewhat not surprise me if this causes ICEs, I would expect that if we have these defids in the parent tree then we actually do need to encode them 🤔
| #![feature(min_generic_const_args)] | ||
| #![allow(incomplete_features)] | ||
| pub struct S<const N: usize>; | ||
| pub fn f() -> S<{ const { 1 + 1 } }> { |
There was a problem hiding this comment.
is this an anon const with a const block inside it? can we have comment about what we expect this to lower to :3
| let mut parser = cx.new_parser_from_tts(tts); | ||
| let expr = match parser.parse_expr() { | ||
| Ok(parsed) => parsed, | ||
| Err(err) => { | ||
| return ExpandResult::Ready(DummyResult::any(span, err.emit())); | ||
| } | ||
| }; |
There was a problem hiding this comment.
I have this question, like do we expect the direct_const_arg! to contain multiple expressions?
There was a problem hiding this comment.
good point! fixed now :3
eb61cdf to
44acd42
Compare
This comment has been minimized.
This comment has been minimized.
|
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. |
|
r=me if CI passes |
makes good progress on rust-lang/project-const-generics#108
min_generic_const_args(mGCA) is all about "is this const lowered to a const item with a body, or, is it "directly" represented and visible in the type system?"Here's various answers to the question of "can we represent this const arg directly?" - if we answer "no", then we fall back to an anon const (or error, sometimes, depending on context)
on stable, the full list of things can be directly represented is (... it's just the one thing)
on
main, under mGCA, before this PR:const { }gives you an escape hatch to go back to being an anon conston macroful gca, the directly represented things will be:
direct_const_arg!macroon macroless gca, the directly represented things will be:
direct_const_arg!macro (not particularly useful under macroless gca)this PR implements macroless gca, with the intent of a quick follow-up introducing a "macroless gca" feature and transitioning
#[feature(min_generic_const_args)]to be macroful gcaAlso of note is that this PR removes the logic to skip generating "fake" DefIds for directly represented AnonConsts under mGCA, we now always generate a DefId when encountering an AnonConst. This lines up with stable, which has a fake AnonConst DefId for a directly represented plain path generic parameter (i.e. the bullet point under the "stable" entry above).
r? @BoxyUwU