Skip to content

[WIP] mgca: Add ConstArg representation for const items #139558

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

camelid
Copy link
Member

@camelid camelid commented Apr 8, 2025

@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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Apr 8, 2025
@camelid camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 8, 2025
@rust-log-analyzer

This comment has been minimized.

@camelid camelid force-pushed the mgca-const-items branch from fa42f86 to 6054bd5 Compare April 9, 2025 16:14
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

let ty = this
.lower_ty(ty, ImplTraitContext::Disallowed(ImplTraitPosition::ConstTy));
let body =
this.lower_const_item(span, body_id.unwrap(), expr.as_deref().unwrap());
Copy link
Member Author

Choose a reason for hiding this comment

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

These new unwraps should be correct in theory since an error should've already been emitted, but they might ICE if the compiler didn't stop earlier. It might be a good idea to add ConstArgKind::Err and use delayed_span_bug.

@rust-log-analyzer

This comment has been minimized.

@camelid camelid force-pushed the mgca-const-items branch from 4f6c9ab to a46aa4c Compare May 15, 2025 12:37
@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added the WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) label May 15, 2025
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@camelid camelid added A-const-generics Area: const generics (parameters and arguments) F-min_generic_const_args `#![feature(min_generic_const_args)]` labels May 17, 2025
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented May 21, 2025

☔ The latest upstream changes (presumably #141343) made this pull request unmergeable. Please resolve the merge conflicts.

Comment on lines 217 to 250
DefKind::Const if tcx.generics_of(item_def_id).is_empty() => {
let instance = ty::Instance::new_raw(item_def_id.into(), ty::GenericArgs::empty());
let cid = GlobalId { instance, promoted: None };
let typing_env = ty::TypingEnv::fully_monomorphized();
tcx.ensure_ok().eval_to_const_value_raw(typing_env.as_query_input(cid));
}
Copy link
Member

@BoxyUwU BoxyUwU May 22, 2025

Choose a reason for hiding this comment

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

We should properly handle this before landing 🤔 don't need to care too much rn while everything is still ICEing. I expect we'll just want to update fn check_associated_item and fn check_item in wfcheck.rs to handle const aliases like we do type aliases and then get rid of this codepath

Comment on lines +233 to +302
/// Returns the const of the RHS of a const item.
query const_of_item(def_id: DefId) -> ty::EarlyBinder<'tcx, ty::Const<'tcx>> {
desc { |tcx| "computing the value for `{}`", tcx.def_path_str(def_id) }
cache_on_disk_if { def_id.is_local() }
separate_provide_extern
}
Copy link
Member

Choose a reason for hiding this comment

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

You'll need to do some more stuff to get this separate_provide_extern to actually work. You'll probably start getting ICEs in cross crate scenarios once you're past building core. See this commit for an example of adding a separate_provide_extern query: 996a185

Comment on lines +101 to +103
if let Ok(Some(Instance { def: ty::InstanceKind::Item(def_id), args })) = result
&& matches!(tcx.def_kind(def_id), DefKind::Const | DefKind::AssocConst)
{
Copy link
Member

Choose a reason for hiding this comment

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

So it turns out that not all CTFE entry points actually call try_resolve, some of them just manually construct an instance for a const item. Not sure what to do about that long term 🤔

For now though you can go through all the entry points in rustc_middle/src/mir/interpret/queries and update everything to use Instance::expect_resolve instead of Instance::new_raw which should atleast solve any ICEs in the short term until we figure out how to properly handle this

@BoxyUwU
Copy link
Member

BoxyUwU commented May 23, 2025

@oli-obk I've assigned you to this PR alongside me because I'd definitely want you to review it before it lands since its so CTFE involved 🤔 I don't think it needs reviewing rn though, things are so up in the air and we're not bootstrapping yet :3

@camelid camelid force-pushed the mgca-const-items branch from 786f88a to 5ea917f Compare May 25, 2025 22:06
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

Comment on lines +111 to +114
ty::AnonConstKind::MCG
if matches!(parent_def_kind, DefKind::Const | DefKind::AssocConst) =>
{
Some(parent_did)
}
Copy link
Member

Choose a reason for hiding this comment

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

This will probably make the anon const in const FOO: [u8; ...] have generics too which is not desirable 🤔 Should look at the HIR instead to see if the parent hir node of the ConstArg node is a const item or not. Not sure whether it makes sense to add a new variant to ty::AnonConstKind or not, I guess it depends on whether we wind up special casing them elsewhere (e.g. HIR ty lowering).

Actually that reminds me- we should have a test about const ASSOC: usize = (N);. It's possible that will get lowered to an anon const instead of a path (expected) and then ICE in ty lowering due to being a bare usage of a generic parameter not lowered to a path. Properly fixing this might just require changing ty lowering to not do anything fancy for anon consts used as the rhs of const items, which probably motivates adding a ty::AnonConstKind::ConstBody

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented May 27, 2025

☔ The latest upstream changes (presumably #141605) made this pull request unmergeable. Please resolve the merge conflicts.

@rustbot rustbot added the T-clippy Relevant to the Clippy team. label Aug 4, 2025
* Centralize const item lowering logic
* Add dummy NodeId to AST const item for mgca lowering
* wip on using ConstArg for const item bodies
* wip on fixing const normalization ICEs
* Extract helper function for normalizing free/assoc terms
* more wip on fixing ICEs
* Recurse through const items in instance resolution
* Fix more ICEs
* Use AnonConst as RHS of const items instead of side channel hack
* Implement type_of for anon consts on RHS of const items
* Fix yet more ICEs
* wip on fixing more ICEs (this might make it worse)
* Fix clippy and rustfmt
* Address some review comments
* Inherit generics correctly for anon const as const item RHS
* Lower const item RHS to const path even on stable
* wip on moving const RHS special-casing
* intern + write to place
* allow anon consts with expected types with generics

Co-authored-by: Boxy <[email protected]>
@rust-log-analyzer
Copy link
Collaborator

The job tidy failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)
tidy check
Checking tidy rustdoc_json...
No error code explanation was removed!
tidy: Skipping binary file check, read-only filesystem
##[error]tidy error: /checkout/compiler/rustc_lint/src/context.rs:976: TODO is used for tasks that should be done before merging a PR; If you want to leave a message in the codebase use FIXME
##[error]tidy error: /checkout/compiler/rustc_hir/src/hir.rs:4324: TODO is used for tasks that should be done before merging a PR; If you want to leave a message in the codebase use FIXME
removing old virtual environment
creating virtual environment at '/checkout/obj/build/venv' using 'python3.10' and 'venv'
creating virtual environment at '/checkout/obj/build/venv' using 'python3.10' and 'virtualenv'
Requirement already satisfied: pip in ./build/venv/lib/python3.10/site-packages (25.1.1)
Collecting pip
---

added 291 packages in 38s
Running eslint on rustdoc JS files
info: ES-Check: there were no ES version matching errors!  🎉
some tidy checks failed
Command has failed. Rerun with -v to see more details.
Build completed unsuccessfully in 0:01:55
  local time: Mon Aug  4 03:24:02 UTC 2025
  network time: Mon, 04 Aug 2025 03:24:02 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-generics Area: const generics (parameters and arguments) F-min_generic_const_args `#![feature(min_generic_const_args)]` S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-clippy Relevant to the Clippy team. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants