Unify IAsyncTexlFunction #2751
Labels
Comments
MikeStall
added a commit
that referenced
this issue
Jan 22, 2025
Address #2751. This unifies most of them. To scope this PR, there are subissues on 2751 for remaining issues. We have a plethora of IAsyncTexlFunction* interfaces. Want to consolidate on 1, and make it public. This Ttake the parameters on the various IAsyncTexlFunction* overloads and move them as properties on a new public `FunctionInvokerInfo` class. And then have a new IAsyncTexlFunction* that takes the invoker. And remove all the others, migrating them onto this - and fixing the various bugs/hacks along the way that would impede migration. Most fundamentally, this FunctionInvokerInfo must have access to interpreter state and so must live in the interpreter. (Whereas some of the IAsyncFunction* interfaces live in core). Successfully removes several here so we get a net reduction, and shows a path to remove the rest. This problem quickly touches other problems: - **How to map from the TexlFunction to the invoker**. This is made more complicated because we register on PowerFxConfig, but that lives in Fx.Core and can't refer to interpreter implementations. - **dll layering around Fx.Json**: Some function implementations live in Fx.Json, but that specifically _does not_ depend on interpreter (because it is included by PowerApps). - **Standard Pipeline**: how to handle default args, common error handling scenarios, etc. Today, that's still in interpreter. Whereas we really want those checks to be in the IR and shared across front-ends (and available to designer). - **Split up Library.cs and class** - we have one giant class, partial over many files, containing 100s of function impls. Just give each impl its own file, similar to how we did for TexlFunction and the static declarations. - **lack of unit testing**: Today, most of our interpreter coverage comes from .txt files. But we should have C# unit tests on interpreter classes that provide coverage on core classes, like EvalVisitor. Some related prereq fixes that will directly help here: - Can we remove runner/context from Join? and leverage LambdaValue - which already closes over these. - fix Json layering problem. - IsType/AsType _UO shouldn't live in Json. They should work on any UO. - Remove _additionalFunctions from the serviceProvider. --------- Co-authored-by: Mike <jmstall@microsoft.com>
anderson-joyle
pushed a commit
that referenced
this issue
Jan 23, 2025
Address #2751. This unifies most of them. To scope this PR, there are subissues on 2751 for remaining issues. We have a plethora of IAsyncTexlFunction* interfaces. Want to consolidate on 1, and make it public. This Ttake the parameters on the various IAsyncTexlFunction* overloads and move them as properties on a new public `FunctionInvokerInfo` class. And then have a new IAsyncTexlFunction* that takes the invoker. And remove all the others, migrating them onto this - and fixing the various bugs/hacks along the way that would impede migration. Most fundamentally, this FunctionInvokerInfo must have access to interpreter state and so must live in the interpreter. (Whereas some of the IAsyncFunction* interfaces live in core). Successfully removes several here so we get a net reduction, and shows a path to remove the rest. This problem quickly touches other problems: - **How to map from the TexlFunction to the invoker**. This is made more complicated because we register on PowerFxConfig, but that lives in Fx.Core and can't refer to interpreter implementations. - **dll layering around Fx.Json**: Some function implementations live in Fx.Json, but that specifically _does not_ depend on interpreter (because it is included by PowerApps). - **Standard Pipeline**: how to handle default args, common error handling scenarios, etc. Today, that's still in interpreter. Whereas we really want those checks to be in the IR and shared across front-ends (and available to designer). - **Split up Library.cs and class** - we have one giant class, partial over many files, containing 100s of function impls. Just give each impl its own file, similar to how we did for TexlFunction and the static declarations. - **lack of unit testing**: Today, most of our interpreter coverage comes from .txt files. But we should have C# unit tests on interpreter classes that provide coverage on core classes, like EvalVisitor. Some related prereq fixes that will directly help here: - Can we remove runner/context from Join? and leverage LambdaValue - which already closes over these. - fix Json layering problem. - IsType/AsType _UO shouldn't live in Json. They should work on any UO. - Remove _additionalFunctions from the serviceProvider. --------- Co-authored-by: Mike <jmstall@microsoft.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
We shouldn't have 5+ variations of IAysncTexlFunction plus AsyncFunctionPtr delegate.
We should be able to remove this branch in EvalVisitor and have it be a single dispatch.
Power-Fx/src/libraries/Microsoft.PowerFx.Interpreter/EvalVisitor.cs
Line 311 in 07a817b
Ideally, unify across:
Broader design things to consider:
This is an engineering hygiene issue. Need to come up with a good design.
The text was updated successfully, but these errors were encountered: