Patch 1660 Eliminate redundant constraints in Util, Repository and Patch

Title Eliminate redundant constraints in Util, Repository and Patch
Superseder Nosy List gh
Related Issues
Status needs-review Assigned To

Created on 2018-03-07.20:35:17 by gh, last changed 2018-03-12.02:36:00 by gh.

File name Status Uploaded Type Edit Remove
eliminate-redundant-constraints-in-darcs_util.dpatch gh, 2018-03-07.20:35:17 application/octet-stream
stop-using-patchy-in-the-test-harness.dpatch gh, 2018-03-10.20:38:17 application/octet-stream
See mailing list archives for discussion on individual patches.
msg19950 (view) Author: gh Date: 2018-03-07.20:35:17
I found these using the flag of GHC -fwarn-redundant-constraints .

As this flag returns sometimes false positives, I had to manually check
whether said constraints were really redundant by compiling.

The most common redundant constraints were:

* Functor when Monad is already required, not a problem since GHC 7.10
* Repopatch
* IsRepoType
* Patchy
* ApplyState p ~ Tree

The weirdest / most doubtful case is the following:

instance (IsHunk p, PatchListFormat p, Patchy p) => Patchy (FL p)
instance (IsHunk p, PatchListFormat p, Patchy p) => Patchy (RL p)

Which seems to only need:

instance (Patchy p) => Patchy (FL p)
instance (Patchy p) => Patchy (RL p)

I will screen this in a couple of days in case someone discovers this is

patch a448b9779110a652518dbfdd16e530d93215f150
Author: Guillaume Hoffmann <guillaumh@gmail.com>
Date:   Mon Feb 26 13:40:36 -03 2018
  * eliminate redundant constraints in Darcs.Repository

patch 0f1d11bd1d54e0f847f3f136592bcb0a82292621
Author: Guillaume Hoffmann <guillaumh@gmail.com>
Date:   Mon Feb 26 13:40:17 -03 2018
  * eliminate redundant constraints in Darcs.Patch

patch e8795ae71453accbb86dad3e4497151ad9612f81
Author: Guillaume Hoffmann <guillaumh@gmail.com>
Date:   Mon Feb 26 13:30:16 -03 2018
  * eliminate redundant constraints in Darcs.Util
msg19960 (view) Author: bf Date: 2018-03-10.09:21:03
I am screening them now, no problems I can see.
msg19963 (view) Author: gh Date: 2018-03-10.20:38:17
My previous comment about Patchy made me look into what was this
typeclass for.

Patchy is simply a subclass of Apply, Invert and Commute. The idea
seemed to be to write smaller contexts. However, I found that in most
places, Patchy was used instead of just one or two of the aforementioned

Also, "Patchy" is not a very informative name, unlike for instance,
Matchable, another empty class. Nor is it very all-encompassing, unlike
RepoPatch which is a subclass of almost 20 classes.

Moreover, many modules import Darcs.Patch.Patchy in order to import
other class definitions, so API-wise this does not look great.

So I went ahead and removed the Patchy class, and found that many
functions now have amore understandable context.

After that, I compiled again Darcs with -fwarn-redundant-constraints,
and could further removed unneeded constraints all over the place.

I'm screening this followup bundle.
msg19964 (view) Author: ganesh Date: 2018-03-11.11:09:18
I think that Patchy is also trying to express a higher-level concept, 
that these classes are the fundamentals of what it means to be a patch.
msg19965 (view) Author: bf Date: 2018-03-11.20:00:22
Yes! I agree fully wrt Patchy, been thinking similar thoughts now and again.
msg19966 (view) Author: gh Date: 2018-03-12.02:36:00
Ganesh: yes, and I guess this is correlated with the contemporary
documentation about patch theory, that talks about commutation and
inversion, but falls short of the issue of conflicts (Patchy is not a
subclass of Conflict nor Merge).

To make reviewing easier, here is a diff of before/after the patches of
this ticket:

Date User Action Args
2018-03-07 20:35:17ghcreate
2018-03-10 09:21:04bfsetstatus: needs-screening -> needs-review
messages: + msg19960
2018-03-10 20:38:18ghsetfiles: + stop-using-patchy-in-the-test-harness.dpatch
messages: + msg19963
2018-03-11 11:09:18ganeshsetmessages: + msg19964
2018-03-11 20:00:22bfsetmessages: + msg19965
2018-03-12 02:36:00ghsetmessages: + msg19966