Investigate Improving `External` class

[ecaustin]

As of right now, the HERMIT.Server.Parser.Transform module is a behemoth. This is largely due to the fact that it contains a number of External instances that are mutually recursive. Ideally, we'd find some way to split these up into separate modules.

To summarize the current implementation, External instances are split into two groups, one for "base" values and one for "functional" values. The lone instance for functional values uses the type family R to compute the return type for matchExternal, overloading the default implementation to parse a functional external value's arguments; hence the mutual recursion.

My current thought is to lift the R type family out of External so that we can use hs-boot files to move mutually recursive instances into separate modules.

@andygill is not a huge fan of hs-boot files, though, so is there an alternative way we can break the cycle?

[RyanGlScott]

Well, we could cut one head off of the hydra by removing the documentation from each function definition in HERMIT.Server.Parser.Transform, since external discards it anyway. Is there any reason to keep the documentation lying around, since it's copied directly from HERMIT?

[ecaustin]

@RyanGlScott, that was brought up in discussion last Thursday. The consensus was that we can get rid of the documentation fairly trivially, but reconstructing it once it's gone would take a significant amount of time.

We're erring on the side of caution and keeping it around until we're sure whether or not we want to implement something in the shell that corresponds to HERMIT's help command.

Edit: I probably should have mentioned that I raised this point with issue https://github.com/ku-fpg/hermit-shell/issues/10.

[ecaustin]

@andygill, you requested a specific example of the mutual recursion:

If you comment out the External (TransformH LCore LocalPathH) instance (lines 1212 - 1242 in the HERMIT.Server.Parser.Transform module) you should get the following error:

src/HERMIT/Server/Parser/Transform.hs:1424:7:
    No instance for (External
                       (Transform HermitC HERMIT.Monad.HermitM LCore LocalPathH))
      arising from a use of ‘external’
...

It's referring to the external value for focus which is our shell's alias to the hfocusT transformation. This transformation is of type TransformH LCore LocalPathH -> TransformH LCore String -> TransformH LCore String), thus focus relies on the functional instance of External. Given that, the External instance for TransformH LCore String now depends the instance for TransformH LCore LocalPathH.

The mutual recursion comes into play when two instances depend on each other, for example TransformH LCore () and RewriteH LCore. These happen to be two of the largest instances, so it would be nice to separate them regardless of if they still include their documentation strings or not.

[RyanGlScott]

I'd certainly support for using .hs-boot files in this case. They are definitely not conventional, but the sheer size of HERMIT.Server.Parser.Transform leaves a bad aftertaste. (AFAIK, Agda also resorted to .hs-boot files to manage its module hierarchy.)

[ecaustin]

Right, the only issue is that External is a type family which hs-boot files don't currently support.

As I mentioned, my proposed change is to lift the R type family out of External to solve this problem, but @andygill wanted to personally look into other solutions before we settled for this change.

[RyanGlScott]

If we do go down this route, I have a branch (hs-boot) which implements this idea. If you split the instances in HERMIT.Server.Parser.Transform into three modules (one for BiRewrites, one for Rewrites, and one for Transforms): then the module dependency graph looks like this:

          ____         ____
         /    \       /    \
BiRewrite      Rewrite      Transform
         \____/       \____/

Since Rewrite is a common vertex in these two cycles, only one .hs-boot file is needed, making the change pretty simple. The net effect is that HERMIT.Server.Parser.Transform goes from 1478 LoC to 1028 (and it won't get much smaller than that, since one instance takes up 934 LoC).

[ecaustin]

Documentation has been removed. Ryan has a branch waiting that uses boot files in case we want to go that direction.