This is an incremental improvement for the
fromPrim/fromPrims default methods, that I think
removes their inherent unsafety. The idea is
simply to add a DefaultSignatures-based constraint
PatchId p ~ ().

Incidentally I was surprised to discover that we
were getting away without type instance PatchId for
both V1 and V2 patches, even though fromPrim
mentions them in its signature.

I'd still like to do more to refactor the FromPrim class
but this makes it less urgent.

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

patch 6d2cf4a3e07e4da98221ffc47be502d00d2388b0
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Wed Aug 28 12:39:36 BST 2019
  * restrict the defaults for fromPrim/fromPrims
  
  Now they can only be used when the patch type identifier
  is (), so there's no risk of accidentally instantiating
  a real patch identifier type with a dummy value.

Attachments

• patch-preview.txt
• restrict-the-defaults-for-fromprim_fromprims.dpatch
• unnamed

> This is an incremental improvement for the
> fromPrim/fromPrims default methods, that I think
> removes their inherent unsafety. The idea is
> simply to add a DefaultSignatures-based constraint
> PatchId p ~ ().

Good idea.

> Incidentally I was surprised to discover that we
> were getting away without type instance PatchId for
> both V1 and V2 patches, even though fromPrim
> mentions them in its signature.

Yes, quite strange. I was wondering about that, but didn't investigate.
I hope this is not some sort of defaulting.

>> Incidentally I was surprised to discover that we
>> were getting away without type instance PatchId for
>> both V1 and V2 patches, even though fromPrim
>> mentions them in its signature.
> 
> Yes, quite strange. I was wondering about that, but didn't investigate.
> I hope this is not some sort of defaulting.

I think it's because the type checker is happy to leave type family
applications unreduced if it doesn't have a rule for them. For something
like 'PatchId a' this makes sense as it doesn't know what 'a' is yet.
For things like 'PatchId (RepoPatchV1 prim)' it's a bit weird and often
it leaks into error messages that say "Can't unify PatchId (RepoPatchV1
prim) with Int" when really your mistake was just to not define the type
instance. In this case the argument is being ignored so probably it's
just getting unified with a free type variable which always succeeds.

It's a temporary measure as we probably want to remove these defaults
at some point, but okay for now.