On Thu, Sep 11, 2008 at 6:48 PM, Jason Dagit <[EMAIL PROTECTED]> wrote: >> 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.
Right, this feature of =\/= and =/\= is why Sealed return types are okay with one free witness. > 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. Indeed, you're right. >> 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? Mostly, smart_diff seems tricky to me because it does something that *should* be verified by type witnesses, but isn't. But probably the solution is just to seal its output, which is a pretty standard solution. >> 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. That's true, and I think get_force_replace really should be moved into a lower-level module, probably Darcs.Patch.Apply It ultimately ought to have the type: force_replace :: SubPath -> String -> String -> String -> Slurpy C(x) -> Sealed (FL Prim C(x)) at which point it'll be a considerably safer function. But of course, when we have type witnesses on Slurpies, we can make a safe smart_diff. But force_replace :: SubPath -> String -> String -> String -> Slurpy -> Sealed (FL Prim C(x)) is still a considerable improvement. It can't enforce that you use an appropriate Slurpy, but it is still reasonably safe. > 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. No, =\/= is safe, smart_diff is not safe. =\/= is in a sense safe by definition, since without it we will have no type witnesses. And because it actually *is* safe, if we don't use any unsafe functions such as smart_diff. >> 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. It's mostly just that it seems like a waste of my time. It isn't going to lead to these patches being applied to darcs, and I don't see that it is the best way to improve your thesis. And we've lost enough time due to discussing code that wasn't right without working on making it actually right (i.e. Repository.Internal sans RIO, where there is no correct answer). I just don't see what your goal is, to be honest. If it is to get this code into darcs, you'll have to submit it in such a way that it can be reviewed. If it isn't to get this code into darcs, it'd be better to make that clear. It's true that I can answer your questions, I just don't really see what that will gain either you or me, if I'm unable to properly review your changes. > 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. Sure. There's certainly no urgency to get this done, which is why I see no reason to do it any other way than right. Type witnesses that aren't right don't provide the safety we're looking for, and are just a burden on potential contributors. David _______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
