Port "available actions" code from Vonnegut frontend #159

georgefst · 2021-10-06T17:13:41Z

This is the first significant step towards #60.

This is in preparation for them being used elsewhere.

These are equivalent to definitions from our old frontend, and will shortly be used by some code we port from there.

This is a straightforward translation of `Vonnegut.AST.unfoldFun` from our old frontend, using a pair instead of an anonymous record.

This makes way for some upcoming changes which would otherwise cause module dependency cycles, since `Primer.App` is near the top of the graph.

….Common`

There are only trivial changes: - We replace `top` with the Haskell equivalent `maxBound`. - Changes to the module header. See: ``` git remote add vonnegut https://github.com/hackworthltd/vonnegut git fetch vonnegut georgefst/frontend-refactors-for-backend-port git diff vonnegut/georgefst/frontend-refactors-for-backend-port:frontend/src/Vonnegut/ActionPriorities.purs primer/src/Primer/Action/Priorities.hs ```

Changes are mostly trivial - see: ``` git remote add vonnegut https://github.com/hackworthltd/vonnegut git fetch vonnegut georgefst/frontend-refactors-for-backend-port git diff vonnegut/georgefst/frontend-refactors-for-backend-port:frontend/src/Vonnegut/Actions.purs primer/src/Primer/Action/Available.hs ``` The only subtle change is that `exprMeta` and `exprMetaTraversal` are renamed to `exprMetaLens` and `exprMeta` respectively (and similarly `typeMeta` etc.), due to a mismatch in naming conventions between the old frontend and backend.

This was unused in the old frontend before porting. It's just that the PureScript compiler doesn't warn about such things.

We use this to avoid passing the same `toProgAction` function around everywhere, instead simply mapping over the structure at the end.

These were unused in the old frontend before porting, but we wouldn't have noticed, since the PureScript compiler doesn't warn about such things.

These are all type/kind arguments to an `ActionSpec`, used by `realise` to match `ActionSpec`'s phantom parameter.

primer/src/Primer/Action/Available.hs

georgefst · 2021-10-07T14:59:44Z

weeder.dhall

@@ -1 +1,6 @@
-{ roots = [ "^Main.main$" ], type-class-roots = False }
+let
+    -- TODO remove these once this code (recently ported from old frontend) is exercised


Should be tracked in an issue.

primer/src/Primer/Core.hs

primer/src/Primer/Action/Available.hs

primer/src/Primer/Action.hs

primer/src/Primer/Core.hs

georgefst · 2021-10-07T16:27:06Z

primer/src/Primer/Action.hs

+  where
+    avoid :: [Text]
+    avoid = Map.elems $ map (unName . defName) defs
+


This and Level possibly belong somewhere more general than Primer.Action. But this seemed a convenient place to put them for now.

primer/src/Primer/Action.hs

…rsive

primer/src/Primer/Core.hs

georgefst · 2021-10-07T17:02:00Z

One recurring problem here is a lack of clarity about which module to put things in, especially when it comes to avoiding dependency cycles.

In general, perhaps we should clarify the scope and responsibilities of each module. Having a brief top-level Haddock comment for each module in the core library would be a good start.

primer/src/Primer/Core.hs

primer/src/Primer/Action.hs

brprice · 2021-10-11T13:44:06Z

primer/src/Primer/Action/Available.hs

+-- Type parameter 'p' is a ghost parameter that provides some type
+-- safety, to prevent the accidental mixing of actions for different
+-- types (e.g., to prevent the mixing of actions for 'Expr's and
+-- 'Type's). The argument of type 'p' in the type signature is not
+-- used other than to provide proof that you have a value of type 'p'.
+type ActionSpec p a =
+  p -> ([Action] -> [ProgAction]) -> Meta a -> OfferedAction


I wonder if it is worth taking advantage of GADTs here?

I don't know how this will affect automatic deriving of serialisation code though (if that is a thing we are going to do). Though I expect we will not serialise ActionSpecs, but only OfferedActions?

I wonder if it is worth taking advantage of GADTs here?

Yeah this was a not-very-satisfying thing I did in PureScript to try to get get better type-safety. Now that we're on the Haskell side there's no reason not to use its full power.

I don't know how this will affect automatic deriving of serialisation code though (if that is a thing we are going to do). Though I expect we will not serialise ActionSpecs, but only OfferedActions?

Agreed, and possibly even something simpler than that. Pretty much all we need to send to the frontend is what to display on the button, and an identifier of some kind to send back to the backend to say, "Here's the action the student selected."

I'm not sure exactly what you're proposing if you mean turning ActionSpec in to a GADT?

What I could see us wanting is for Action to be a GADT, with each constructor carrying a tag which tells us whether it applies to a type or expression. Then, in theory, we could write something like applyAction :: a -> Action a -> m (Either ActionError a). But this is difficult. I think it's best that I open a separate issue and link to the branch in which I have a very messy WIP.

