On Thu, Aug 28, 2008 at 4:37 PM, Jason Dagit <[EMAIL PROTECTED]> wrote: >> there is no t' or t'' here, and there must not be. The Monad only >> allows actions that don't change state. > > I thought the point was for the Monad to allow actions that don't > *track* the state. We are doing IO under the hood after all. So it > seems like a lie to claim that we don't allow actions that change > state, since internally anything is still possible.
No, that's just another way of saying that the Monad allows users to violate the type witnesses. I want the Monad to allow *safe* actions, but not unsafe actions. In order to do this, it must allow only actions that don't change the repository state--which is almost all actions that we do. The only ones that modify state are tentatively*, finalizeRepositoryChanges and applyToWorking (and withGutsOf, if we move finalizeRepositoryChanges into withGutsOf). All the other actions won't change state, and will be safe, and can be performed in a Monad. > I still don't understand what is the difference between the derived > instance and the specified one above. I do understand the types of > the instances. The newtype deriving hasn't been a source of > confusion, at least not for me. I pointed out in my original email > that it might be wrong for our type safety. Clearly, the newtype > derived instance looks like this: > > instance Monad (IORepo p i j) where > (>>) :: IORepo p i j a -> IORepo p i j b -> IORepo p i j b > > While you're saying we should have: > instance Monad (IORepo p i i) where > (>>) :: IORepo p i i a -> IORepo p i i b -> IORepo p i i b > > I'm saying, I don't understand why the second one is safer than the first. The first one allows me to sequence two changes that act on the same state, and both replace that state with a new state. This is always a bug. The second (correctly) acknowledges that we can only sequence changes that don't modify the state. > Are you saying that the first one allows for IO actions that knowingly > change the repo state whereas the other one is safe because it only > allows IO actions that either unknowingly change the state or don't > change it at all? Yes, that's correct. Actions that unknowingly change the state should be hard to introduce (accidentally), once we're exporting a safe API from Darcs.Repository. It would require someone writing IO functions that explicitly write to the _darcs/ directory. Or importing unsafe functions from Darcs.Repository.HashedRepo or something like that. Unsafe imports can be checked with haskell_policy.sh, and explicit writes to _darcs are very easy to spot in code reviews. > It seems to me that return is more problematic than (>>) if your goal > is to disallow IO actions that might knowingly modify repository > state. On the other hand, a custom io :: IO a -> IORepo p i i a, > helps to make this safer in practice (not theory) since most IO > actions get in there via io. Why is return problematic? It seems like return is absolutely safe, since it's guaranteed to make no changes. >> See below about FlexibleInstances. > > FlexibleInstances seem to be documented here: > http://haskell.org/ghc/docs/latest/html/users_guide/type-class-extensions.html#instance-rules > > All that we're getting from FlexibleInstances is the ability to list > non-distinct type variables. Okay, that doesn't sound so bad anymore. > In our case, it's "InstancesWithEqualTypes" Great! So it sounds like there shouldn't be a barrier to introducing a safe Monad instance... unless FlexibleInstances turns out to trigger interesting compiler bugs or weird errors. >>> The problem for me is that I don't see how to apply this approach. I >>> thought we were trying to fix things like tentativelyMergePatches. >>> But, here we're now concerned with withGutsOf. >> >> withGutsOf brackets all the functions like tentativelyMergePatches. >> Or rather, it currently brackets all such cases that are dangerous on >> darcs-1 repositories (amend, obliterate, optimize --reorder). I'm >> proposing that we could extend it to bracket all uses of tentative >> functions, which would enable us to move those functions into RIO, >> without touching the rest of the code. > > So you're saying that I only have to modify functions which get > bracketed by withGutsOf? That might not be so bad. Right, that's what I'm saying. >> We could easily leave both functions in place. So we'd have >> >> unsafeTentativelyMergePatches :: ... -> IO (Sealed (FL Prim C(u))) >> >> and >> >> tentativelyMergePatches :: ... -> RIO (Sealed (FL Prim C(u))) >> >> and most likely tentativelyMergePatches would be implemented in using >> unsafeTentativelyMergePatches. >> >>> I don't have #1 for either approach which is a show stopper both for >>> type witnessing the commands and implementing RIO. >> >> Why is tentativelyMergePatches particularly problematic? > > In your example you didn't put in the rest of RIO, and that's the part > I'm not understanding. I thought the rest would follow naturally. > tentativelyMergePatches :: ... -> RIO p C( what goes here ?) (Sealed > (FL Prim C(u))) > > One problem is that I suspect the final tentative recorded state for > tentativelyMergePatches should be the same as the final context of the > Sealed (FL Prim C(u)). But, No, the Sealed (FL Prim C(u)) is a patch that applies to the working state. But it *is* true that the final tentative state should be sealed, and this does make it a very tricky function (which I hadn't thought about). > Supposing we have a SealedRIO, that hides the resulting tentative > recorded state: > > tentativelyMergePatches :: ... -> SealedRIO p C(r u t) (Sealed (FL Prim C(u))) > > Wouldn't this create problems for the caller? I think the two sealed > types are meant to be equal. No, they aren't meant to be equal, as you can see from the existing type for tentativelyMergePatches: tentativelyMergePatches :: RepoPatch p => Repository p C(r u t) -> String -> [DarcsFlag] -> FL (PatchInfoAnd p) C(x r) -> FL (PatchInfoAnd p) C(x y) -> IO (Sealed (FL Prim C(u))) The return value is sealed in the unrecorded state, which is because it's a patch to apply to the working directory (as you can see by examining uses of this function). So a SealedRIO type ought to work fine. We'd unfortunately need new combinators to handle this slightly different sort of action, maybe something like: (*>>=) :: SealedRIO C(r u t) a -> (FORALL(t') a -> RIO C(r u t' t'') b) -> RIO C(r u t t'') b Another possibility might be to break tentativelyMergePatches into a pure function returning existentials for both the (merged) patches to be added to the repository and the changes to be made to the working directory. I don't think this would be as nice as the SealedRIO approach, since it'd put more burden on the users (i.e. Darcs.Command.*). > Or perhaps we should give the type a name and add an unsafeCoerce# in > the right places to quiet the complains about escaping existential > types. I think it's the only place this comes up at the moment. Of course, we can always leave this function in the IO monad until after the rest of the RIO framework is in place, so we don't need to solve this problem immediately. David _______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
