[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