JacquesCarette
Add LValues to Gool
Inside of InterfaceCommon.hs we have the following very suspicious bit of code:
class (VariableSym r) => VariableElim r where
variableName :: r (Variable r) -> String
variableType :: r (Variable r) -> r (Type r)
A quick search reveals that we only use variableName to write this following cursed bit of code:
arrayElem :: (OORenderSym r) => SValue r -> SVariable r -> SVariable r
arrayElem i' v' = do
i <- IC.intToIndex i'
v <- v'
let vName = variableName v ++ "[" ++ render (RC.value i) ++ "]"
vType = listInnerType $ return $ variableType v
vRender = RC.variable v <> brackets (RC.value i)
mkStateVar vName vType vRender
In short, we are using the variableName method to build a new "variable" name that looks like x[e] so that we can assign to array indices 😱
We should fix this by introducing an explicit notion of l-value to gool, and making both variables and array indices lvalues. This issue is currently blocking the state refactor that @B-rando1 is working on, so we should fix it in the course of that work.
After some further investigation, it does seem like Gool does have a concept of lvalues (though it calls them VariableSym). The problem here really lies with LanguagePolymorphic, which tries to implement these in a "generic" way. Unfortunately, doing so requires us to expose a bunch of the representation information as part of the API, which is not great.
LanguagePolymorphic was done in an "opportunistic" manner, i.e. abstracting out the code patterns as they occurred. It was not done in a semantics-driven way -- basically because too many things were still moving around.
If some abstractions are premature, the right solution is to undo them completely, i.e. merge the code back in to its point-of-use. In particular, if GOOL's conception of lvalues is broken, which it seems it is, this should be undone. Sometimes what this will imply is that some generic code will need to become new interfaces in (potentially new) classes along with specialized instances.
After further investigation, I think that LanguagePolymorphic should probably be razed to the ground, and we should take large parts of RenderClasses with it. This is causing enough problems with the state refactor that it probably makes sense to put that on ice for now, and fix the architectural issues first.
Typical of Drasil/GOOL refactor: current refactor can't be done without another one being done first. I have no problem with LanguagePolymorphic disappearing. I would need to hear more about the problems caused by RenderClasses first before saying "kill that too".
This is an extension of the Elim problem that was mentioned before. Most of those classes are things like InternalVarElim:
class InternalVarElim r where
variableBind :: r (Variable r) -> Binding
variable :: r (Variable r) -> Doc
The sole purpose of this class is to break the abstraction barrier of Variable. This caused heaps of problems with our state refactor, as r (Variable r) got replaced with just Variable r, which for most instances is something like State SomeState Doc. This makes writing things like variable obviously no good :)
Moreover, these get used almost exclusively to do abstraction-breaking evil purely for the sake of DRY or to try and recover what should be explicit in the code (EG: imports), so removing them would improve the interface quite a bit.
@TOTBWF's comment is more to-the-point, but I wrote this out already so I might as well add my thoughts 😅
I think #4377 is a typical example of the sorts of problems that RendererClasses creates. In general RendererClasses has a habit of mixing levels of abstraction, converting AST constructs to Doc and back again. I get the sense that this is a bit of a code smell in general, but for the purposes of this refactor the issue is that these mixed abstractions tend to entail assumptions about the structure of the language renderers, a structure that we are changing with the refactor.
An example of this would be the variable (see the class here). This is used by a lot of the renderers to create more abstract constructs, like in this example:
https://github.com/JacquesCarette/Drasil/blob/795898c6867dfe4f6bce8db710bf9c2f59125a0d/code/drasil-gool/lib/Drasil/GOOL/LanguageRenderer/PythonRenderer.hs#L988-L991
Reed and I discussed a lot in the last couple hours and I'm still digesting it all, but this is another piece of the motivations for a more 'drastic' refactor upfront.
Doc and back again is definitely evil and should not happen.
As usual with Drasil, a refactor very often needs other refactors to happen first. And that is only clear when the first refactor is attempted.
Hopefully those spawned refactors can be done in small increments.