Sure, let's punt this until later.

brprice

Thanks for taking on this large bit of work! Before we merge, I would like some clarity around how we expect to integrate this into the API. In particular I have some comments at #159 (comment).

Other than this I have only small comments, left above.

Existing `True` or `False` values have been set to `Destructive` or `Primary` respectively.

This untangles our module dependency tree a little, in particular removing the dependency of `Question` on `Action`. In the next commit, we will introduce the inverse dependency. Requires generalising the type signatures of `mkFreshName` and `mkFreshNameTy`. All this does is remove some redundant constraints coming from the `ActionM` synonym. Other than this, the contents of `Fresh.hs` (except the module header) come verbatim from `Action.hs`

This type was ported from PureScript, which does not generate field selector functions, so it was safe in that context.

georgefst · 2021-10-12T14:55:34Z

bors merge

ghost · 2021-10-12T15:09:08Z

Build succeeded:

ci/hydra:hackworthltd:primer-bors-staging:required

georgefst · 2022-11-14T12:55:00Z

My only regret is that we didn't do some of these things in the original implementation: in retrospect, it seems like a bad idea to have moved so much "smarts" into the frontend, regardless of what language the frontend was written in, especially given our intention to open source the Haskell implementation and make it generally useful without needing our particular frontend implementation. Ultimately, that was a poor decision on my part to split things up the way we did.

Historical notes, for the curious:

We calculated available actions on the frontend from essentially the very beginning. It was pretty simple, and a frontend/backend split at that point would probably have seemed unnecessary over-engineering.

We missed an opportunity to move the logic to the backend when we separated it from rendering code a few weeks later, creating Actions.purs. But at that point, it was clearly more expedient to keep it on the frontend.

Actions.purs gradually expanded in complexity. It ended up being the only significant chunk of frontend code which we decided was worth porting to the backend. We did so in #159. It was then heavily re-written in this PR.

georgefst · 2022-11-14T13:47:45Z

My only regret is that we didn't do some of these things in the original implementation: in retrospect, it seems like a bad idea to have moved so much "smarts" into the frontend, regardless of what language the frontend was written in, especially given our intention to open source the Haskell implementation and make it generally useful without needing our particular frontend implementation. Ultimately, that was a poor decision on my part to split things up the way we did.

Historical notes, for the curious:

We calculated available actions on the frontend from essentially the very beginning. It was pretty simple, and a frontend/backend split at that point would probably have seemed unnecessary over-engineering.

We missed an opportunity to move the logic to the backend when we separated it from rendering code a few weeks later, creating Actions.purs. But at that point, it was clearly more expedient to keep it on the frontend.

Actions.purs gradually expanded in complexity. It ended up being the only significant chunk of frontend code which we decided was worth porting to the backend. We did so in #159. It was then heavily re-written in this PR.

Whoops, I put this on the wrong thread (juggling too many open tabs with similar names...): #762 (comment),

georgefst added 11 commits September 28, 2021 15:40

[refactor] Move expr/type metadata lenses in to Core

8aeb322

This is in preparation for them being used elsewhere.

Implement some useful lenses and traversals

9f9446e

These are equivalent to definitions from our old frontend, and will shortly be used by some code we port from there.

Add a function for unfolding function types

ea17808

This is a straightforward translation of `Vonnegut.AST.unfoldFun` from our old frontend, using a pair instead of an anonymous record.

[refactor] Move some types from Primer.App module to Primer.Action

e6aae32

This makes way for some upcoming changes which would otherwise cause module dependency cycles, since `Primer.App` is near the top of the graph.

Copy some useful definitions from old frontend's `Vonnegut.Projection…

9ac4b95

….Common`

Remove unused definition findNode

389c718

This was unused in the old frontend before porting. It's just that the PureScript compiler doesn't warn about such things.

[refactor] Parameterise OfferedAction over the underlying action type

3eeaa8e

We use this to avoid passing the same `toProgAction` function around everywhere, instead simply mapping over the structure at the end.

Remove unused metadata arguments

43ea107

These were unused in the old frontend before porting, but we wouldn't have noticed, since the PureScript compiler doesn't warn about such things.

Silence unused variable warnings

0d8e966

These are all type/kind arguments to an `ActionSpec`, used by `realise` to match `ActionSpec`'s phantom parameter.

georgefst force-pushed the georgefst/frontend-actions-port branch from 16fcd92 to 0d8e966 Compare October 7, 2021 14:10

georgefst added 4 commits October 7, 2021 15:21

Use Haddock syntax in all code recently ported from old frontend

cb4bd98

HLint pass

ea5499e

Use haddock subheadings in Primer.Action.Priorities

c2c3042

Add un-utilised action modules to weeder roots

05a1456