snowleopard / build

Build Systems à la Carte
MIT License
242 stars 18 forks source link

Rewrite all functions on top of runZoom #15

Closed ndmitchell closed 6 years ago

ndmitchell commented 6 years ago

Advantages:

I'm a fan!

snowleopard commented 6 years ago

@ndmitchell One downside is that go is back to keeping unnecessary Chain k in the state. Can't you get rid of it with another runZoom?

ndmitchell commented 6 years ago

@snowleopard a standard zoom would let us hide the Chain k in the state. I don't think its worthwhile for the complexity though.

snowleopard commented 6 years ago

I like that there is a single adapter function runZoom, but I don't like the underlying complexity and the dependency on the lens library. Would it be too difficult to implement the required combinator locally, so the solution becomes more self-contained and amenable for further simplifications?

ndmitchell commented 6 years ago

I think the complexity is of MonadState, it's a principled and not overly complex thing in itself. The use of lens is only to make the first argument to runZoom simpler - we could ditch it easily - but you'd have to pass a s -> (a, a -> s) function yourself.

snowleopard commented 6 years ago

The choice seems to be between three simple adapters, which readers would be able to write themselves, especially after seeing liftStore, and one complex adapter, which they likely won't be able to write.

I think I prefer three simple ones.

But, of course, three simple ones don't give us a way to get rid of the duplication of i...

ndmitchell commented 6 years ago

The three simple ones occur in lots of places, and the definition of suspending confuses me with all the lift and what not. The runZoom might be harder to implement, but feels like a generic primitive deployed in a single place.

snowleopard commented 6 years ago

The use of lens is only to make the first argument to runZoom simpler - we could ditch it easily - but you'd have to pass a s -> (a, a -> s) function yourself.

So, we could add something like data InfoLens = InfoLens { viewInfo :: ..., setInfo :: ... }, and a similar datatype for FstLens and then we don't need to depend on lens? I think that would be preferable. The less magic, the better :)

P.S.: Actually, my datatypes above make little sense, we do need parameterised ones.

snowleopard commented 6 years ago

Ah, I guess we then won't be able to compose the lenses as easily :(

snowleopard commented 6 years ago

@ndmitchell What about using a less polymorphic but simpler function for suspending?

runZoom :: Task (MonadState i) k v -> (k -> State (Store i k v, Set k) v) -> State (Store i k v, Set k) v

In this way we do not need to mention any lenses in the paper, but can still get rid of the duplication of i and all the lifts in the implementation of suspending.

For consistency, we could also adapt simpler liftStore and liftInfo to runZoom equivalents taking a Task as an argument. We could then show the implementation of one of them, hopefully convincing the reader that the implementation of the two others is doable, but uninteresting.

Another alternative is to introduce the following combinator:

zoomTask :: Task (MonadState i) k v -> Task (MonadState (Store i k v, Set k)) k v

This has the advantage of a simpler type signature, but the implementation might actually be harder.

ndmitchell commented 6 years ago

I think the zoomTask signature would be nicer - although then doesn't it only work for one specific instance? I think having lots of lift/unlift things is a problem.

simonpj commented 6 years ago

I'm not following this thread in detail (yet, anyway).

I'm not keen on taking a dependency on lens. Not so much because it pulls in a big library; more because it pulls in a dependency on some intellectual infrastructure that not everyone will have.

I'd rather have a few well-chosen combinators, whose type signatures make sense in the context of our paper, and whose implementations are straightforward if tiresome (combinations of lifts).

I agree that the lift stuff currently rather clutters up the code; modularising would be good.

I don't understand the zoomTask signature

zoomTask :: Task (MonadState i) k v -> Task (MonadState (Store i k v, Set k)) k v

What is this Set k business?

Is this relevant to the paper?? RSVP.

ndmitchell commented 6 years ago

It is relevant to the paper because either we explain a zoom function or the lifts in S5. We have to focus on one or the other.

snowleopard commented 6 years ago

I don't understand the zoomTask signature What is this Set k business?

@simonpj We have a Task (MonadState i) but we typically want to run it in a state monad with a larger state, for example in State (Store i k v, Set k) instead of State i. Rignt now we solve this using various liftX functions, but the suspending scheduler is still quite complex.

@ndmitchell figured out a general lens-based combinator runZoom, which runs a Task (MonadState i) in an arbitrary State x provided you have a lens from x to i.

I'm not keen on using lenses either, so I suggested to instead add a dedicated zoomTask specifically for simplifying the suspending scheduler with the type:

zoomTask :: Task (MonadState i) k v -> Task (MonadState (Store i k v, Set k)) k v

With it, the suspending scheduler will look like this:

suspending :: forall i k v. Ord k => Scheduler Monad i i k v
suspending rebuilder tasks target store = fst $ execState (build target) (store, Set.empty)
  where
    build :: k -> State (Store i k v, Set k) ()
    build key = case tasks key of
        Nothing -> return ()
        Just task -> do
            done <- gets snd
            when (key `Set.notMember` done) $ do
                value <- gets (getValue key . fst)
                let newTask :: Task (MonadState (Store i k v, Set k)) k v
                    newTask = zoomTask $ rebuilder key value task
                    fetch :: k -> State (Store i k v, Set k) v
                    fetch k = do build k                          -- build the key
                                 getValue k . fst <$> get
                newValue <- run newTask fetch
                modify $ \(s, d) -> (updateValue key value newValue s, Set.insert key d)

I think this is much nicer than what we have at the moment and doesn't have any lenses.

ndmitchell commented 6 years ago

Closing as universally reviled!

ndmitchell commented 6 years ago

For info (mostly for Simon) we agreed to have a liftRun that only matches the signature required for suspending, and thus can be much simpler. I'm adding that to the paper now.