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

[Jason Dagit]

On Tue, Sep 9, 2008 at 9:06 AM, David Roundy <droundy at darcs.net> wrote:

>> 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 very confused by this.  We take the result of sequenceRepls and
hand it off to add_to_pending and applyToWorking which both take a
sequence starting from the unrecorded state.  The new type of
sequenceRepls from your patch returns an arbitrary starting type,
which happens to unify with the unrecorded state.  But, we could
equally make it unify with some other function from
Repository.Internal that takes, say, a recorded state.  This seems
like a violation of type safety to me.

I would agree that the recursive call shouldn't return something from
the unrecorded state, I tried to say that much in my description.
But, your version doesn't return something that is ultimately
constrained to starting in the unrecorded state so it seems dangerous
to me.  My understanding is that we apply the results to the
unrecorded state very intentionally.  Perhaps the unrecorded state is
not useful here?  And if so, why?

How about:
sequenceRepls :: Repository p C(r u t) -> String -> Slurpy -> Slurpy
-> [SubPath] -> IO (Sealed (FL Prim C(u)))
sequenceRepls _ = sequenceRepls_
  where
  sequenceRepls_ :: String -> Slurpy -> Slurpy -> [SubPath] -> IO
(Sealed (FL Prim C(u)))
  <insert your definition of sequenceRepls>

This version would mean than whoever calls sequenceRepls gets
something from the unrecorded state while fixing the problem we both
saw in the recursive call.

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

I'm sorry, but comments like this are not helpful to me.  What bug are
you thinking of?

>> Now, moving on to the next unsafeCoerceP that I see (this is from Changes.lhs):
>
> I'll try to get to this one tomorrow.

Okay, I won't update anything until then.

thanks,
Jason