9 patches for repository http://darcs.net/screened:

patch 1f19c2b69aa92fda58ef17ecedd4e7f7a7f2d2e8
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jan 18 19:17:21 CET 2021
  * inline D.P.Rebase.toRebaseChanges into its only call site
  
  The function was badly named anyway.

patch 896349529a4e5641c3f763bff1542221bd3ee831
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jan 18 19:34:56 CET 2021
  * remove an unnecessary type application

patch 1255861d3ffea00ec174fc758ce77e172edfabd6
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jan 18 20:07:08 CET 2021
  * move Darcs.Patch.Named.Wrapped to Darcs.Patch.Rebase.Legacy

patch e1f1c93eff0d28248c2f1123016bb012546a3cab
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jan 18 21:03:20 CET 2021
  * remove two unneeded instances for WrappedNamed

patch d3f501be875c2b52b5db0e866f0e55e273f4cb21
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jan 18 21:04:15 CET 2021
  * impossible case in instance Commute WrappedNamed is now an error

patch 961041416cb567d96f3db962beb3b2238f28517b
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jan 18 21:05:59 CET 2021
  * convert WrappedNamed into Named directly in commuteOutOldStyleRebase

patch 963778ac7b604b3dc9be2bbaf69afabf0672cf1c
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jan 19 11:10:02 CET 2021
  * remove instance Commute WrappedNamed
  
  This instance has impossible cases and we also need it only in one
  direction, namely to commute out a mixed-in rebase patch. It is thus more
  straight-forward to use removeFixupsFromSuspended directly when we extract
  such a rebase state (to upgrade an old-style rebase). This also simplifies
  upgradeOldStyleRebase because we now unwrap the Suspended immediately.

patch 38a6d48e4cb9b654ececbe3541b3dbf3586ea97f
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jan 19 11:29:40 CET 2021
  * remove all remaining instances for WrappedNamed except ReadPatch

patch 853ea5a273099df17c00422b02464b9a847eed7e
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Jan 20 13:58:54 CET 2021
  * cleanup exports in Darcs.Repository.Rebase

Attachments

• inline-d_p_rebase_torebasechanges-into-its-only-call-site.dpatch
• patch-preview.txt
• unnamed

>   * inline D.P.Rebase.toRebaseChanges into its only call site

OK

>   * remove an unnecessary type application
OK, assuming it compiles with all our supporter compilers.

Minor nitpick: I wouldn't have removed the forall in the type definition
of toRebaseChanges as these often turn out to be needed again in the
future. Also the indentation of the reformatted signature isn't
resistant to changes in the name of the function. Neither are big deals.

>   * move Darcs.Patch.Named.Wrapped to Darcs.Patch.Rebase.Legacy

OK

>   * remove two unneeded instances for WrappedNamed

OK

>   * impossible case in instance Commute WrappedNamed is now an error

OK

>   * convert WrappedNamed into Named directly in commuteOutOldStyleRebase

OK

>   * remove instance Commute WrappedNamed

> -- | The opposite of sealing is to duplicate a single witness. This is for
> -- situations where a patch-like type is expected (i.e. one with two witnesses)
> -- but you have only one. Naturally, any concrete value must have both witnesses
> -- agreeing.

I disagree that this is the opposite of sealing, so I would prefer to
remove that sentence. Not a big deal though.

>       writeTentativeInventory (repoCache repo) compr (PatchSet ts (unsafeCoerceP ps))

I think this is a new unsafeCoerce - do you know why it's needed?

The rest of the change looks ok.

>   * remove all remaining instances for WrappedNamed except ReadPatch
>   * cleanup exports in Darcs.Repository.Rebase

Both fine.

>>   * remove an unnecessary type application
> OK, assuming it compiles with all our supporter compilers.

This is checked by the github CI which I trigger regularly, usually
before I send a patch bundle.

> Minor nitpick: I wouldn't have removed the forall in the type definition
> of toRebaseChanges as these often turn out to be needed again in the
> future.

But you have to admit that the end result is less cluttered now, and
thus easier to read.

> Also the indentation of the reformatted signature isn't
> resistant to changes in the name of the function.

I merely followed the convention we use almost everywhere else. I agree
that this convention is not ideal for the reason you stated and I am
open to a change here. For type signatures that don't fit on a single
line you seem to prefer

fun
  :: (Constraints)
  => x
  -> y

I could adopt that style. Are you using an auto-formatter? I have used
hindent for a long time, but it has lots of bugs and often the
formatting is not quite what I want so I have to amend the result
manually. I have just now installed brittany and it seems to work a lot
more reliably and does indeed format type signatures in that way.

>>   * remove instance Commute WrappedNamed
> 
>> -- | The opposite of sealing is to duplicate a single witness. This is for
>> -- situations where a patch-like type is expected (i.e. one with two witnesses)
>> -- but you have only one. Naturally, any concrete value must have both witnesses
>> -- agreeing.
> 
> I disagree that this is the opposite of sealing, so I would prefer to
> remove that sentence.

