[darcs-devel] witnesses changes

Ganesh Sittampalam ganesh at earth.li
Wed Jun 27 06:50:24 UTC 2012


Hi,

OK, so the problem is that there's a mismatch between the expected end
contexts on the two sides of the if statement - in the patch index case,
the end context is the recorded state of the repository wR - as seen in
the signature of annotate itself - and in the other it's some unknown
context wX coming from the unwrapping of 'Sealed patches' earlier on.

Since the old code was also using wX, and patches isn't used inbetween
the 'Sealed patches' match and this point in the code, I would try just
moving the unwrapping further down - i.e. keep patches as Sealed, and
unwrap the Sealed when binding ans_patches from this if statement. That
would require adding a seal to the patch index case somewhere.

It also seems a bit fishy that in the patch index case, patches is being
calculated then ignored, but perhaps the magic of lazy evaluation makes
that ok.

Cheers,

Ganesh

On 27/06/2012 07:18, Aditya wrote:
> Here is the error if I remove unsafeCoerce on repository for the call to
> getPatches:
> 
>         src/Darcs/UI/Commands/Annotate.hs:177:63:
> 
>             Could not deduce (wR ~ wX)
> 
>             from the context (RepoPatch p, ApplyState p ~ Tree)
> 
>               bound by the type signature for
> 
>                          annotate' :: (RepoPatch p, ApplyState p ~ Tree) =>
> 
>                                       [DarcsFlag] -> [String] ->
>         Repository p wR wU wR -> IO ()
> 
>               at src/Darcs/UI/Commands/Annotate.hs:(116,1)-(182,62)
> 
>               `wR' is a rigid type variable bound by
> 
>                    the type signature for
> 
>                      annotate' :: (RepoPatch p, ApplyState p ~ Tree) =>
> 
>                                   [DarcsFlag] -> [String] -> Repository
>         p wR wU wR -> IO ()
> 
>                    at src/Darcs/UI/Commands/Annotate.hs:116:1
> 
>               `wX' is a rigid type variable bound by
> 
>                    a pattern with constructor
> 
>                      Sealed :: forall (a :: * -> *) wX. a wX -> Sealed a,
> 
>                    in a pattern binding in
> 
>                         'do' block
> 
>                    at src/Darcs/UI/Commands/Annotate.hs:146:4
> 
>             Expected type: Repository p wX wU wX
> 
>               Actual type: Repository p wR wU wR
> 
>             In the first argument of `getPatches', namely `repository'
> 
>             In the second argument of `fmap', namely
> 
>               `getPatches repository path patch_id'
> 
> 
> 
> On Wed, Jun 27, 2012 at 11:36 AM, Ganesh Sittampalam <ganesh at earth.li
> <mailto:ganesh at earth.li>> wrote:
> 
>     What error do you get without the unsafeCoerce on repository?
> 
>     On 27/06/2012 06:58, Aditya wrote:
>     > Hi Heffalump,
>     >  Thanks for the review! I will encapsulate the functions that use
>     > unsafeCoerce.
>     >  I need you help with unsafeCoerce in a different place, however.
>     > Please look at
>     > Darcs.UI.Commands.Annotate.
>     http://darcsden.com/bsrkaditya/darcs-patch-index-rebase-2012-06-17/browse/src/Darcs/UI/Commands/Annotate.hs
>     > I have ported patch-index annotate into the module.
>     > (patch:
>     http://darcsden.com/bsrkaditya/darcs-patch-index-rebase-2012-06-17/patch/20120627050014-ae621)
>     > I am using unsafeCoerce on repository, and I want to avoid using it.
>     > (This was not necessary before combining the two annotates)
>     >
>     > Thanks,
>     > Aditya.
>     >
>     > On Tue, Jun 26, 2012 at 11:47 PM, Ganesh Sittampalam
>     <ganesh at earth.li <mailto:ganesh at earth.li>
>     > <mailto:ganesh at earth.li <mailto:ganesh at earth.li>>> wrote:
>     >
>     >     Hi Aditya,
>     >
>     >     I had a look at the witnesses changes you mentioned on IRC
>     after our
>     >     meeting (in your branch at
>     >     darcsden.com/bsrkaditya/darcs-patch-index-rebase-2012-06-17/
>     <http://darcsden.com/bsrkaditya/darcs-patch-index-rebase-2012-06-17/>
>     >    
>     <http://darcsden.com/bsrkaditya/darcs-patch-index-rebase-2012-06-17/>).
>     >
>     >     It's good to see at least some of the Sealed2 stuff gone - I
>     lost track
>     >     of whether it's all gone yet, but the end goal should
>     definitely be to
>     >     avoid any new uses of [Sealed2 ...], although I think that the
>     changes
>     >     code might already have a bit of that before the patch index
>     was added
>     >     so obviously it's fine for that to remain for now.
>     >
>     >     I forgot to emphasise that use of unsafeCoerce on witnesses in
>     darcs is
>     >     supposed to be tightly constrained, to reduce the risk of
>     using it in a
>     >     way that's actually dangerous. Having getElems and dropFLFL use it
>     >     directly is therefore not a good idea, because those functions
>     are quite
>     >     general could be used in a different context to patch index
>     filtering.
>     >
>     >     In the meeting I suggested using filterFLFL to achieve this,
>     with an
>     >     unsafe function being passed in that works only on the basis
>     of whether
>     >     something is excluded from the patch index or not - but
>     looking at the
>     >     patch index code a bit more I see that may be difficult as it
>     relies on
>     >     a positional analysis which would be tricky to do as a filter.
>     >
>     >     Given that, it's probably best to encapsulate the unsafeCoerce
>     within
>     >     one or two functions that read the repository and the patch index
>     >     together (two might be needed given what you said about
>     needing an FL in
>     >     one case and a PatchSet in another). Overall the goal should
>     be that the
>     >     unsafety can't be easily used by outside callers to do something
>     >     completely different.
>     >
>     >     In the long run I would like to have as simple an abstraction as
>     >     possible for doing this (hence my suggestion of viewing this as a
>     >     filter), but overall safety is the primary goal and simplicity is
>     >     secondary.
>     >
>     >     BTW it's better to use unsafeCoerceP than unsafeCoerce; it has
>     a type
>     >     that means it's less likely to be used by accident on the
>     wrong kind of
>     >     thing.
>     >
>     >     Feel free to ask more questions by email or arrange another
>     morning IRC
>     >     meeting.
>     >
>     >     Cheers,
>     >
>     >     Ganesh
>     >
>     >
>     >
>     >
>     > --
>     > BSRK Aditya
> 
> 
> 
> 
> 
> -- 
> BSRK Aditya




More information about the darcs-devel mailing list