server: validation code simplifications #4585
Conversation
Rather than defining a new monad class and constructing new instances of it, just use the `StateT` that it is really based on. Also get rid of some dead code, and Monoid instances that aren't actually Monoids. Also avoid an instance of Lenses where this doesn't add much value.
|
Deploy preview for hasura-docs ready! Built with commit fdffb10 |
|
Review app for commit fdffb10 deployed to Heroku: https://hge-ci-pull-4585.herokuapp.com |
abooij
changed the title
|
In fact, arguably we should define this in terms of writers rather than state: we should never need to know the current list of validated variables. |
|
I now see that #4111 already contains similar simplification work on |
| recordVariableUse :: G.Variable -> RQL.PGColumnType -> m () | ||
| markNotReusable :: m () | ||
| type MonadReusability m = MonadState QueryReusability m | ||
|
|
||
| instance (MonadReusability m) => MonadReusability (ReaderT r m) where | ||
| recordVariableUse a b = lift $ recordVariableUse a b | ||
| markNotReusable = lift markNotReusable | ||
| type ReusabilityT m a = StateT QueryReusability m a |
There was a problem hiding this comment.
I normally recommend against doing this. Why? Two reasons:
-
You can’t use
MonadStatefor something else when this transformer is somewhere in the stack. -
It’s a weaker abstraction. Code that is polymorphic in the monad but written against
MonadReusabilitycan only callrecordVariableUseandmarkNotReusable, which accumulate information monoidally. But usingMonadState, they can do arbitrary other things, including dropping the state altogether.
Perhaps the above two points seem like small potatoes, and maybe they are. But I don’t think they’re inconsequential enough to justify switching away from the abstraction just to be a little bit simpler!
One improvement that would be nice would be to switch from StateT to WriterT as the underlying transformer, since ReusabilityT is fundamentally about accumulating information monoidally, and it doesn’t need the full power of StateT. Alas, even the strict variant of WriterT leaks space, so I avoid using it for anything that isn’t tiny.
Fortunately, recent versions of transformers include Control.Monad.Trans.Writer.CPS, which is a non-leaky implementation of WriterT (it uses StateT under the hood), but mtl doesn’t include support for that in the currently-released version. It does support it on master (because I added support for it—probably at around the same time I wrote ReusabilityT, in fact!), but it doesn’t seem to have made its way into a release yet. I should probably bother the maintainers about that.
There was a problem hiding this comment.
Thanks for this background, Alexis. I completely agree with the points you raise. I am not as well-versed in the space leakage of the various monad transformers, so what you wrote is very informative.
The reason I opened this PR nonetheless is that I think in this case the abstraction of MonadReusability has a cost. There is an argument to be made against abstraction when this is in conflict with interpretability, as it was for me. But I'm sure we can resolve this conflict in the refactored version by making sure there is good documentation, and by minimizing dependency on this stateful computation. (At first sight this seems to be much better in the refactored version, which is why above I proposed to ignore this PR in lieu of that work.)
There was a problem hiding this comment.
FWIW I agree with you 100% that the “mtl-style” approach to effect composition is way, way too complicated, and we want something better. That is in fact precisely why I have written eff: I think monad transformers are the single largest obstacle to new Haskell programmers who want to get productive with the language, and I think algebraic effects are a better design.
Unfortunately, existing approaches to algebraic effects in Haskell aren’t very efficient, so eff is also a project in making them fast. And eff is fast… but it is fast because it depends on some modifications to GHC. I have an open GHC proposal to get those modifications merged, but until then we’re stuck with what we have.
So yes, less tangentially, I’m happy to include some more documentation wherever it would help. Please feel free to add some extra documentation wherever it would be helpful! (Or, for that matter, request for someone else to add some documentation if you don’t feel like you understand it enough to do it yourself.)
| instance Semigroup QueryReusability where | ||
| Reusable a <> Reusable b = Reusable (a <> b) | ||
| _ <> _ = NotReusable | ||
| instance Monoid QueryReusability where | ||
| mempty = Reusable mempty |
There was a problem hiding this comment.
You say this “isn’t actually a monoid,” but that’s just not true; this is a monoid. It satisfies all the monoid laws. It wouldn’t be a monoid if we defined mempty = NotReusable, but we don’t. As-written, it’s a perfectly cromulent monoid.
(You’re right that the other instances aren’t monoids, and we should get rid of those. But this one seems fine to me.)
There was a problem hiding this comment.
Yes, having taken a second look, you are right. So it makes sense to treat QueryReusability as a monoid.
|
Closing this for now, awaiting the results of #4111. |
|
Review app https://hge-ci-pull-4585.herokuapp.com is deleted |
Description
Rather than defining a new monad class and constructing new instances
of it, just use the
StateTthat it is really based on. I am proposing this change for several reasons: it is shorter, we reduce the amount of code that can be incorrect, and most importantly, it is easier to understand. At first I found it difficult to understandMonadReusability. But its description as a state monad explains everything to me. However, this description is only present in its implementation.This PR also gets rid of some dead code, and
Monoidinstances that aren'tactually monoids (such as the ones for
QueryReusabilityandUnionTyInfo).Also avoid an instance of prisms where this doesn't add much value. This has the added benefit of the code being more welcoming to programmers who are perhaps not so familiar with lenses/prisms.
Affected components
Server checklist
Catalog upgrade
Does this PR change Hasura Catalog version?
Metadata
Does this PR add a new Metadata feature?
GraphQL
Breaking changes