Well, 'Sealed (Dup p wX)' is isomorphic to 'p wX':

fromSealedDup :: Sealed (Dup p wX) -> p wX
fromSealedDup (Sealed (Dup x)) = x

toSealedDup :: p wX -> Sealed (Dup p wX)
toSealedDup x = Sealed (Dup x)

does typecheck, and the two functions are obviously inverses.

So Dup is the opposite of Sealed in the same sense in that
pretty-printing is the opposite of parsing.

I should perhaps elaborate the haddocks to make that clearer.

>>       writeTentativeInventory (repoCache repo) compr (PatchSet ts (unsafeCoerceP ps))
> 
> I think this is a new unsafeCoerce - do you know why it's needed?

Not really. I don't think the new code does anything fundamentally
different wrt the witnesses. The error message says "‘wX0’ is
untouchable" and I have only a vague idea what that means.

On 06/04/2021 07:24, Ben Franksen wrote:
>> Minor nitpick: I wouldn't have removed the forall in the type definition
>> of toRebaseChanges as these often turn out to be needed again in the
>> future.
> 
> But you have to admit that the end result is less cluttered now, and
> thus easier to read.

I guess the foralls are generically clutter, but I'm fairly used to them
now. They have the advantage of making the type variable ordering
explicit for TypeApplications and that defaulting to having them there
reduces churn.

>> Also the indentation of the reformatted signature isn't
>> resistant to changes in the name of the function.
> 
> I merely followed the convention we use almost everywhere else. I agree
> that this convention is not ideal for the reason you stated and I am
> open to a change here. For type signatures that don't fit on a single
> line you seem to prefer
> 
> fun
>   :: (Constraints)
>   => x
>   -> y
> 
> I could adopt that style. Are you using an auto-formatter? I have used
> hindent for a long time, but it has lots of bugs and often the
> formatting is not quite what I want so I have to amend the result
> manually. I have just now installed brittany and it seems to work a lot
> more reliably and does indeed format type signatures in that way.

No, I'm not using an auto-formatter. Maybe I should. I don't feel that
strongly about a particular format but I would like one that allows
renames easily.

> Well, 'Sealed (Dup p wX)' is isomorphic to 'p wX':
> 
> fromSealedDup :: Sealed (Dup p wX) -> p wX
> fromSealedDup (Sealed (Dup x)) = x
> 
> toSealedDup :: p wX -> Sealed (Dup p wX)
> toSealedDup x = Sealed (Dup x)
> 
> does typecheck, and the two functions are obviously inverses.
> 
> So Dup is the opposite of Sealed in the same sense in that
> pretty-printing is the opposite of parsing.

Yeah, Sealed can undo Dup, but Dup can't undo Sealed (where the
witnesses weren't the same to begin with). I don't feel that strongly
about the exact wording :-)

>>>       writeTentativeInventory (repoCache repo) compr (PatchSet ts (unsafeCoerceP ps))
>>
>> I think this is a new unsafeCoerce - do you know why it's needed?
> 
> Not really. I don't think the new code does anything fundamentally
> different wrt the witnesses. The error message says "‘wX0’ is
> untouchable" and I have only a vague idea what that means.

I had a quick play and I can remove it by deferring the pattern-match on
Dup to a bit later - so make the case Just (ps :> dupr) rather than Just
(ps :> Dup r). It's not very satisfactory and I also don't understand
what exactly is going on.

>>> Minor nitpick: I wouldn't have removed the forall in the type definition
>>> of toRebaseChanges as these often turn out to be needed again in the
>>> future.
>>
>> But you have to admit that the end result is less cluttered now, and
>> thus easier to read.
> 
> I guess the foralls are generically clutter, but I'm fairly used to them
> now. They have the advantage of making the type variable ordering
> explicit for TypeApplications and that defaulting to having them there
> reduces churn.

I agree that these advantages exist. Nevertheless, IME it is usually
(though not always) possible to restructure the code such that explicit
type applications become unnecessary; and similarly with pattern type
signatures. I have done so in many places in Darcs and invariably I find
that the result is simpler and easier to read. So I tend to regard them
as indicators for code that may profit from a bit of refactoring.

That said, explicit foralls are also needed when we delegate the
implementation to a local function that has to have a type signature
because of varying witnesses, e.g. when we traverse RLs or FLs with an
accumulating argument. This happens more often than I would prefer,
which is because with witness types it is not so easy to factor out all
the interesting recursion patterns to higher order functions. Indeed, I
would say that this is the greatest disadvantage of the more precise
typing we get with the witnesses.

Anway, I can agree on keeping existing foralls in type signatures even
when they are no longer strictly needed (except in cases where the type
signature has to be largely re-written anyway).

