For discussion initially.

It's inspired by the discussion about replacing Apply with
Effect+PrimApply as it would be necessary to break a
dependency cycle in that refactoring, but it makes sense
anyway.

It's also in keeping with the recent changes to make
PrimPatch and RepoPatch into type synonyms and the
design of Ident/PatchId.

1 patch for repository darcs-unstable@darcs.net:screened:

patch d91deaee5a0b51ae35db89e3b9a891612b27e5a7
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Mon Sep  2 23:00:01 BST 2019
  * Move PrimOf out of PrimPatchBase
  
  PrimPatchBase becomes a type synonym, and could perhaps
  just be removed altogether.

Attachments

• move-primof-out-of-primpatchbase.dpatch
• patch-preview.txt
• unnamed

Constraint synonyms have one disadvantage that I wasn't aware of until
recently. A superclass constraint can include a constraint on a type
family application e.g.

  class PrimApply (PrimOf p) => Whatever p

But of you use such a constraint for an instance (directly or via a
constraint synonym), then GHC complains and tells us it needs
UndecidableInstances.

It is interesting that this doesn't seem to apply to equality
constraints, i.e. Applystate p ~ ApplyState (PrimOf p) is okay, even on
an instance.

I am not happy about having to enable UndecidableInstances all over the
place. We need to find a better solution. These modules have it ATM:

src/Darcs/Patch/ApplyMonad.hs
src/Darcs/Patch/Rebase/Fixup.hs
src/Darcs/Patch/Rebase/Container.hs
src/Darcs/Patch/Rebase/Viewing.hs
src/Darcs/Patch/Rebase/Item.hs
src/Darcs/Patch/V2/Non.hs
src/Darcs/Patch/V2/Prim.hs
src/Darcs/Patch/V1/Prim.hs

I think all of them are due to the same issue above i.e. we need to have
"Class (TypeFunction p) => instance Whatever p".

> Constraint synonyms have one disadvantage that I wasn't aware of until
> recently. A superclass constraint can include a constraint on a type
> family application e.g.
> 
>   class PrimApply (PrimOf p) => Whatever p
> 
> But of you use such a constraint for an instance (directly or via a
> constraint synonym), then GHC complains and tells us it needs
> UndecidableInstances.

Ah yes, I meant to mention that. I actually thought some of the code
that uses RepoPatch/PrimPatch needed it too after the synonym was
introduced, but perhaps I was wrong.

> I am not happy about having to enable UndecidableInstances all over the
> place. We need to find a better solution. These modules have it ATM:

I guess that it's either use UndecidableInstances or keep the class
(with no members).

Here's an updated version that preserves PrimPatchBase,
so doesn't require any more UndecidableInstances.

Ben, what do you think?

1 patch for repository darcs-unstable@darcs.net:screened:

patch c86f011113412fc60b579238d19f8508ba5d4a19
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Tue Sep 17 17:56:29 BST 2019
  * Move PrimOf out of PrimPatchBase
  
  Quite a few PrimPatchBase instances then turn out to be
  unnecessary.

Attachments

• move-primof-out-of-primpatchbase.dpatch
• patch-preview.txt
• unnamed

> Here's an updated version that preserves PrimPatchBase,
> so doesn't require any more UndecidableInstances.
> 
> Ben, what do you think?

I am undecided. I don't see a clear win doing this but neither do I see
us losing anything. If you feel this is an improvement or if it helps
you with some other refactor, go ahead, no objections. Otherwise I'd say
let's save this until we have a pressing need. I am fine either way.

> I am undecided. I don't see a clear win doing this but neither do I see
> us losing anything. If you feel this is an improvement or if it helps
> you with some other refactor, go ahead, no objections. Otherwise I'd say
> let's save this until we have a pressing need. I am fine either way.

OK, I'm leaning towards having it because reducing the number of
PrimPatchBase instances seems nice. But I'll leave it for a bit to see
if I still think it's a good idea.

It is a pre-requisite for patch1914 but I think the merits of that are
very much in doubt.

> It is a pre-requisite for patch1914 but I think the merits of that are
> very much in doubt.

Do you really mean patch1914? (which is in screened)

On 18/09/2019 17:49, Ben Franksen wrote:
> 
> Ben Franksen <ben.franksen@online.de> added the comment:
> 
>> It is a pre-requisite for patch1914 but I think the merits of that are
>> very much in doubt.
> 
> Do you really mean patch1914? (which is in screened)

Sorry, patch1919.