[darcs-devel] darcs patch: add context to HashedRepo.copy_repo (and 2 more)

Tue Mar 11 21:27:18 UTC 2008

On Tue, Mar 11, 2008 at 1:07 PM, David Roundy <droundy at darcs.net> wrote:

> (boring talk going on right now... trying to type quietly...)
>
> On Tue, Mar 11, 2008 at 12:50:35PM -0700, Jason Dagit wrote:
> > Thanks for reviewing this David.  I've tried to explain it below.
> >
> > On Tue, Mar 11, 2008 at 7:43 AM, David Roundy <droundy at darcs.net> wrote:
> >
> > > I'm afraid I don't really understand what you're doing with the
> Context
> > > type.  As you hint, it does look scary.  I'm in a break in a
> conference
> > > here (a short break), so I haven't much time to concentrate on this.
> > >  Could
> > > you write up a short explanation of why you added Context and what
> it's
> > > supposed to do? In particular, I'd be interested in hearing what
> problem
> > > it's supposed to solve.  At a quick glance, it looks like a wrapper
> around
> > > unsafeCoerceP, and if that's the case, then it definitely should be
> called
> > > something like UnsafeContext, and we should strive not to use it.  But
> > > maybe it's not that bad.  I was trying to concentrate on the Physics
> talk
> > > I
> > > was listening to while reading your patch.  The next session will be
> even
> > > more interesting...
> >
> >
> > The basic problem with fixing read_repo_private was that between parse,
> > createHashed, read_patches and rp no one knew what context of patches
> were
> > being created and yet they are supposed to go into a sequence.  I
> realized
> > at some point that as humans we know the patches form a sequence because
> > right before we try to parse them we read the sequence of PatchInfos
> from
> > the inventory.  One option would have been to change PatchInfo to have a
> > context, the context of the patch that it is associated with, but this
> > solution would not be incremental and I was afraid it is beyond the
> scope of
> > what I can accomplish in the near future.
> >
> > So my next idea was to create a wrapper type that can be used to
> associate a
> > context with a non-patch data type.  This allows me to, temporarily
> > (basically where convenient), give those PatchInfos the same context as
> the
> > patches they are related to.  Or, at least, that's my idea.  Once I had
> this
> > context associated with the PatchInfos I could pass it along to the
> > functions mentioned above, parse, createHashed, read_patches and rp so
> that
> > the type checker can tell that the patches should form a sequence.
> >
> > I tried quite a few other things before I arrived at the Context
> solution.
> > I convinced myself that using sealed types would not work without
> several
> > unsafeCoercePs.  I think what I've done with the Context type is a step
> up
> > in terms of safety from using the unsafeCoercePs directly.  But since I
> only
> > use the Context type in places that didn't even have context previously,
> I
> > don't know that it's fair to call it a wrapper for unsafeCoerceP.  It
> may be
> > fair to say it's coercing patch infos.
>
> The problem is that you expose a constructor for Context, and you unseal
> the output from createHashed and friends (I believe).  Which is
> effectively
> equivalent to simply unsealing the output of createHashed and friends.

If I just unseal createHashed and friends then I end up with existential
types which are not equal to any other type.  To then put the freshly
created patches into a sequence I would have to unsafeCoerceP them.  This
use of unsafeCoerceP seemed unconstrained and susceptible to misuse.  The
only way I could think of to eliminate this was to pass more context
information to those functions.  As I said before, it seemed best to do this
incrementally instead of completely changing the PatchInfo type.  For the
Context type to be useful at all I need a way to assign context to objects
and conversely a way to remove it again.  Even if I hide the constructor of
Context I still have to expose:
unsafeContext :: a -> Context a C(x y)
unsafeUncontext :: Context a C(x y) -> a

Each corresponds to exactly to the constructor of Context.  These can still
be used together in the same module to arbitrarily change the context of
something.  That's why I didn't bother to create those two functions
initially (I have now and I'm recording the patch as I type this).  I wasn't
sure we gained anything.  I guess making people use the unsafe versions
forces us to think about why and where we are adding/removing context.  I
know this isn't as safe as our previous approaches, but without completely
rewriting all the code that uses PatchInfo I'm not sure how else to approach
this problem.  I don't think the Monadish approach applies here at all since
we are doing normal IO in createHashed/parse and not patch operations.  And
as I understand Monadish, the point is to make monadic patch operations
safe.

>  We
> could do that, but it would mean that those are unsafe functions.  They're
> already marginally dangerous, so maybe that isn't the end of the world.
> I'm not clear that Context gains us any real safety over merely unsealing
> these functions.  I guess the only safety we'd have would be gained by
> auditing the use of the Context constructor.

Someone could lie and create an invalid context using Context, but I think
it's harder now.  And I think it's easier to create a sequence, associate a
Context with the elements and then process that sequence correctly than it
is to use unsafeCoerceP directly inside of read_repo_private.

Something I've learned from working pragmatically with our type witnesses is
that it's unrealistic to think we'll ever make it impossible for people to
introduce patch manipulation bugs.  I think the best we can get is that it's
harder to do things dangerously than it is to do it safely.

I agree that it is still important to audit the code because someone could
always make use of unsafeCoerc#, unsafeCorceP, unsafeMap_l2f, etc.  Given
that sometimes it is very hard to express what you mean in the type system I
think these functions have a purpose and we just have to be cautious about
using them.  The same will now apply to unsafeContext and unsafeUncontext
(if they get accepted).

>
> > I think we should try to avoid it's use when we have a 'native' solution
> > (like having contexts on patches), but I think even if we decide it
> should
> > be UnsafeContext that it's useful as an incremental trick.
> >
> > Does my explanation help?  Do you understand what I was trying to do
> now?
> > Do you have ideas how to improve it?
>
> Yes, that makes sense.  I think that either the constructor shouldn't be
> exported, or it should be renamed to be "unsafe".  If we don't export it,
> we'd create a function for adding contexts, which itself would be labelled
> unsafe.  The problem is that Context (the constructor) + createHashed is
> actually equivalent in unsafety to unsafeCoerce#, which is pretty darn
> unsafe (i.e. we don't just lose type witness safety, we could introduce
> segfaults).

I know that with unsafeCoerce#  you could do evil things like lie to the
compiler and claim an Int is a String and your program would segfault (if
you're lucky).  But, in this case, Context just manipulates phantom types
and similarly for createHashed wrt Sealed (or not Sealed).  My understanding
of phantom types is that at a low level it's okay to coerce them (eg, you
won't segfault directly from that, but you may confuse the compiler and
still get results you didn't want).  It's unclear to me how we would cause a
segfault in this case.  I agree type safety of our witness types is
important, but it's not clear to me how we could construct an example using
Context and createHashed that would cause darcs to segfault.  The only
possibility I'm thinking of is that we could write a version of
read_repo_private that used Context to create a PatchInfo sequence that
isn't really sequential and darcs could die when applying that sequence.  I
believe this is equivalent to tampering with the inventory, but I don't
think my approach to fixing read_repo_private improves or degrades our
handling of tampered inventories.

>
>
> The boring talk's over now, and I still haven't carefully read your patch.
> But if you resend a version with an unsafe label (and be sure to think
> about where you put the label, and don't just trust me), I'll apply it.

My patch to do that was sent in a different email.  Please let me know what
you think.  I've put the unsafe labels where I think they belong.

Thanks,
Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.osuosl.org/pipermail/darcs-devel/attachments/20080311/aae3c73c/attachment.htm 