Add .NET 10 support: fix InvalidProgramException in expression compilation#1429
Add .NET 10 support: fix InvalidProgramException in expression compilation#1429danielgerlag wants to merge 4 commits into
Conversation
…ation Pre-compile dynamic expressions outside closures/local functions in DefinitionLoader to avoid InvalidProgramException on .NET 10's updated expression tree compiler. The previous pattern of calling LambdaExpression.Compile() inside closures that capture expression objects produces invalid IL on .NET 10. Changes: - BuildScalarInputAction: compile expression before local function - BuildObjectInputAction: pre-scan JObject and compile all @-prefixed expressions at definition load time instead of at each invocation - AttachDirectlyOutput: pre-compile source expression before lambda - AttachNestedOutput: pre-compile both target and source expressions - MemberMapParameter: cache compiled source delegate in constructor - Add net10.0 to test TFMs (Directory.Build.props, UnitTests, IntegrationTests) Fixes #1428 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses a .NET 10 InvalidProgramException observed during dynamic expression compilation by moving LambdaExpression.Compile() calls out of closures/local functions and caching compiled delegates to avoid problematic capture patterns in the updated .NET 10 expression tree compiler.
Changes:
- Pre-compile and cache delegates for scalar/object input expressions and output mappings in
DefinitionLoader. - Cache the compiled source delegate in
MemberMapParameterto avoid recompiling on each assignment. - Add
net10.0as a test target framework across test props and key test projects.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/WorkflowCore.UnitTests/WorkflowCore.UnitTests.csproj | Multi-target unit tests on net6/net8/net10. |
| test/WorkflowCore.IntegrationTests/WorkflowCore.IntegrationTests.csproj | Multi-target integration tests on net6/net8/net10. |
| test/Directory.Build.props | Adds net10.0 to default test TFMs. |
| src/WorkflowCore/Models/MemberMapParameter.cs | Caches compiled source delegate to avoid repeated compilation. |
| src/WorkflowCore.DSL/Services/DefinitionLoader.cs | Pre-compiles expressions outside closures and caches compiled delegates for inputs/outputs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| foreach (var child in subobj.Children<JObject>()) | ||
| scanStack.Push(child); |
There was a problem hiding this comment.
Good catch, but this is intentional here: the pre-scan deliberately mirrors the exact same Children<JObject>() traversal the runtime acn uses below, so the pre-compiled compiledExpressions dictionary is guaranteed to contain a key for every @-property the runtime scan will later look up — no KeyNotFound risk. The nested-object limitation you spotted is pre-existing behavior on master (this PR only lifts the compile step out of the closure to fix the .NET 10 InvalidProgramException); making the template walk recurse into nested objects is a separate behavior change I would rather not fold into this fix.
| if (prop.Name.StartsWith("@")) | ||
| { | ||
| var sourceExpr = DynamicExpressionParser.ParseLambda(ParsingConfig, false, new[] { dataParameter, contextParameter, environmentVarsParameter }, typeof(object), TransformExpression(prop.Value.ToString())); | ||
| var exprText = prop.Value.ToString(); | ||
| object resolvedValue; | ||
| try |
There was a problem hiding this comment.
This Children<JObject>() line is unchanged from master — it is the existing runtime traversal, not something introduced by this PR. Scope here is limited to the .NET 10 compile fix, so I am keeping the traversal behavior identical. Happy to open a follow-up to properly recurse nested objects/array elements (in both the pre-scan and this loop together) if we want to support nested @ expressions.
Covers the reporter's exact scenario: a scalar variable-binding input
(data.MessageId) plus a scalar string-literal input ("waits-for-batching").
Loads the definition and assigns the inputs the same way
WorkflowExecutor.ExecuteStep does, asserting both values resolve.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The Azure-Tests job runs a repo-wide 'dotnet restore', which now fails with NETSDK1045 because the test projects target net10.0 but this job only installed the .NET 8/9 SDKs. Align it with the other jobs (6/8/9/10). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Summary
Fixes #1428 —
InvalidProgramExceptionfromDefinitionLoader.BuildScalarInputActionwhen running on .NET 10.Root Cause
Several methods in
DefinitionLoader.cscalledLambdaExpression.Compile()inside closures / local functions that captured the expression objects, and recompiled on every invocation. On .NET 10's updated expression-tree compiler this pattern can produce invalid IL at runtime.Changes
Expression pre-compilation (
DefinitionLoader.cs)BuildScalarInputAction: compile the expression before the local function instead of on each invocation.BuildObjectInputAction: pre-scan the JObject template and compile all@-prefixed expressions at definition-load time.AttachDirectlyOutput: pre-compile the source expression before the lambda.AttachNestedOutput: pre-compile both target and source expressions before the lambda.Expression pre-compilation (
MemberMapParameter.cs)Assigncall.Regression test (issue #1428)
data.MessageId) plus a scalar string-literal input ("waits-for-batching"). It loads the definition and assigns the inputs the same wayWorkflowExecutor.ExecuteStepdoes, asserting both values resolve. Backed by newScalarInputData/ScalarInputSteptest assets.Test framework (.NET 10 TFMs)
net10.0totest/Directory.Build.props,WorkflowCore.UnitTests.csproj, andWorkflowCore.IntegrationTests.csproj.CI
dotnet restore, which failed withNETSDK1045once the test projects began targetingnet10.0; it now matches the other jobs (6/8/9/10).masterto bring the branch current (it was behind the Azure Table Storage provider work).Testing
Elasticsearch-Tests, which is a pre-existing infrastructure flake (Elasticsearch container health-check timeout) that fails identically onmasterand is unrelated to this change.Note on the reported crash
The
InvalidProgramExceptionoriginates in the JIT (RuntimeHelpers.CompileMethod). I was unable to reproduce it on any GA/servicing .NET 10 build (10.0.0-preview.1/.4/.6/.7, 10.0.0 GA, 10.0.10) across arm64 and x64 — so it appears tied to a specific runtime build/architecture. This change is still the correct fix: it removes the fragileCompile()-inside-closure pattern and eliminates per-invocation recompilation (a real perf win).