Skip to content
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

Refactor Fold to take by value #642

Closed
detrumi opened this issue Oct 30, 2020 · 5 comments · Fixed by #660
Closed

Refactor Fold to take by value #642

detrumi opened this issue Oct 30, 2020 · 5 comments · Fixed by #660
Labels
good first issue A good issue to start working on Chalk with

Comments

@detrumi
Copy link
Member

detrumi commented Oct 30, 2020

Currently, the Folder trait takes folded values by reference. As part of the shared type library plan, the fold traits should take their folded values by value, to match the TypeFolder trait in rustc.

For example, this

fn fold_ty(&mut self, ty: &Ty<I>, outer_binder: DebruijnIndex) -> Fallible<Ty<TI>>

should be changed to this:

fn fold_ty(&mut self, ty: Ty<I>, outer_binder: DebruijnIndex) -> Fallible<Ty<TI>>

Same thing with the other folding functions.
Lastly, code that calls folding functions should pass the values by value, instead of by reference.

@detrumi detrumi added the good first issue A good issue to start working on Chalk with label Oct 30, 2020
@lnicola
Copy link
Member

lnicola commented Oct 30, 2020

Should the Fold implementations and trait be updated to also pass by value, or can they copy/clone?

impl<I: Interner, TI: TargetInterner<I>> Fold<I, TI> for Const<I> {
    type Result = Const<TI>;

    fn fold_with<'i>(
        &self,
        folder: &mut dyn Folder<'i, I, TI>,
        outer_binder: DebruijnIndex,
    ) -> Fallible<Self::Result>
    where
        I: 'i,
        TI: 'i,
    {
        folder.fold_const(self /* <-- this */, outer_binder)
    }
}

Clone works, since they all use interned values, but I guess the point is to make this change everywhere?

@detrumi
Copy link
Member Author

detrumi commented Oct 30, 2020

Yes, because fold_with should also take self by value. The Fold and SuperFold traits should also be updated here.

@lnicola
Copy link
Member

lnicola commented Oct 30, 2020

Sorry for the noob questions, but is it all right to add Clone bounds to the T in u_canonicalize and map_from_canonical? They seem to call fold_with on borrowed values.

@detrumi
Copy link
Member Author

detrumi commented Oct 30, 2020

Sorry for the noob questions, but is it all right to add Clone bounds to the T in u_canonicalize and map_from_canonical? They seem to call fold_with on borrowed values.

You mean the calls like this?

            .fold_with(
                &mut UMapFromCanonical {
                    interner,
                    universes: self,
                },
                DebruijnIndex::INNERMOST,
            )

I think you can remove the &mut and have it work, no need to clone here.
Oh wait, that doesn't work, because it's the self that needs to be cloned. Nvm

@lnicola
Copy link
Member

lnicola commented Nov 27, 2020

I tried to make this work, but couldn't manage. I left some notes in #644 (comment) and the branch is up, in case someone else wants to try.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue A good issue to start working on Chalk with
Projects
None yet
2 participants