reimplement `~const Trait` bounds via a fourth kind of generic param by oli-obk · Pull Request #101900 · rust-lang/rust

oli-obk · 2022-09-16T13:12:26Z

tracking issue: #102090

bors · 2022-09-23T16:50:55Z

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

oli-obk · 2022-09-29T10:06:43Z

Looks like src/tools/jsondoclint is not covered by ./x.py check

bors · 2022-09-29T14:05:43Z

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

…fects

…eature gate

lcnr · 2023-01-12T11:45:11Z

-            .unwrap();
+            let substs = if self.tcx.effects() {
+                self.tcx
+                    .mk_substs(instance.substs.iter().chain([self.tcx.effects.always_const.into()]))


host_on or something?

I explicitly named it this way (it would be host_off fwiw) and I dislike inverses.

lcnr · 2023-01-12T11:45:31Z

+                                            )
+                                            .into()]
+                                            .into_iter()
+                                            .chain(tcx.host_effect()),


Suggested change

.chain(tcx.host_effect()),

.chain(tcx.default_host_effect()),

or something like that?

why are u using mk_trait_ref instead of mk_trait_ref_with_effect here?

Hmm... I do get the naming confusing between this not returning the effect kind, but a value of the host effect... I'll try out a few namings

why are u using mk_trait_ref instead of mk_trait_ref_with_effect here?

for diagnostics I opted to just always fall back to host-on. Many times we don't have any context information available anyway

lcnr · 2023-01-12T11:47:22Z

    /// Allows `dyn* Trait` objects.
    (incomplete, dyn_star, "1.65.0", Some(102425), None),
+    // Uses generic effect parameters for ~const bounds
+    (active, effects, "1.62.0", Some(102090), None),


Suggested change

(active, effects, "1.62.0", Some(102090), None),

(incomplete, effects, "1.62.0", Some(102090), None),

?

lcnr · 2023-01-12T12:17:04Z

        }
    }

+    if has_constness_effect {


has_host_effect.

lcnr · 2023-01-12T12:37:15Z

+        let flags = FlagComputation::for_effect(e);
+        if flags.intersects(self.needs_canonical_flags) { e.super_fold_with(self) } else { e }


why is that not simply happening in the _ => {} branch and we stop explicitly using return for the other branches.

lcnr · 2023-01-12T12:38:38Z

+    ) -> ty::Effect<'tcx> {
+        let infcx = self.infcx;
+        let bound_to = infcx.shallow_resolve(effect_var);
+        if bound_to != effect_var {


can you make that an assert instead? shouldn't we shallow resolve in fold_effect already?

lcnr · 2023-01-12T12:39:28Z

+                }
+                GenericArgKind::Effect(result_value) => {
+                    if let ty::EffectValue::Bound(debrujin, b) = result_value.val {
+                        // ...in which case we would set `canonical_vars[0]` to `Some(const X)`.


Some(const x)?

lcnr · 2023-01-12T12:42:36Z

                    GenericArgKind::Lifetime(_) => 0, // erased
                    GenericArgKind::Type(ty) => self.ty_cost(ty),
                    GenericArgKind::Const(_) => 3, // some non-zero value
+                    GenericArgKind::Effect(_) => 3, // some non-zero value


would put them together with regions, you cannot explicitly mention effects so they don't have a cost

lcnr · 2023-01-12T12:43:32Z

+                EffectVariableValue::Unknown { .. } => {}
+                EffectVariableValue::Known { .. } => continue,
+            }
+            let value = self.tcx.mk_effect(ty::EffectValue::Rigid { on: true }, origin.effect_kind);


not applying vs ty::EffectValue::Rigid { on: true },. afaict either the function comment or the impl is wrong

lcnr · 2023-01-12T12:52:10Z

+        b: ty::Effect<'tcx>,
+    ) -> RelateResult<'tcx, ty::Effect<'tcx>> {
+        match (a.val, b.val) {
+            // Any effect matches the rigid "on" effect. For example: `T: ~const Trait` (off) implies `T: Trait` (on)


effects and subtyping seems very sus. What's the default variance for effect parameters?

lcnr · 2023-01-12T12:57:39Z

            }
            ty::PredicateKind::Clause(ty::Clause::Projection(..)) => {
-                // Nothing to elaborate in a projection predicate.
+                // `<T as ~const Trait>::TYPE` implies `<T as Trait>::TYPE`


can you add a comment that this is only correct if the predicate has exactly 1 effect param? If there were multiple params i could imagine us wanting to add all possible combinations of const + ~const.

lcnr · 2023-01-12T13:06:47Z

+                GenericParamDefKind::Effect { kind } => kind == effect_kind,
+                _ => false,
+            })
+            .or_else(|| tcx.generics_of(self.parent?).effect_param(effect_kind, tcx))


