Skip to content

clean up handling of paths in mgca - #160216

Open
khyperia wants to merge 1 commit into
rust-lang:mainfrom
khyperia:gca-path-cleanup
Open

clean up handling of paths in mgca#160216
khyperia wants to merge 1 commit into
rust-lang:mainfrom
khyperia:gca-path-cleanup

Conversation

@khyperia

@khyperia khyperia commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

related tracking issue: #132980

Handling of ambiguous type-or-constant argument paths was really wonky and distinct from the regular can_lower_expr_to_const_arg_direct lowering paths. Clean that up and unify it, as discussed here #158479 (comment)

Also, do a bunch of other little cleanups at the same time while I'm here:

  • add support for parens to the inside of direct_const_arg! and macroless_generic_const_args (idk why this wasn't a thing before)
  • add constant and lifetime ribs when resolving TyKind::DirectConstArg - note the TyKind, not ExprKind (I am unsure if/how this changes behavior added a test, direct-const-arg-correct-rib.rs) - this was discussed here allow mGCA const arguments to fall back to anon consts #158617 (comment)
  • minor debug logging change
  • various minor code shuffles/cleanups
  • add test for direct_const_arg!(_) inferring to a type - this test should have been added in implement #![feature(macroless_generic_const_args)] #159058 which added this behavior (well, changed the behavior from an ICE to inferring to a type)
  • add a test asserting the current behavior of parens/braces/underscores on stable - one or two of these cases already have tests elsewhere, but it's nice to assert the whole set, and also assert the whole set in the same place so it's easily visible to be able to give me confidence the whole set actually is tested (both positives and negatives are important here)
  • add a test similar to the previous one, but with mgca enabled, asserting all combinations now compile successfully

r? @BoxyUwU

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 30, 2026
@rustbot

rustbot commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

BoxyUwU is currently at their maximum review capacity.
They may take a while to respond.

@rust-log-analyzer

This comment has been minimized.

@khyperia
khyperia force-pushed the gca-path-cleanup branch 2 times, most recently from 8dda2c6 to 5b41ff7 Compare July 31, 2026 07:44

@bit-aloo bit-aloo left a comment

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.

LGTM. Few questions and nits.

View changes since this review

Comment thread compiler/rustc_ast_lowering/src/lib.rs Outdated
}

fn can_lower_path_to_const_arg_direct(
&mut self,

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.

nit:

Suggested change
&mut self,
&self,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

thanks! I also removed the mut from can_lower_expr_to_const_arg_direct :3

Comment thread compiler/rustc_resolve/src/late.rs Outdated
TyKind::DirectConstArg(expr) => self.resolve_anon_const_manual(
true,
AnonConstKind::ConstArg(IsRepeatExpr::No),
|this| this.resolve_expr(&expr, None),

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.

nit:

Suggested change
|this| this.resolve_expr(&expr, None),
|this| this.resolve_expr(expr, None),

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

out of curiosity, how did you spot this? just a good eye, or do you have some sort of setup where you can run clippy locally or something, and you checked out my code?

{
let ct =
self.lower_const_path_to_const_arg(&None, path, res, ty.id, ty.span);
let ct = self.arena.alloc(ct);

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.

Curious why we do arena alloc here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

  • GenericArg::Const(&'hir ConstArg<..>) is arena'd, so it needs an arena alloc somewhere
  • lower_anon_const_to_const_arg returns ConstArg by value (this has been the case since before I started working on this code)
    • there's a lower_anon_const_to_const_arg_and_alloc wrapper, fwiw
  • which requires lower_expr_to_const_arg_direct to return it by value
  • which requires lower_path_to_const_arg_direct to do the same
  • and then I said shrug, make lower_const_path_to_const_arg do the same, since it's a parallel to lower_anon_const_to_const_arg
  • so we arena alloc in the caller of lower_const_path_to_const_arg, here

there's no particular reason why this whole set of functions (lower_anon_const_to_const_arg/lower_expr_to_const_arg_direct/lower_const_path_to_const_arg/lower_path_to_const_arg_direct) returns by value instead of by &'hir, could go the other way around, but they should all be consistently the same IMO.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I guess in other words:

the diff adds an arena alloc here, because I removed it from lower_const_path_to_const_arg, because I wanted lower_const_path_to_const_arg to be more parallel to lower_anon_const_to_const_arg: they are conceptually extremely similar functions (take an X, attempt to lower it to a direct const arg, and fall back to anon const if that fails), just X is a path for one, and an AST anon const in the other.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants