Assign instance NodeIds in generated CreateOrReplace<Child> helpers

[marcschier]

Description

The model source generator emits two families of instance helpers with inconsistent NodeId behaviour:

• ISystemContext.CreateInstanceOf<Type>(parent, browseName) rebases the whole materialised subtree onto per-instance NodeIds.
• <Owner>.CreateOrReplace<Child>(context, replacement) — the plumbing behind NodeState.CreateChild / ReplaceChild — did not. The child it produced was either bare (null NodeId, no browse name) or kept the type-level NodeId of the replacement.

Any node manager mixing the two had to hand-roll a recursive re-stamp before registration, or sibling instances collided on NodeIds — a failure that only surfaces at registration time. samples/PumpDeviceIntegrationServer did exactly this.

What changed

Generator (tools/Opc.Ua.SourceGeneration.Core)

• CreateOrReplace<Child> gains bool assignInstanceNodeIds = true. When enabled and the context supplies an ISystemContext.NodeIdFactory, a child whose NodeId is null or still equal to its declaration NodeId — and its descendants — are rebased through NodeInstanceExtensions.AssignInstanceNodeId / AssignInstanceChildNodeIds, with the owner as reference root. A NodeId the caller already assigned is never overwritten.
• The helper also stamps the new child's SymbolicName / BrowseName / DisplayName. Without an identity, a path-based INodeIdFactory derives {parent}_ for every child of a parent, so the assignment would have introduced collisions rather than removed them. BrowseName stamping is guarded on context.NamespaceUris != null — event-reporting paths pass a context without a namespace table.
• All generated Create* factory call sites pass assignInstanceNodeIds: false: they build declaration subtrees whose NodeIds must stay at their type-level values, and the enclosing CreateInstanceOf<Type> factory rebases the finished subtree in a single pass.
• The CreateInstanceOf<Type> rebase guard drops the parent != null requirement — supplying a browseName is what marks a dynamically materialised instance. Calling it without one (as the generated NodeStateActivators do, and when replacing a well-known singleton) still keeps the declaration NodeIds.
• replacement is now emitted as BaseInstanceState? on CreateOrReplace<Child> and on the generated FindChild override, matching the documented contract and the base NodeState.FindChild signature.

Hand-written twins — MethodState.CreateOrReplace{Input,Output}Arguments and BaseDataVariableState.CreateOrReplaceEnumStrings gain the same parameter and behaviour, sharing a new internal NodeInstanceExtensions.AssignNewChildInstanceNodeIds. This is also required so the generated public new overrides keep hiding a matching base signature (otherwise CS0109 with TreatWarningsAsErrors).

Sample — PumpDeviceIntegrationServer now passes the real DeviceSet parent to the factory and drops AssignChildNodeIds plus the manual root stamp.

Docs — new "Materialising instances at runtime — NodeId assignment" section in SourceGeneratedNodeManagers.md and a refreshed DeviceIntegration.md.

Design note: why not the guard proposed in the issue

Gating on "the owner already has an instance NodeId" cannot be implemented as owner.NodeId != <ownerTypeNodeId>. The generated child factories call <Owner>.CreateOrReplace<Child> on declaration instances — CreatePumpType_Operational runs on a FunctionalGroupState whose NodeId is ObjectIds.PumpType_Operational, not ObjectTypeIds.FunctionalGroupType — so the comparison would be true while the type template is being materialised (forInstance: false) and would rewrite the address space's declaration NodeIds. The explicit opt-out separates the two callers unambiguously.

Compatibility

Adding an optional parameter is source-compatible. Behaviour changes for callers of NodeState.CreateChild / ReplaceChild when the context carries a NodeIdFactory: the new child now carries a browse name and a per-instance NodeId instead of nothing.

Related Issues

• Fixes Source generator: CreateOrReplace<Child> helpers should assign instance NodeIds #4099

Note

The issue asks to delete AssignChildNodeIds from samples/MinimalRobotServer. That sample does not exist on master — it only lives on the still-open #4096 branch and carries no such helper. The equivalent boilerplate on master is PumpDeviceIntegrationServer/PumpNodeManager, which this PR cleans up. Details in this comment.

Note

One commit is an unrelated one-line-scope build-hygiene fix: two CA2000 warnings in ConsoleReferenceClient where subscription ownership transfers to the session. Happy to split it out if preferred.

Checklist

• I have signed the CLA and read the CONTRIBUTING doc.
• I have added tests that prove my fix is effective or that my feature works and increased code coverage.
• I have added all necessary documentation.
• I have verified that my changes do not introduce (new) build or analyzer warnings.
• I ran all tests locally using the UA.slnx solution against at least .net framework and .net 10, and all passed.
• I fixed all failing and flaky tests in the CI pipelines and all CodeQL warnings.
• I have addressed all PR feedback received.

Tests added

Generator emission (NodeManagerGeneratorTests) — CreateOrReplaceChildAssignsInstanceNodeIdsByDefault, TypeFactoriesOptOutOfCreateOrReplaceNodeIdAssignment, CreateInstanceOfFactoriesRebaseWithoutAnExplicitParent.

Helper behaviour (NodeInstanceExtensionsTests) — CreateOrReplaceArgumentsAssignsInstanceNodeIds, CreateOrReplaceArgumentsKeepsCallerAssignedNodeId, CreateOrReplaceArgumentsHonoursTheAssignmentOptOut, CreateOrReplaceEnumStringsAssignsInstanceNodeIds.

Pump DI server regression (PumpInstanceNodeIdRegressionTests, added on review feedback):

Test	Asserts
ConfiguredPumpExposesTheFullModelSurface	The exact set of 77 model identifiers a configured pump materialises.
EveryPumpInstanceExposesTheSameGeneratedModelSurface	A second pump exposes the identical generated surface, minus what the fluent Configure pass adds.
MaterialisedPumpNodeIdsComeFromTheNodeIdFactory	Every NodeId is {parentIdentifier}_{symbolicName} in the parent's namespace — minted by PumpNodeManager.New.
MaterialisedPumpNodeIdsAreUniqueAcrossInstances	No NodeId is null or shared between two instances of the type.
GeneratedFactoryAssignsInstanceNodeIdsBeforeRegistration	The factory output already carries per-instance NodeIds before AddPredefinedNodeAsync applies its defensive rebase.
AlarmSubtreeKeepsStandardDeclarationNodeIds	Pins the alarm subtree's standard-namespace ids — a pre-existing gap outside the generated helpers.

The 77-identifier baseline was captured by running the same subtree walk against the pre-change commit and diffing it against the post-change run: identical — same node set, same NodeIds.

Local test runs

Suite	net10.0	net48	net472
Opc.Ua.SourceGeneration.Core.Tests	3722 passed	3721 passed	—
Opc.Ua.SourceGeneration.Tests	81 passed	81 passed	—
Opc.Ua.SourceGeneration.Stack.Tests	90 passed	—	—
Opc.Ua.Types.Tests	8083 passed	8076 passed	—
Opc.Ua.Di.Tests	303 passed	303 passed	277 passed
Opc.Ua.Core.Tests	4134 passed	—	—
Opc.Ua.Gds.Tests	1095 passed	—	—
Opc.Ua.History.Tests	506 passed	—	—
Opc.Ua.Server.Tests	3626 passed, 1 failed	3574 passed, 1 failed	—

The single Opc.Ua.Server.Tests failure — ServerFluentApiHostingTests.ConfigureApplicationBuildsSharedClientAndServerConfigurationAsync — is pre-existing and unrelated; it reproduces on a stashed clean master tree.

Note

The source-generation test harness compiles generated model code against a hand-written Opc.Ua API stub (tests/Opc.Ua.SourceGeneration.Core.Tests/CompilerUtils.cs, shared by three test projects). Its 73 CreateOrReplace<Child> declarations were updated to the new signature — without that, every generated call site passing assignInstanceNodeIds: false failed to bind. NodesetMethodArgumentTests pinned the exact emitted call and was updated to the new one for the same reason as the NodeStateGeneratorTests expectations.

Note

master has been merged into this branch. The only conflict was in NodeStateGeneratorTests.cs, where master rewrapped the expected call-site strings while this branch appended assignInstanceNodeIds: false to them — kept master's wrapping and re-applied the argument. Master's two new templates (Create_MethodArguments, Assign_MethodArgumentValues) call CreateOrReplace{Input,Output}Arguments while building a declaration subtree, so they now pass assignInstanceNodeIds: false to preserve their stated intent of leaving the property node's identity untouched.

Tip

Changing a source generator leaves dependent projects' cached generated output stale. If a local build reports CS1739: ... does not have a parameter named 'assignInstanceNodeIds' or CS0109 on a generated CreateOrReplace*, run dotnet build <project> -t:Rebuild. Clean CI builds are unaffected.

[Copilot]

Pull request overview

This PR improves runtime instance materialization consistency by ensuring source-generated CreateOrReplace<Child> helpers (used by NodeState.CreateChild/ReplaceChild) can assign per-instance NodeIds and stamp child identity, aligning behavior with CreateInstanceOf<Type> and removing the need for manual re-stamping in node managers (e.g., Pump sample). It also adds tests and updates documentation/migration guidance; plus a small build-hygiene change for CA2000 in the console client sample.

Changes:

• Extend generated CreateOrReplace<Child> helpers with assignInstanceNodeIds (default true) and per-instance NodeId rebasing logic; update generator call sites to opt out in type/declaration factories.
• Add shared runtime helper NodeInstanceExtensions.AssignNewChildInstanceNodeIds and apply it to handwritten MethodState and BaseDataVariableState CreateOrReplace* helpers.
• Update sample + docs/migration guidance to rely on the new NodeId assignment behavior (and add targeted unit tests).

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tools/Opc.Ua.SourceGeneration.Core/Generators/NodeStateTemplates.cs	Updates generated CreateOrReplace<Child> template to stamp identity + optionally assign per-instance NodeIds.
tools/Opc.Ua.SourceGeneration.Core/Generators/NodeStateGenerator.cs	Ensures generator call sites opt out of assignment when building declaration/type subtrees; adds child declaration NodeId constant for comparisons.
src/Opc.Ua.Types/State/NodeInstanceExtensions.cs	Adds shared helper to assign per-instance NodeIds for newly materialized children.
src/Opc.Ua.Types/State/MethodState.cs	Adds opt-in/out parameter and uses shared helper for per-instance NodeId assignment of argument properties.
src/Opc.Ua.Types/State/BaseDataVariableState.cs	Adds opt-in/out parameter and uses shared helper for per-instance NodeId assignment of EnumStrings.
tests/Opc.Ua.Types.Tests/State/NodeInstanceExtensionsTests.cs	Adds coverage for new per-instance NodeId assignment behavior and opt-out/caller-assigned cases.
tests/Opc.Ua.SourceGeneration.Core.Tests/Generators/NodeManagerGeneratorTests.cs	Adds generator-output assertions for default assignment + type-factory opt-out + CreateInstanceOf guard change.
tests/Opc.Ua.SourceGeneration.Core.Tests/Generators/NodeStateGeneratorTests.cs	Updates expected generated call sites to include assignInstanceNodeIds: false.
samples/PumpDeviceIntegrationServer/PumpNodeManager.cs	Removes manual recursive NodeId re-stamping; passes the real parent into CreateInstanceOfPumpType.
samples/ConsoleReferenceClient/ClientSamples.cs	Adds targeted CA2000 suppression around subscription allocation with rationale.
docs/SourceGeneratedNodeManagers.md	Documents runtime instance NodeId assignment behavior and helper families.
docs/migrate/2.0.x/node-states.md	Adds migration note explaining changed CreateChild/ReplaceChild behavior and removal of manual re-stamping.
docs/DeviceIntegration.md	Refreshes narrative to reflect new instance rebasing behavior.

Comments suppressed due to low confidence (1)

tools/Opc.Ua.SourceGeneration.Core/Generators/NodeStateTemplates.cs:1248

• When replacement is already of the target child type (typedReplacement), the code adopts it directly but does not stamp SymbolicName/BrowseName/DisplayName. This undermines the new per-instance NodeId assignment for path-based NodeIdFactory implementations (a bare typed replacement will still collide) and contradicts the stated intent that the helper stamps the new child's identity.

                if (replacement is {{Tokens.ClassName}} typedReplacement)
                {
                    // a replacement of the matching type is used directly,
                    // replacing any child that may already exist.
                    {{Tokens.ChildName}} = typedReplacement;
                }

[codecov]

Codecov Report

❌ Patch coverage is 98.82353% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 79.87%. Comparing base (38c7356) to head (9eb8956).
⚠️ Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
src/Opc.Ua.Types/State/NodeInstanceExtensions.cs	87.50%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #4104       +/-   ##
===========================================
+ Coverage   62.58%   79.87%   +17.29%     
===========================================
  Files        1403     1514      +111     
  Lines      190439   209245    +18806     
  Branches    33322    36008     +2686     
===========================================
+ Hits       119184   167139    +47955     
+ Misses      59788    29568    -30220     
- Partials    11467    12538     +1071     

Files with missing lines	Coverage Δ
src/Opc.Ua.Types/State/BaseDataVariableState.cs	88.75% <100.00%> (+12.43%)	⬆️
src/Opc.Ua.Types/State/MethodState.cs	88.42% <100.00%> (+20.91%)	⬆️
...ceGeneration.Core/Generators/NodeStateGenerator.cs	87.53% <100.00%> (+2.25%)	⬆️
...ceGeneration.Core/Generators/NodeStateTemplates.cs	100.00% <100.00%> (ø)
src/Opc.Ua.Types/State/NodeInstanceExtensions.cs	90.38% <87.50%> (+17.65%)	⬆️

... and 618 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.