i am not certain whether we will always only have 1 effect param of the same kind, or I can at least imagine future extensions which will break this code.

Can you add a Debug_assert to generics_of whichi check for this?

lcnr · 2023-01-12T13:07:53Z

                bug!("Instance::mono: {:?} has const parameters", def_id)
            }
+            ty::GenericParamDefKind::Effect { kind } => {
+                // FIXME: this is necessary for `panic` and friends in the monomorphization collector.


Suggested change

// FIXME: this is necessary for `panic` and friends in the monomorphization collector.

// FIXME(effects): this is necessary for `panic` and friends in the monomorphization collector.

lcnr · 2023-01-12T13:27:29Z

+        // hook in here to catch this case (annoying...), but
+        // otherwise we do want to remember to visit the rest of the
+        // effect, as it has types/regions embedded in a lot of other
+        // places.


i don't understand that comment 🤔

I think I copy pasted it from constants without looking at the details

lcnr · 2023-01-12T13:47:37Z

    tcx: TyCtxt<'tcx>,
    fn_trait_def_id: DefId,
    self_ty: Ty<'tcx>,
+    // Make this an array once the effect feature is always active


Suggested change

// Make this an array once the effect feature is always active

// FIXME(effects): Make this an array once the effect feature is always active

lcnr · 2023-01-12T13:47:58Z

+        };
+        let trait_ref = tcx.mk_trait_ref(
+            fn_trait_def_id,
+            // FIXME: have the caller pass the effects here and use them.


Suggested change

// FIXME: have the caller pass the effects here and use them.

// FIXME(effects): have the caller pass the effects here and use them.

lcnr · 2023-01-12T13:48:10Z

    tcx: TyCtxt<'tcx>,
    fn_trait_def_id: DefId,
    self_ty: Ty<'tcx>,
+    // Make this an array once the effect feature is always active


Suggested change

// Make this an array once the effect feature is always active

// FIXME(effects): Make this an array once the effect feature is always active

lcnr · 2023-01-12T13:49:45Z

-    #[rustc_const_unstable(feature = "const_alloc_layout", issue = "67521")]
    #[inline]
-    pub const fn extend(&self, next: Self) -> Result<(Self, usize), LayoutError> {
+    pub fn extend(&self, next: Self) -> Result<(Self, usize), LayoutError> {


why this change?

lcnr · 2023-01-12T13:50:01Z


    #[inline]
-    fn index(&self, index: I) -> &Self::Output {
+    fn index(&self, index: I) -> &<[T] as Index<I>>::Output {


same here, why these libs changes?

Because Self didn't work ^^ I'm still figuring out that part

lcnr · 2023-01-27T11:38:18Z


 impl const S {}
 //~^ ERROR inherent impls cannot be `const`
+//~| ERROR `host` is not constrained


adding an effect param for inherent impls seems bad ui wise ^^

yea I hate everything about it ^^ I'll remove it and just require everyone to repeat trait bounds on the functions for now

apiraino · 2023-02-01T22:38:44Z

If I am correct this can be switched to waiting on author. Feel free to request a review with @rustbot ready, thanks!

@rustbot author

oli-obk · 2023-03-14T22:25:23Z

This PR has become impossible to rebase. I'm starting from scratch on a new branch with a smaller MVP

rust-highfive assigned lcnr Sep 16, 2022

rustbot added 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 Sep 16, 2022

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