Move abstract const to middle #99000

JulianKnodt · 2022-07-07T04:25:56Z

Moves AbstractConst (and all associated methods) to rustc middle for use in rustc_infer.
This allows for const resolution in infer to use abstract consts to walk consts and check if
they are resolvable.

This attempts to resolve the issue where Foo<{ concrete const }, generic T> is incorrectly marked as conflicting, and is independent from the other issue where nested abstract consts must be resolved.

r? @lcnr

rustbot · 2022-07-07T04:25:59Z

Some changes occurred in const_evaluatable.rs

cc @lcnr

lcnr

please move some more stuff, then r=me

compiler/rustc_infer/src/infer/mod.rs

compiler/rustc_middle/src/thir/abstract_const.rs

bors · 2022-07-08T18:30:44Z

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

bors · 2022-07-09T20:41:13Z

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

BoxyUwU · 2022-07-11T09:07:32Z

doesnt seem like this PR has any changes to rustc_infer to solve the bug this PR is talking about.

compiler/rustc_ty_utils/src/consts.rs

compiler/rustc_middle/src/ty/abstract_const.rs

lcnr · 2022-07-11T09:13:06Z

compiler/rustc_middle/src/ty/abstract_const.rs

+#[instrument(skip(tcx), level = "debug")]
+pub fn try_unify_abstract_consts<'tcx>(
+    tcx: TyCtxt<'tcx>,
+    (a, b): (ty::Unevaluated<'tcx, ()>, ty::Unevaluated<'tcx, ()>),
+    param_env: ty::ParamEnv<'tcx>,
+) -> bool {
+    (|| {
+        if let Some(a) = AbstractConst::new(tcx, a)? {
+            if let Some(b) = AbstractConst::new(tcx, b)? {
+                let const_unify_ctxt = ConstUnifyCtxt { tcx, param_env };
+                return Ok(const_unify_ctxt.try_unify(a, b));
+            }
+        }
+
+        Ok(false)
+    })()
+    .unwrap_or_else(|_: ErrorGuaranteed| true)
+    // FIXME(generic_const_exprs): We should instead have this
+    // method return the resulting `ty::Const` and return `ConstKind::Error`
+    // on `ErrorGuaranteed`.
+}


it feels weird to have this be a query which is provided in rustc_trait_selectioneven though it's fully defined in rustc_middle.

I guess we actually want to keep try_unify_abstract_const and the ConstUnifyCtxt in rustc_trait_selection for now?

i feel like this should be in rustc_trait_selection still since it seems reasonable for try_unify to want to be able to unify inference vars eventaully- although "eventually" we probably want to remove try_unify_abstract_consts so I dont know how important this is

still relevant

JulianKnodt · 2022-07-12T00:33:10Z

@BoxyUwU oops I initially made this PR to fix the bug, but turns out I would have to move this to rustc_middle::ty, and doing both seemed to mix two different concerns

compiler/rustc_infer/src/infer/mod.rs

compiler/rustc_middle/src/ty/abstract_const.rs

compiler/rustc_trait_selection/src/traits/const_evaluatable.rs

compiler/rustc_ty_utils/src/consts.rs

JulianKnodt · 2022-07-13T04:33:14Z

It seems that it works just by attempting the identity substs without actually walking the abstract const,
so I removed the HasTyError variant from FailureKind (although it might make sense to have it in there otherwise).

compiler/rustc_infer/src/infer/mod.rs

lcnr

there's still a final nit, and then r=me ❤️ thanks a lot

lcnr · 2022-07-14T06:30:01Z

compiler/rustc_middle/src/ty/abstract_const.rs

+#[instrument(skip(tcx), level = "debug")]
+pub fn try_unify_abstract_consts<'tcx>(
+    tcx: TyCtxt<'tcx>,
+    (a, b): (ty::Unevaluated<'tcx, ()>, ty::Unevaluated<'tcx, ()>),
+    param_env: ty::ParamEnv<'tcx>,
+) -> bool {
+    (|| {
+        if let Some(a) = AbstractConst::new(tcx, a)? {
+            if let Some(b) = AbstractConst::new(tcx, b)? {
+                let const_unify_ctxt = ConstUnifyCtxt { tcx, param_env };
+                return Ok(const_unify_ctxt.try_unify(a, b));
+            }
+        }
+
+        Ok(false)
+    })()
+    .unwrap_or_else(|_: ErrorGuaranteed| true)
+    // FIXME(generic_const_exprs): We should instead have this
+    // method return the resulting `ty::Const` and return `ConstKind::Error`
+    // on `ErrorGuaranteed`.
+}


still relevant

lcnr · 2022-07-14T08:30:01Z

@bors delegate+

bors · 2022-07-14T08:30:04Z

✌️ @JulianKnodt can now approve this pull request

lcnr · 2022-07-14T09:03:09Z

@bors r+

bors · 2022-07-14T09:03:10Z

📌 Commit 20fb8ab has been approved by lcnr

It is now in the queue for this repository.

Rollup of 6 pull requests Successful merges: - rust-lang#98072 (Add provider API to error trait) - rust-lang#98580 (Emit warning when named arguments are used positionally in format) - rust-lang#99000 (Move abstract const to middle) - rust-lang#99192 (Fix spans for asm diagnostics) - rust-lang#99222 (Better error message for generic_const_exprs inference failure) - rust-lang#99236 (solaris: unbreak build on native platform) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 7, 2022

rust-highfive assigned lcnr Jul 7, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 7, 2022

JulianKnodt force-pushed the allow_resolve_no_substs branch 4 times, most recently from 88d4c71 to b189045 Compare July 7, 2022 06:13