[SourceGeneration] Improve NodeSet to ModelDesign conversion and generated node state - #4132
[SourceGeneration] Improve NodeSet to ModelDesign conversion and generated node state#4132marcschier wants to merge 8 commits into
Conversation
Fixes several code generation defects that show up on combined and multi-namespace NodeSets. A standalone node that is already a method type, with no owning parent and the conventional MethodType name, is no longer given a synthesized declaration; that previously emitted a spurious "<Name>MethodTypeMethodType" node. Where a NodeSet already ships a method type declaration explicitly, the existing declaration is reused rather than emitting two identifiers with the same name. Method argument resolution, object type proxies, fluent builders and the node state generator are updated accordingly, along with the shared generation helpers and model design validation. The ISA95 NodeIds are regenerated as a consequence of these changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9e6a5abf-3299-4cd1-9855-010fedbf0ad8
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR improves OPC UA NodeSet → ModelDesign conversion and downstream code generation to better handle multi-namespace/combined NodeSets, method argument naming collisions, and correct runtime decoding/registration of generated nodes.
Changes:
- Added canonical C# identifier generation and improved string-literal escaping for generated code.
- Improved NodeSet import to better match ModelCompiler conventions (placeholder browse names, method declarations, DataTypeEncoding parenting).
- Updated generators (NodeState/ObjectType proxies/Fluent builders/NodeManager templates) to use stable method-argument symbol allocation and correct decoding contexts; added/expanded regression tests and fixtures.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/Opc.Ua.SourceGeneration.Core/Shared/SourceGenerationUtils.cs | Adds ToCSharpIdentifier and improves literal escaping for generated code. |
| tools/Opc.Ua.SourceGeneration.Core/Schema/NodeSetToModelDesign.cs | Fixes placeholder symbolic-name mapping, method argument naming, method-type reuse, and DataTypeEncoding parenting. |
| tools/Opc.Ua.SourceGeneration.Core/Schema/ModelDesignValidator.cs | Ensures generated method-argument code names are assigned during import/validation. |
| tools/Opc.Ua.SourceGeneration.Core/Schema/ModelDesignExtensions.cs | Introduces scoped, collision-aware generated names for method arguments (non-serialized). |
| tools/Opc.Ua.SourceGeneration.Core/Generators/ObjectTypeProxyGenerator.cs | Uses scoped argument identifiers and correct message context for decoding structured outputs. |
| tools/Opc.Ua.SourceGeneration.Core/Generators/NodeStateGenerator.cs | Uses scoped argument identifiers, correct decoding context, and handles argument value materialization from model parameters. |
| tools/Opc.Ua.SourceGeneration.Core/Generators/NodeManagerTemplates.cs | Adds __FindByDataTypeId to support VariableFromDataTypeId(...) in fluent APIs. |
| tools/Opc.Ua.SourceGeneration.Core/Generators/MethodDesignArgumentResolver.cs | Refactors argument resolution and assigns generated argument names as a side effect. |
| tools/Opc.Ua.SourceGeneration.Core/Generators/FluentBuilderGenerator.cs | Adds pass-through VariableFromDataTypeId overloads and minor doc formatting. |
| tests/Opc.Ua.SourceGeneration.Core.Tests/Shared/SourceGenerationUtilsTests.cs | Adds unit tests for new identifier/literal escaping helpers. |
| tests/Opc.Ua.SourceGeneration.Core.Tests/Schema/NodeSetToModelDesignTests.cs | Adds regression test covering preservation of runtime argument names. |
| tests/Opc.Ua.SourceGeneration.Core.Tests/Schema/ModelDesignExtensionsTests.cs | Adds tests for scoped method-argument name allocation and serialization behavior. |
| tests/Opc.Ua.SourceGeneration.Core.Tests/Resources/SameNamespaceEncoding.NodeSet2.xml | Adds fixture covering DataTypeEncoding nodes with same-namespace ParentNodeId. |
| tests/Opc.Ua.SourceGeneration.Core.Tests/Resources/SameNamedMethodArguments.NodeSet2.xml | Adds fixture covering argument naming/pathological characters and collisions. |
| tests/Opc.Ua.SourceGeneration.Core.Tests/Resources/MethodArgumentNamespace.NodeSet2.xml | Adds fixture covering method-argument datatype namespace resolution at runtime. |
| tests/Opc.Ua.SourceGeneration.Core.Tests/Generators/ObjectTypeProxyGeneratorTests.cs | Adds regression test for structured output decode using session message context. |
| tests/Opc.Ua.SourceGeneration.Core.Tests/Generators/NodeStateGeneratorTests.cs | Adds generator-level regressions for encoding nodes and argument symbol stability. |
| tests/Opc.Ua.SourceGeneration.Core.Tests/Generators/FluentBuilderGeneratorTests.cs | Updates assertions to match new codegen behaviors and file names. |
| tests/Opc.Ua.SourceGeneration.Core.Tests/CompilerUtils.cs | Adds helpers to create in-memory AdditionalText and collect generator diagnostics. |
| src/Opc.Ua.ISA95/Design/Common/Opc.ISA95.NodeIds.csv | Regenerates NodeIds to reflect placeholder symbolic-name mapping changes. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4132 +/- ##
==========================================
- Coverage 80.23% 79.92% -0.32%
==========================================
Files 1515 1524 +9
Lines 209980 210992 +1012
Branches 36213 36387 +174
==========================================
+ Hits 168479 168627 +148
- Misses 28867 29693 +826
- Partials 12634 12672 +38
🚀 New features to boost your workflow:
|
Resolve method inputs and outputs together where callers need both, and move generated code-name assignment into the generator scopes that consume those names. Clarify that description assignment already targets a NodeState-typed local, and avoid repeated XmlQualifiedName allocations for method argument browse names. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9e6a5abf-3299-4cd1-9855-010fedbf0ad8
The fluent builder generator emits calls to INodeManagerBuilder.VariableFromDataTypeId and to a NodeManagerBuilder constructor overload that carries the data-type lookup, but the runtime side of that API was missing, so every generated node manager failed to compile with CS1729 and CS1061. Adds the VariableFromDataTypeId resolution to the builder interface and implementation, along with NodeStateLookupExtensions.FindByDataType, which is the lookup the builder delegates to and has no other consumer. Resolution reports BadNodeIdInvalid for a null data type, BadNodeIdUnknown when nothing matches, BadBrowseNameDuplicated when the match is ambiguous, and BadTypeMismatch when the resolved node is not a variable. An optional browse name disambiguates a data type that is carried by more than one variable. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9e6a5abf-3299-4cd1-9855-010fedbf0ad8
|
Fixed the net10.0 solution build failure on this PR. The A generator change and the API it emits against have to land together, so the builder Also brings the eight Verified locally:
|
DiNodeManager constructed NodeManagerBuilder without the data-type resolver, so
VariableFromDataTypeId reported BadNodeIdUnknown ("no predefined variable has DataType")
for every DI node manager - a misleading error, since the lookup had simply never
been supplied rather than the variable being absent.
Delegates to NodeStateLookupExtensions.FindByDataType rather than hand-rolling the
scan a fourth time in this file.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9e6a5abf-3299-4cd1-9855-010fedbf0ad8
…ourcegen # Conflicts: # src/Opc.Ua.Di.Server/DiNodeManager.cs
…ourcegen # Conflicts: # tests/Opc.Ua.SourceGeneration.Core.Tests/Schema/NodeSetToModelDesignTests.cs # tools/Opc.Ua.SourceGeneration.Core/Generators/FluentBuilderGenerator.cs # tools/Opc.Ua.SourceGeneration.Core/Generators/MethodDesignArgumentResolver.cs # tools/Opc.Ua.SourceGeneration.Core/Generators/ObjectTypeProxyGenerator.cs # tools/Opc.Ua.SourceGeneration.Core/Schema/ModelDesignValidator.cs # tools/Opc.Ua.SourceGeneration.Core/Schema/NodeSetToModelDesign.cs
The generator no longer derives placeholder accessor names by sanitising the angle brackets of the placeholder browse name, which produced identifiers such as AddxBinding_ for a <Binding> placeholder. It now emits the intent directly, as AddBinding_Placeholder. The OpenUsd and OpenUsdScene server projects still called the old sanitised names, so they no longer compiled. Updates the thirteen call sites and the comments that referenced them. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9e6a5abf-3299-4cd1-9855-010fedbf0ad8
Summary
Stack context
This is PR 2 of a stacked split of integration PR #4093 into independently reviewable pieces. This PR is independent of the other stack PRs and contains no WoT content.
Validation
dotnet build tests\Opc.Ua.SourceGeneration.Core.Tests\Opc.Ua.SourceGeneration.Core.Tests.csproj -c Release -p:CustomTestTarget=net10.0 -v:mdotnet test tests\Opc.Ua.SourceGeneration.Core.Tests\Opc.Ua.SourceGeneration.Core.Tests.csproj -c Release -p:CustomTestTarget=net10.0 --no-builddotnet build tests\Opc.Ua.SourceGeneration.Core.Tests\Opc.Ua.SourceGeneration.Core.Tests.csproj -c Release -p:CustomTestTarget=net48 -v:mdotnet test tests\Opc.Ua.SourceGeneration.Core.Tests\Opc.Ua.SourceGeneration.Core.Tests.csproj -c Release -p:CustomTestTarget=net48 --no-builddotnet build src\Opc.Ua.ISA95\Opc.Ua.ISA95.csproj -c Release -f net10.0 -v:msrc\Opc.Ua.ISA95\Design\Common\Opc.ISA95.NodeIds.csvwith no unstaged regeneration diff.