On Mon, Sep 8, 2008 at 5:05 PM, Jason Dagit <[EMAIL PROTECTED]> wrote:
> David,
>
> This is an updated version of the patches I have. I still need
> feedback on them. I removed a bunch of unsafeCoercePs and fixed the
> compile problems on 6.6.
>
> I'll send another email describing the problems in the remaining uses
> of unsafeCoerceP. You can either give your feedback here or there,
> but either way I need you to comment on each case so I can continue
> making refinements.
And here is that email:
Please give Feedback on the following:
------------------------------------
The sample code you sent doesn't unify and I'll explain why.
In the refactor for RIO you specifically asked that we avoid tracking
modifications to anything except the tentative recorded state; because
of that we can't thread changes to recorded or unrecorded:
sequenceRepls :: Repository p C(r u t) -> String -> Slurpy
-> Slurpy -> [SubPath] -> IO (Sealed (FL Prim C(u)))
sequenceRepls _ _ _ _ [] = return $ seal NilFL
sequenceRepls r toks cur work (s:ss) = do Sealed x <- repl
toks cur work s
Sealed xs <-
sequenceRepls r toks cur work ss
return $ seal (x+>+xs)
The problem here is that each replace patch we construct in the
sequence basically comes from a different unrecorded state (since they
are a sequence each new one comes from the final state of the previous
one). With the two seals, we get that the sealed phantom on x won't
unify with the unrecorded state.
This is an instance of a familiar problem: We are instantiating the
phantom types as we create patches. In my opinion, this code to
sequence and synthesize the replacement patches should probably not be
here but somewhere else such as Repository.Internal but I'm trying not
to be disruptive in my refactoring and other commands, such as Add, do
similar patch creations.
On the other hand, if I remove the Repository parameter from
sequenceRepls, then we can't return something that has the right
context at all, so having it in the parameter list seems right, but
making it RIO won't help either.
I thought that 'unsafeCoerceP NilFL' was our accepted solution for
functions of this form:
foo :: Bar -> FL a C(x y)
The problem being is that without an unsafeCoerceP that function
cannot return NilFL (at least I haven't seen a way). For example, you
can add this to the where block and it will give the following error:
_foo :: String -> FL a C(x y)
_foo _ = NilFL
src/Darcs/Commands/Replace.lhs:155:19:
Couldn't match expected type `x' against inferred type `y'
`x' is a rigid type variable bound by
the type signature for `_foo'
at src/Darcs/Commands/Replace.lhs:154:33
`y' is a rigid type variable bound by
the type signature for `_foo'
at src/Darcs/Commands/Replace.lhs:154:35
Expected type: FL a x y
Inferred type: FL a y y
In the expression: NilFL
In the definition of `_foo': _foo _ = NilFL
The only other trick that comes to mind is that it could be:
_foo :: String -> Maybe (FL a C(x y))
_foo _ = Nothing
That is, we only return non-empty FLs. I've made this change and it
does compile, so I guess I'll now add this to my bag of FL tricks.
Now, moving on to the next unsafeCoerceP that I see (this is from Changes.lhs):
where xx :: Prim C(x y) -> EqCheck C(x y)
xx x = case list_touched_files x of
[z] | z `elem` fs -> NotEq
_ -> unsafeCoerceP IsEq
Is there a way to be rid of this one? The ways I can see to get rid
of it seem to violate the purpose of doing the EqCheck. This xx is
the predicate given to filterFL, and we use filterFL because the
filtered results are fed to showFriendly (and I'm pretty sure
showFriendly assumes it may need to commute the patches, as it takes a
patchy).
Moving on to the next one, still in Changes.lhs. I added a comment to
this one hoping I would get some feedback. It appears to happen
because we have this function:
get_changes_info :: RepoPatch p => [DarcsFlag] -> [FilePath] -> PatchSet p C(x)
-> ([Sealed2 (PatchInfoAnd p)], [FilePath], Doc)
And it gets used like this:
+ case mF (:<:NilRL) (headRL (slightly_optimize_patchset r)) of
+ FlippedSeal ps -> do
+ when (not $ nullRL r || unsealFlipped nullRL (headRL r)) $
+ putDocLnWith simplePrinters $ changelog opts' NilRL $
+ -- This unsafeCoerceP seems to be neccessary due to
+ -- inventory splits at tags. Without the context
+ -- would be too long (and redundant) for practical
+ -- usage.
+ get_changes_info opts' [] (unsafeCoerceP ps)
+ where opts' = if HumanReadable `elem` opts || XMLOutput `elem` opts
+ then opts
+ else MachineReadable : opts
+ mF :: (FORALL(y) a C(y x) -> b C(y x)) -> FlippedSeal a C(x)
-> FlippedSeal b C(x)
+ mF f (FlippedSeal a) = flipSeal $ f a
The problem here is that we don't give a PatchSet to get_changes_info,
we give it a patch sequence (specifically the starting context is not
()). It seems as though we are trying to treat all the patches since
the tag as being all the patches in the repository, as if starting
from (). The easiest fix here is to get rid of what I interpreted as
an optimization and just pass the result of slightly_optimize_patchset
directly to get_changes_info. That seemed like a pretty big change
for me to make without your feedback.
Next we have to Add.lhs, in particular addp is problematic, but I
think I may have that one figured out finally.
After that is MarkConflicts.lhs:
-- These unsafeCoercePs are safe because above, if there were
-- unrecorded changes we reverted them by applying their inverse
-- to the working copy. Thus, r = u.
do add_to_pending repository (unsafeCoerceP res :: FL Prim C(u x))
applyToWorking repository opts (unsafeCoerceP res :: FL Prim
C(u x)) `catch` \e ->
bug ("Problem marking conflicts in mark-conflicts!" ++ show e)
Again we have an issue with which state is tracked in RIO. Just a few
lines up from that we check that pending is NilFL and if it's not
apply the inverse pending, but doing that in RIO won't help. What
should we do about that other than unsafeCoerceP or extending RIO?
I moved on to Mv.lhs and I was able to get rid of one more
unsafeCoerceP by using the Maybe (FL a C(x y)) trick above, but I
don't see how to eliminate this one:
mapMFL :: Monad m => (FORALL(u v) a -> m (Maybe (b C(u v)))) -> [a] ->
m (FL b C(x y))
mapMFL _ [] = return $ unsafeCoerceP NilFL
mapMFL f (a:as) = do b <- f a
bs <- mapMFL f as
if isJust b
then return (fromJust b :>: bs)
else return bs
I spent an entire morning trying to get rid of the unsafeCoerceP in
the above function but so far I have failed to remove it.
The next one I see is in Optimize.lhs:
choose_order :: forall p C(z). RepoPatch p => PatchSet p C(z) -> PatchSet p C(z)
choose_order ps = case last_tag of
Sealed NilRL -> ps
Sealed (lt:<:_) ->
co lt $ mapSeal slightly_optimize_patchset $ get_patches_in_tag
(info lt) ps
where last_tag = dropWhileRL (not . is_tag . info) (concatRL ps)
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
co :: (PatchInfoAnd p) C(a b) -> SealedPatchSet p -> PatchSet p C(z)
co lt' (Sealed ((t:<:NilRL):<:pps)) =
case get_patches_beyond_tag lt' ps of
-- This unsafeCoerceP is safe because t = lt'. We would use
-- (=\/=) to prove it, except that get_patches_in_tag must first
-- be fixed to return a meaniful context.
p :<: NilRL -> (p+<+((unsafeCoerceP t):<:NilRL)) :<: pps
_ -> impossible
co _ _ = impossible
My comment is talking about get_patches_in_tag which is passed a
PatchInfo. I looked at refactoring get_patches_in_tag but it was
quite daunting. It's a 40 line Haskell function that seems to be
quite related to get_patches_beyond_tag. I think, that we should
probably have something like,
get_patches_around_tag :: RepoPatch p => PatchInfo -> PatchSet p C(z)
-> (RL (RL (PatchInfoAnd p)) :> (PatchInfoAnd p) :> RL (RL
(PatchInfoAnd p))) C(() z)
Actually, I think that middle element needs to have the same context
as the PatchInfo, so:
get_patches_around_tag :: RepoPatch p => PatchInfoAnd p C(x y) ->
PatchSet p C(z) -> (RL (RL (PatchInfoAnd p)) C(() x), (PatchInfoAnd
p) C(x y), RL (RL (PatchInfoAnd p)) C(y z))
Actually, since the first param should be a tag, it could be
PatchInfoAnd p C(x x), but I think that would require a proof every
time we pass something to the above function which may get tiresome
without adding any real safety. This is another possibility:
get_patches_around_tag :: RepoPatch p => PatchInfoAnd p C(x y) ->
PatchSet p C(z) -> (PatchInfoAnd p C(x x), RL (RL (PatchInfoAnd p)) :>
RL (RL (PatchInfoAnd p))) C(() z)
But, ultimately, I've didn't do this yet because get_patches_in_tag
didn't explain itself enough to me and I just didn't feel confident
with which types it should have. Does it really return all the
patches that the tag depends on or not?
How is the unsafeCoerceP in Put.lhs avoidable? patchset == patchset2,
but due to the unsealing their ending contexts are considered
different by the type system. The only way to fix it is by updating
this function:
get_one_patchset :: RepoPatch p => Repository p C(r u t) ->
[DarcsFlag] -> IO (SealedPatchSet p)
get_one_patchset repository fs =
case nonrange_matcher fs of
Just m -> do ps <- read_repo repository
if nonrange_matcher_is_tag fs
then return $ get_matching_tag m ps
else return $ match_a_patchset m ps
Nothing -> (seal . scan_context) `liftM` mmapFilePS
(toFilePath $ context_f fs)
where context_f [] = bug "Couldn't match_nonrange_patchset"
context_f (Context f:_) = f
context_f (_:xs) = context_f xs
That function seems to call several Sealed returning functions. What
is the solution here? Obviously the unsafeCoerceP is currently quite
safe and is the easiest solution.
I was able to remove the unsafeCoerceP from Remove.lhs.
Please advise.
Thanks,
Jason
_______________________________________________
darcs-users mailing list
[email protected]
http://lists.osuosl.org/mailman/listinfo/darcs-users
