Make NodeId assignment during a node copy explicit

[marcschier]

Fixes #4137.

The problem

NodeState.Initialize(ISystemContext, NodeState) copies a node. Every child it materialises through CreateChild gets a per-instance NodeId from ISystemContext.NodeIdFactory, and is then initialised from its source on the very next statement - which overwrites that NodeId. The assignment is therefore always discarded: it only burns identifiers, and permanently leaks them for factories that track outstanding allocations. Booting the Robotics node manager left ten reservations behind that every later allocation skipped.

#4123 fixed the symptom by hiding the factory for the duration of the copy, wrapping the context in one that reports NodeIdFactory as null. The review objection was fair: a context that lies about one property is hard to reason about, and it only works because that property is allowed to be null.

What this does

States the intent instead of faking the environment. NodeId assignment is now an argument of the two members that materialise children:

public virtual BaseInstanceState? CreateChild(
    ISystemContext context,
    QualifiedName browseName,
    bool assignInstanceNodeIds = true);

protected virtual BaseInstanceState? FindChild(
    ISystemContext context,
    QualifiedName browseName,
    bool createOrReplace,
    BaseInstanceState? replacement,
    bool assignInstanceNodeIds = true);

A copy passes assignInstanceNodeIds: false and every node type - generated or hand-written - sees the real context. There is no capability property, no wrapper, and no second overload to keep in sync.

MethodState, BaseDataVariableState and the source generator template each collapse to a single FindChild override that threads the argument into its CreateOrReplace<Child> helpers.

Breaking change

The four argument FindChild and the two argument CreateChild virtuals are gone.

• Call sites are unaffected. The new parameter defaults to true, so CreateChild(context, browseName) and FindChild(context, browseName, true, null) keep compiling and keep the 1.5.378 behaviour.
• Overrides must add the parameter. A node manager that overrides either member gets CS0115 until it does. The compiler points at every site; docs/migrate/2.0.x/node-states.md shows the before/after.

This is deliberate, per review: the stack ships all base types, and types generated from a model are regenerated for 2.0 anyway.

Also fixed

MethodState.FindChild returned OutputArguments when asked for InputArguments with createOrReplace: false - a pre-existing bug in the method being restructured. Covered by FindChildReturnsTheRequestedArgumentsProperty.

Validation

• Full dotnet build UA.slnx: 0 errors, 0 warnings.
• Opc.Ua.Types.Tests, Opc.Ua.SourceGeneration.Core.Tests, Opc.Ua.SourceGeneration.Tests, Opc.Ua.Di.Tests, Opc.Ua.Robotics.Tests - all passing.
• Each new regression test was checked to fail without its corresponding fix.

[Copilot]

Pull request overview

This PR fixes NodeId “leak/burn” behavior during NodeState copies by making instance NodeId assignment an explicit opt-in/opt-out during child materialization, while preserving compatibility for legacy overrides that can’t be told about the new intent flag.

Changes:

• Added FindChild(..., bool assignInstanceNodeIds) and CreateChild(..., bool assignInstanceNodeIds) to allow callers (notably copies) to decline per-instance NodeId assignment explicitly.
• Added SupportsInstanceNodeIdAssignmentControl and updated generated types plus MethodState / BaseDataVariableState to honor the opt-out without lying about the context (while retaining the wrapper fallback at the legacy override boundary).
• Added regression tests covering “copy consumes no NodeIds” across modern, legacy, and derived-legacy override scenarios; also fixed a pre-existing MethodState.FindChild bug (InputArguments vs OutputArguments).

Reviewed changes

Copilot reviewed 7 out of 7 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 node templates to opt into assignment-control and thread the flag through child creation paths.
src/Opc.Ua.Types/State/NodeState.cs	Introduces assignment-control overloads and the opt-in capability flag; updates copy path to decline assignment.
src/Opc.Ua.Types/State/MethodState.cs	Opts into assignment-control, adds the new overload, and fixes InputArguments resolution bug.
src/Opc.Ua.Types/State/BaseDataVariableState.cs	Opts into assignment-control and threads the flag through EnumStrings child creation.
src/Opc.Ua.Types/State/NodeIdFactorySuppressedContext.cs	Refines documentation: wrapper remains as a compatibility fallback for legacy override shapes.
tests/Opc.Ua.Types.Tests/State/NodeInstanceExtensionsTests.cs	Adds regression coverage for copy behavior (no NodeIdFactory consumption) and for legacy/derived override edge cases.

[romanett]

IMO we should do a breaking change here, to have this cleanly resolved, and also a better more explicit NodeState.CreateChild interface

[marcschier]

Took the breaking change - pushed in 26b572c. The two overloads, the capability property and the context wrapper are all gone; NodeState now has one virtual per operation and the intent is an argument:

public virtual BaseInstanceState? CreateChild(
    ISystemContext context,
    QualifiedName browseName,
    bool assignInstanceNodeIds = true);

protected virtual BaseInstanceState? FindChild(
    ISystemContext context,
    QualifiedName browseName,
    bool createOrReplace,
    BaseInstanceState? replacement,
    bool assignInstanceNodeIds = true);

What that buys:

• Callers are unaffected. The default is true, so CreateChild(context, browseName) and FindChild(context, browseName, true, null) keep compiling with the 1.5.378 behaviour.
• Overrides adapt once. A node manager overriding either member gets CS0115 until it adds the parameter and passes it on - the compiler points at every site.
• No more lying context. NodeIdFactorySuppressedContext is deleted; every type sees the real ISystemContext during a copy and is simply asked not to assign.
• No more capability flag. SupportsInstanceNodeIdAssignmentControl is deleted - with one virtual there is no override shape left that cannot be told.
• No more FindDeclaredChild helpers. They only existed to keep the two overrides from recursing; MethodState, BaseDataVariableState and the generator template each collapse to one override.

Migration is documented in docs/migrate/2.0.x/node-states.md (before/after) and docs/NodeManagers.md. Validation: full dotnet build UA.slnx 0 errors / 0 warnings; Opc.Ua.Types.Tests 8097 (net10.0) and 8090 (net48), Opc.Ua.SourceGeneration.Core.Tests 3749, Opc.Ua.SourceGeneration.Tests 94, Opc.Ua.Di.Tests 364, Opc.Ua.Robotics.Tests 103 - all green. The copy regression tests were re-checked to fail (4 of them) when the copy stops declining assignment.

Changes applied, re-requested review