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

[Jason Dagit]

On Thu, Sep 11, 2008 at 2:36 PM, David Roundy <daveroundy at gmail.com> wrote:

> No, the type witness corresponding to the ending state isn't
> controlled by _foo, it's controlled by the caller of _foo, so the only
> safe version of _foo would be one that always returns Nothing.

With _foo the call site determines how the phantom type will unify in
the type system.  I agree with that, but this is different than what I
was talking about.  I meant that given a pure implementation of _foo
:: String -> Maybe (FL a C(x y)), that we should expect that _foo a =
_foo b, when a = b (because _foo is a pure function its output should
only depend on its parameters).  So my idea was that if only
existential types (and ()) can be used as witnesses it might make
things like the above a bit safer.  But, we can't restrict like this,
save for seals, so it's probably not worth continuing to debate it
other than it lead me to a better way to discuss sealed types.

> Right, we need to be disciplined about the creation of patches.  Just
> as exporting constructors is unsafe, so is exporting functions that
> act as constructors.  These are known holes that need to be plugged
> eventually.  They don't seem all that urgent, but perhaps the best
> scheme would be to fix these holes as we come across them.

Well, it seems that due to exporting functions like (=\/=), which use
unsafeCoerce# to make two phantoms equal in the type system, that any
function which has a phantom appearing on the right side of the
function arrow but not on the left leads to reimplementing
unsafeCoerce#.  The only exception to how I just stated that is when
everything except the phantom that we want to exploit is sealed, but
that's actually just due to the type signature of (=\/=) and (=/\=).
If we had exported unsafeCoerce# as a different type signature then
the limitation I mention would be different.

The above was not an obvious observation for me.  I had to work with
this stuff quite a bit to see it.  It also seems like something we'll
need to educate people on when they work with type witness code.  I
sure wish I had understood it better in the beginning.

> The solution is to return Sealed patches in ordinary functions (e.g.
> one that creates a new addfile patch).  smart_diff is trickier.
>
> We should be able to return (in almost all cases, smart_diff is
> tricky) Sealed patches, and we won't need to use unsafeCoerceP, even
> judiciously.

How do we express what makes smart_diff different?  Why is it such a
special case?  How can we be sure that there aren't other such special
cases hiding about?

> No, smart_diff is never used in high-level code, so far as I'm aware.
> It's a low-level function that isn't used in high-level code, so it's
> less urgent than unsafe functions in high-level code (which we can
> approximately define to be code in Darcs.Commands.*).

Actually, smart_diff is used here in Replace and it was already
imported there to begin with.  smart_diff is called by
get_force_replace which is in turn called by repl which is called by
sequenceRepls, all of which are defined in the same where-clause.

A quick grep shows that it's also used in:
Check, Remove and WhatsNew

Maybe those are bugs that need to be addressed before my refactoring.
It's issues like this that I seek to uncover and decide how to solve
before I write patches that have to be amended a dozen times.

> Your implementation was wrong, since it relied on repl returning a
> sequence of patches with no effect.  You were able to do this because
> we had exported unsafe functions, but that doesn't make it right.

The exported unsafe functions being smart_diff and (=\/=)?  Please
clarify.  If I'm adding to the pool of unsafe exported functions I'd
like to know about that.

> I'd really like to have smaller patches that we could discuss in the
> context of actually applying them to darcs.  It will allow the
> discussion (and my review) to be both more concrete and more
> productive.

I compiled a list in a previous email asking very detailed and
specific questions (with the code in question).  I don't understand
why it's difficult for you to apply my patch bundle in a clean copy of
the repository and look at the referenced spots if you need more
detail than was contained in that email.

This set of changes that is in question also doesn't appear to change
the research value of what I've been working on.  It would seem that I
have local changes that work as a proof of concept, even if the
details of getting them into http://darcs.net haven't been worked out.
 As such, I don't see it as being a show stopper for me to finish my
degree.  You don't have the time to review and I don't have the time
right now to amend.  I propose we both table it until we have more
time.

Jason