[darcs-users] darcs patch: Make Darcs.Repository.Internal compile w... (and 1 more)

David Roundy droundy at darcs.net
Sat Aug 9 00:45:26 UTC 2008


On Thu, Aug 07, 2008 at 08:01:32PM -0700, Jason Dagit wrote:
> David,
> 
> I have more type witnesses coming up, but I'm finally done screwing up and
> fixing Darcs.Repository.Internal so I thought I'd send it your way for a
> review :)
> 
> Jason
> 
> Thu Aug  7 18:53:43 PDT 2008  Jason Dagit <dagit at codersbase.com>
>   * Make Darcs.Repository.Internal compile with type witnesses.
> 
> Thu Aug  7 19:30:25 PDT 2008  Jason Dagit <dagit at codersbase.com>
>   * fixed a bug in identity_commutes property
>   In the right identity check the patch order should have gone from
>   (identity :> p) to (p2 :> i2).  I added a rigid type context too
>   so that ghc 6.8 and newer would type the definition.

First off, I'd rather use fmap rather than liftM, simply because it
requires one less import.  It's not a big deal, but when writing new
code that's my preferred approach.

> hunk ./src/Darcs/Patch/Ordered.lhs 244
> +dropWhileFL :: (FORALL(x y) a C(x y) -> Bool) -> FL a C(r v) -> FlippedSeal (FL a) C(v)
> +dropWhileFL _ NilFL       = flipSeal NilFL
> +dropWhileFL p xs@(x:>:xs')
> +              | p x       = dropWhileFL p xs'
> +              | otherwise = flipSeal xs
> +
> +dropWhileRL :: (FORALL(x y) a C(x y) -> Bool) -> RL a C(r v) -> Sealed (RL a C(r))
> +dropWhileRL _ NilRL = seal NilRL
> +dropWhileRL p xs@(x:<:xs')
> +              | p x       = dropWhileRL p xs'
> +              | otherwise = seal xs

I wonder about these.  It seems like they're functions that we're
better off not using.  I see that dropWhileRL is never used, and
dropWhileFL is used precisely once.  Maybe we're better off not
defining a general function? The problem is that it's a lossy one, so
it seems like one that we'd not use in (most?) good code.

> +consFLSealed :: a C(x y) -> Sealed (FL a C(y)) -> Sealed (FL a C(x))
> +consFLSealed a (Sealed as) = seal $ a :>: as
> +
> +consRLSealed :: a C(y z) -> FlippedSeal (RL a) C(y) -> FlippedSeal (RL a) C(z)
> +consRLSealed a (FlippedSeal as) = flipSeal $ a :<: as

These two don't seem to be used at all... and I suspect that reducing
the number of functions we define is more beneficial that providing
every possibly-useful helper.

> hunk ./src/Darcs/Repository/Internal.lhs 403
> -handle_pend_for_add :: (RepoPatch p, Effect q) => Repository p -> q -> IO ()
> +handle_pend_for_add :: forall p q C(r u t x y). (RepoPatch p, Effect q) 
> +                    => Repository p C(r u t) -> q C(x y) -> IO ()
> hunk ./src/Darcs/Repository/Internal.lhs 412
> -       writePatch pn $ (fromPrims newpend :: Patch)
> -    where rmpend :: FL Prim C(x y) -> FL Prim C(x z) -> Sealed (FL Prim) C(y)
> +       writePatch pn $ fromPrims_ newpend
> +    where rmpend :: FL Prim C(a b) -> FL Prim C(a c) -> Sealed (FL Prim C(b))
> hunk ./src/Darcs/Repository/Internal.lhs 419
> +          fromPrims_ :: FL Prim C(a b) -> Patch C(a b)
> +          fromPrims_ = fromPrims
> hunk ./src/Darcs/Repository/Internal.lhs 439
> -tentativelyMergePatches :: RepoPatch p => Repository p -> String -> [DarcsFlag]
> -                        -> FL (PatchInfoAnd p) -> FL (PatchInfoAnd p) -> IO (FL Prim)
> +
> +tentativelyMergePatches :: RepoPatch p
> +                        => Repository p C(r u t) -> String -> [DarcsFlag]
> +                        -> FL (PatchInfoAnd p) C(u r) -> FL (PatchInfoAnd p) C(u y)
> +                        -> IO (Sealed (FL Prim C(u)))
> hunk ./src/Darcs/Repository/Internal.lhs 446
> -considerMergeToWorking :: RepoPatch p => Repository p -> String -> [DarcsFlag]
> -                       -> FL (PatchInfoAnd p) -> FL (PatchInfoAnd p) -> IO (FL Prim)
> +considerMergeToWorking :: RepoPatch p
> +                       => Repository p C(r u t) -> String -> [DarcsFlag]
> +                       -> FL (PatchInfoAnd p) C(u r) -> FL (PatchInfoAnd p) C(u y)
> +                       -> IO (Sealed (FL Prim C(u)))
> hunk ./src/Darcs/Repository/Internal.lhs 454
> -tentativelyMergePatches_ :: RepoPatch p => MakeChanges
> -                         -> Repository p -> String -> [DarcsFlag]
> -                         -> FL (PatchInfoAnd p) -> FL (PatchInfoAnd p) -> IO (FL Prim)
> +tentativelyMergePatches_ :: forall p C(r u t y). RepoPatch p
> +                         => MakeChanges
> +                         -> Repository p C(r u t) -> String -> [DarcsFlag]
> +                         -> FL (PatchInfoAnd p) C(u r) -> FL (PatchInfoAnd p) C(u y)
> +                         -> IO (Sealed (FL Prim C(u)))
> hunk ./src/Darcs/Repository/Internal.lhs 462
> -         pc = case merge (progressFL "Merging them" them :\/: progressFL "Merging us" us) of
> -              _ :/\: x -> x
> +     Sealed pc <- case merge (progressFL "Merging them" them :\/: progressFL "Merging us" us) of
> +                  _ :/\: x -> return $ seal x

We could cut a line and a case statement here by writing

_ :/\: x <- return (merge (progressFL "Merging them" them :\/: progressFL "Merging us" us))

oh, but that runs into the ghc bug that Simon PJ doesn't consider a
bug, right?  :(


> -tentativelyAddPatch :: RepoPatch p => Repository p -> [DarcsFlag] -> PatchInfoAnd p -> IO ()
> +tentativelyAddPatch :: RepoPatch p
> +                    => Repository p C(r u t) -> [DarcsFlag] -> PatchInfoAnd p C(x y) -> IO ()

This type looks wrong to me.  How can we add a patch which has an
unknown relationship to our repository?

> hunk ./src/Darcs/Repository/Internal.lhs 561
> -applyToTentativePristine :: (Effect q, Patchy q) => Repository p -> q -> IO ()
> +applyToTentativePristine :: (Effect q, Patchy q) => Repository p C(r u t) -> q C(x y) -> IO ()

Similarly here...

> hunk ./src/Darcs/Repository/Internal.lhs 607
> -tentativelyRemovePatches :: RepoPatch p => Repository p -> [DarcsFlag]
> -                         -> FL (Named p) -> IO ()
> +tentativelyRemovePatches :: RepoPatch p => Repository p C(r u t) -> [DarcsFlag]
> +                         -> FL (Named p) C(x t) -> IO ()

Note that in contrast, this looks okay.

Mostly it looks good, and most of my comments were the sorts of code
cleanup thing that I wouldn't insist on.  But I'd really like to see
whether we could extend the type checking to these functions
(tentativelyAddPatch, applyToWorking, applyToTentativePristine and
others), and to have a more careful search for similar issues.  Any
time we've got a function in Repository or Internal that accepts a
Repository and some sort of patch type, one of the contexts of the
patch had better match one of the contexts in the repository, or we'd
better have a very good reason (indicated in a comment) why not.

As it is, these functions only typecheck because HashedRepo and
DarcsRepo are themselves do not export a safe API.  Which is all
right, but Repository really needs to export a safe API if we're going
to make any significant claims about the safety gained by compiling
the entire code with type witnesses enabled.

However, since this isn't a real regression in type safety, perhaps
you can write a new patch (if you can fix this) rather than amending
this one.

David


More information about the darcs-users mailing list