purpleidea / mgmt

Next generation distributed, event-driven, parallel config management!
https://purpleidea.com/tags/mgmtconfig/
GNU General Public License v3.0
3.58k stars 311 forks source link

lang: ExprFunc must produce a FuncValue #701

Closed gelisam closed 1 year ago

gelisam commented 1 year ago

Here is the result of today's hacking session. It builds on top of #697, so the PR will be much easier to review after that is merged.

Things I've figured out since:

  1. func MergedGraph(ReversibleTxn, map[string]Vertex) (Graph, Func, error) still isn't right. It should either receive a ReversibleTxn or return a Graph, not both. In this PR I've kept the ReversibleTxn.
  2. ExprCall.Graph() needs to be updated. It should recur on the function and the arguments, obtaining one Func for the function and a list of Funcs for the arguments. Then construct a CallFunc with the function Func as an upstream node and with ArgVertices set to the argument Funcs.
  3. VarExpr.Graph() needs to be updated. This is where the map[string]Vertex is used: if the variable is a lambda parameter, then we should return the corresponding entry from that map. Since we need to return a Func, that map should probably be map[string]Func.
  4. If the variable is not in the map, then it's a lot less clear what to do. We currently recur on the variable's definition, but that will duplicate the definition for every use site instead of having the definition's node point to all the use sites. Perhaps the environment needs to include everything which is in scope, not just the lambda parameters? If so, does that mean that SetScope() also needs to be merged with Graph() and Func()?? I hope not!
  5. In $f($x + $x), we recur on the definition of $f and on the argument $x + $x. If for $f($x + $x) we have a map which says that $x is a lambda parameter, then for $x + $x that is still true, but for the definition of $f that might not be true, depending on whether $f is defined inside the same lambda as the $f($x + $x) call or higher up. So we need to adjust the map somehow.
gelisam commented 1 year ago

About (4): the variable must be in the map. SetScope() does not need to be merged with MergedGraph(). It is normal that there is a context during type-checking which associates variables with types, and also an environment during evaluation which associates variables with values. Both the context and the environment must have an entry for every variable which is in scope.

gelisam commented 1 year ago

About (5): no, we don't recur on the definition of $f nor the definition of `$x. Both of those variables should already be in the map, so we just return the Func which the map tells us to return, we don't recur.

gelisam commented 1 year ago

This PR has now been incorporated into #704. Closing.