[darcs-users] darcs patch: Begin using RIO (and 39 more)

David Roundy droundy at darcs.net
Tue Sep 9 16:06:29 UTC 2008


On Mon, Sep 08, 2008 at 05:08:20PM -0700, Jason Dagit wrote:
> On Mon, Sep 8, 2008 at 5:05 PM, Jason Dagit <dagit at codersbase.com> 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.

The problem here is that the type of sequenceRepls is wrong--I hadn't
looked carefully at that code previously.  You're making it always
create patches that start at the unrecorded state, but that's not what
it's actually doing (or so we hope).  All we need to do is to
eliminate the Repository argument from sequenceRepls, and the problem
is fixed.

I'm attaching a patch that does this.

> 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.

No, it works fine without the Repository parameter.  (see attached patch)

> 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

No, Sealed (FL C(x)) is our normal solution.  Functions of the type of
foo given above are inherently buggy, so we try to avoid them.

> 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.

This isn't really a good trick, since it just works around a bug
elsewhere.

> Now, moving on to the next unsafeCoerceP that I see (this is from Changes.lhs):

I'll try to get to this one tomorrow.

David


More information about the darcs-users mailing list