Closed purpleidea closed 1 year ago
I don't think this makes sense.
It doesn't work.
Registering those Funcs appears to make it possible to call those functions by name, either from MCL code or from a CallExpr or a CallFunc constructed in golang. But that won't work.
All of those Funcs have public fields which need to be initialized in order for the Func to work properly, but with the register API, there is no way to initialize those fields. Thus, while it is indeed possible to look up those Funcs by name, doing so returns an uninitialized Func which will panic once applied to arguments.
It is not useful.
The hope is that by registering the Funcs, some piece of code will no longer need to refer to Funcs directly, instead that code will be able to call those Funcs by constructing a CallExpr which looks up the Funcs by name, thus avoiding an import cycle. But that won't work.
The import cycle was because some Funcs now construct part of the function graph at run-time. Since the function graph currently contains Exprs, Funcs now need to refer to Exprs. But Expr::Func() implementations already refer to Funcs, so there's an import cycle. For example, CallExpr::Func() refers to CallFunc, and CallFunc::Stream() is now constructing an ExprFunc. This PR would allow CallFunc::Stream() to refer to CallExpr instead of ExprFunc, but that doesn't break the cycle.
We have a much better solution under way.
The ongoing work to make the function graph hold Funcs instead of Exprs should break the cycle, because Funcs will now be able to construct their part of the function graph by referring to other Funcs. Since Funcs no longer need to refer to Exprs, that breaks the cycle.
I completely agree. I thought there was a chance the func API changed in a way that could have made it useful. But not worth thinking about now. Just pointing out the asymmetry.
If these are registered in the normal function system, perhaps we won't have future import cycles when we try and use them. Not sure if it can be made to work, but worth a try.