>>> Also the indentation of the reformatted signature isn't
>>> resistant to changes in the name of the function.
>>
>> I merely followed the convention we use almost everywhere else. I agree
>> that this convention is not ideal for the reason you stated and I am
>> open to a change here. For type signatures that don't fit on a single
>> line you seem to prefer
>>
>> fun
>>   :: (Constraints)
>>   => x
>>   -> y
>>
>> I could adopt that style. Are you using an auto-formatter? I have used
>> hindent for a long time, but it has lots of bugs and often the
>> formatting is not quite what I want so I have to amend the result
>> manually. I have just now installed brittany and it seems to work a lot
>> more reliably and does indeed format type signatures in that way.
> 
> No, I'm not using an auto-formatter. Maybe I should. I don't feel that
> strongly about a particular format but I would like one that allows
> renames easily.

Agreed. I will adopt the above style, especially since it happens to
coincide with what brittany does by default.

I do like auto-formatters, but never use them on complete files, only
selected sections of code; either code I have newly written or
(occasionally) old code with severely messed up indentation. Also, I
always look at the result and only keep it if it is an improvement to
readability; or, if the result is mostly okay, I may keep it and
manually reformat the pieces I don't like.

Stylish-haskell is very well suited for cleaning up import lists, and
perfectly matches our conventions (with the .stylish-haskell.yaml I
added a while ago). I also used hindent for declarations and
expressions, which I have now replaced with brittany for declarations,
expressions, and type signatures.

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

patch 36cfcaf8a7b3d035b0d501c802cd8b4716e42c89
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Tue Apr  6 21:43:23 BST 2021
  * remove need for an unsafeCoerceP
  
  I don't fully understand what's going on but
  https://gitlab.haskell.org/ghc/ghc/-/issues/14539
  gave me the hint to add some type signatures so that
  GHC doesn't need to solve for a type variable in a
  context where an equality constraint is in effect.

Attachments

• patch-preview.txt
• remove-need-for-an-unsafecoercep.dpatch
• unnamed

On 06/04/2021 13:39, Ben Franksen wrote:

> I agree that these advantages exist. Nevertheless, IME it is usually
> (though not always) possible to restructure the code such that explicit
> type applications become unnecessary; and similarly with pattern type
> signatures. I have done so in many places in Darcs and invariably I find
> that the result is simpler and easier to read. So I tend to regard them
> as indicators for code that may profit from a bit of refactoring.

Philosophical aside...

I think I've gone through a bit of a mindset shift in the last few years.

Trying to explicitly constrain a type variable used to require jumping
through hoops so code tended to be more elegant when the types were
inferred "naturally". In some sense Hindley-Milner was the happy path
and anything else was a bit ugly. So I was also generally inclined to
try to avoid specifying types directly.

TypeApplications and to some extent other facilities like
ScopedTypeVariables, PatternSignatures and AllowAmbiguousTypes have now
made it much more elegant to code with a System F view in mind, i.e.
explicitly passing type variables. So now I tend to see it as natural
rather than something to avoid.

Anyway none of this is a big deal, even if I tend to use them a little
bit more than you in practice.

> This happens more often than I would prefer,
> which is because with witness types it is not so easy to factor out all
> the interesting recursion patterns to higher order functions. Indeed, I
> would say that this is the greatest disadvantage of the more precise
> typing we get with the witnesses.

Agreed, the lack of general abstractions is a real downside.

> Stylish-haskell is very well suited for cleaning up import lists, and
> perfectly matches our conventions (with the .stylish-haskell.yaml I
> added a while ago). I also used hindent for declarations and
> expressions, which I have now replaced with brittany for declarations,
> expressions, and type signatures.

Great - looks like you've found a set of tools that fit our current
practice nicely, I shall take a look at adopting them myself. Hopefully
brittany can be used with haskell-language-server.

Jumping back to an old message about the new unsafeCoerceP:

> Not really. I don't think the new code does anything fundamentally
> different wrt the witnesses. The error message says "‘wX0’ is
> untouchable" and I have only a vague idea what that means.

I (sort of) figured this out and have sent a followup patch to this
thread. Unless you have any objections I'll include it when accepting
this bundle.

> I (sort of) figured this out and have sent a followup patch to this
> thread. Unless you have any objections I'll include it when accepting
> this bundle.

I really like your solution, use of pattern type signatures
notwithstanding ;-)

It does conflict with some of my unsent WIP, but the conflict is easily
resolved so not a great problem.

Following up on review.

1 patch for repository http://darcs.net/screened:

patch c9ce734279466a365304e933c4b02651db9424e5
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Apr  6 11:09:10 CEST 2021
  * clarify haddocks for Darcs.Patch.Witnesses.Sealed.Dup

Attachments

• clarify-haddocks-for-darcs_patch_witnesses_sealed_dup.dpatch
• patch-preview.txt
• unnamed