Run tracing functions in MonadState context

snowleopard / build

Build Systems à la Carte

MIT License

247 stars 18 forks source link

Run tracing functions in MonadState context #6

Closed snowleopard closed 6 years ago

snowleopard commented 6 years ago

@ndmitchell The bug you recently spotted in traces made me realise that we might want to move record and verify functions from Monad to MonadState context.

Not sure if that's really an improvement, so raising this PR to discuss -- what do you think?

ndmitchell commented 6 years ago

Good idea. Don't use put, use modify instead.

snowleopard commented 6 years ago

@ndmitchell Thanks! Will do the remaining changes later today.

snowleopard commented 6 years ago

@ndmitchell How does this look?

ndmitchell commented 6 years ago

I preferred it without pushing MonadState into the trace functions. Before you could tell what the trace function did, what it read, when, where it went, what the outputs were etc. Now it just mutates arbitrary state however it pleases.

snowleopard commented 6 years ago

@ndmitchell I agree that it's not ideal. However, "it just mutates arbitrary state however it pleases" may be an exaggeration -- the difference is actually rather small:

Before, we knew that verifyVT-like functions could only read the whole VT. Now they can both read and write the whole VT.
The way we used recordVT-like functions meant they could do an arbitrary change to the whole VT, which is the same now.

It's not clear that we really gained anything by this refactoring though: I'm pretty sure one can still make a subtle bug while mutating the state. So, I'm happy to abandon this PR (but I'll probably keep the removal of double-negation which was a bit confusing).

ndmitchell commented 6 years ago

Removing the double negation seems fine - I was just trying to make everything compute dirty, for consistency, but that's no big deal.

I do prefer not having the monadstate. Could we make recordVT not take the initial trace and have the caller do modify <>?

snowleopard commented 6 years ago

Could we make recordVT not take the initial trace and have the caller do modify <>?

We can, but only in simplest cases. In general, recording a trace seems to require looking at the complete 'trace database', e.g. see DCT.

ndmitchell commented 6 years ago

Yep, to me that's interesting, and we shouldn't be seeking to unify all trace related functionality under the same interface - they all look at different fragments of the database, and that should be reflected in their API's.