Source generator option to override instance modelling rules with type definition modelling rules (default opt in for stack)#3799
Merged
Merged
Conversation
…ation) Investigate the user's hypothesis: 'make DataValue a readonly struct'. Rather than a one-shot 150-callsite refactor, this lands an A/B benchmark suite and an additive 'DataValueStruct' sibling so the trade-off is quantified before any migration commitment. Additive types (no production callers yet, opt-in only): - Stack/Opc.Ua.Types/BuiltIn/DataValueStruct.cs — readonly struct counterpart with the same fields and a mutable ref-builder for decoder hot paths (avoids per-field 'with' copies). - BinaryDecoder.ReadDataValueStruct / ReadDataValueStructArray. - BinaryEncoder.WriteDataValueStruct / WriteDataValueStructArray. Wire format is byte-identical to the class-based path (asserted by ClassAndStructProduceIdenticalWireBytes). Benchmark suite (Tests/Opc.Ua.Core.Tests/Types/BuiltIn): - DataValueBenchmarkPayloads.cs — shared fixtures. - DataValueBenchmarks.cs — 14 BenchmarkDotNet scenarios under [MemoryDiagnoser] (decoder hot loop, encoder, 5-frame dispatch, allocate+List, Copy, Equals, full round-trip), parametrised over N = 1/10/100/1000 and Variant shapes (scalar double / scalar string / ArrayOf<double>[1024]). Class and struct variants share the same fixture so the comparison is like-for-like. - 7 NUnit shape-check tests cover the benchmark methods in CI without paying BDN cost. Findings (captured in plans/25-datavalue-struct-vs-class.md): - ~27% allocation reduction in decode and allocate-capture scenarios on scalar payloads, equating to one fewer Gen0 collection per 60-80 ms at 100K-instance/s subscription dispatch. - Struct dispatch chain (5 frames, ScalarDouble, N=1000) is ~32% slower than the class — the predicted 48-byte-per-frame copy cost. - Wall-clock for decode is neutral; allocate-capture pays ~10% in exchange for the GC win. - Array-payload (1 KB+ Variant) shows ~0% delta — the underlying double[] dominates GC. Recommendation (option C — Hybrid): - Keep DataValue (class) as the primary public type. - Ship DataValueStruct as the opt-in API for GC-sensitive hot paths (V2 subscription engine, dispatch fan-out). No migration of legacy callers — wire format unchanged. - Plan #24 (pooled class) remains the orthogonal track for callers that prefer the class surface. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
First step of the DataValue class -> readonly struct migration plan
(see plans/25-datavalue-struct-vs-class.md and the session plan).
Additive only — no existing callers touched.
- SerializableDataValue: regular class with [DataContract] /
[DataMember] attributes carrying the wire schema. Round-trip
helpers (ctor from DataValue, ToDataValue method) preserve all
six fields including pico-second resolution.
- DataValueSurrogateProvider: ISerializationSurrogateProvider
implementation that bridges DataValue <-> SerializableDataValue.
Consumers that need DataContractSerializer interop wire it via
serializer.SetSerializationSurrogateProvider(
DataValueSurrogateProvider.Instance).
- Compatible with all target TFMs (net472, net48, netstandard2.0,
netstandard2.1, net8.0, net9.0, net10.0).
Subsequent steps will remove [DataContract] from DataValue itself
and replace its setters with With<Property>() mutators.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Each property gets a sibling instance method DataValue WithX(value) that returns a new instance with the named field replaced and every other field carried through unchanged. Decoders, NodeState read paths, and any other code that historically mutated DataValue properties after construction will migrate to chained With* calls. While DataValue is still a class, each With* call allocates a new instance — transient cost on the dvstruct branch; eliminated once the type is flipped to a readonly struct in a follow-up step. Tests (+11) cover: - Each With* returns a new instance with the targeted field replaced. - Every other field is carried through unchanged. - The original instance is not mutated (immutability check). - Chained With* calls (default + 6 mutators) produce the expected composite DataValue. - SerializableDataValue round-trip identity. - DataValueSurrogateProvider both directions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replaces 'var v = new DataValue(); v.X = ...;' setter-mutation
patterns in the four decoders with chained With* calls. Wire format
unchanged; all 2871 Types encoder/decoder tests pass.
- BinaryDecoder.ReadDataValue: per-field With* (conditional on
encoding flags).
- XmlDecoder.ReadDataValue: fluent six-step With chain.
- XmlParser.ReadDataValue: same.
- JsonDecoder.ReadDataValue: replaces the collection-initialiser
('new DataValue(...) { SourcePicoseconds = a, ServerPicoseconds =
b }') with .WithSourcePicoseconds().WithServerPicoseconds().
Allocation regression note: each With* call allocates a new DataValue
while the type is still a class — transient ~7x decoder allocation
multiplier until the type is flipped to a readonly struct in a
follow-up step.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…R-2 step 4/N) Replaces 'dv.X = Y;' setter-mutation patterns with chained With* calls. No public-API signature changes in this step — only sites where the mutation is purely local to the method scope. Files migrated: - Subscription/MonitoredItem/MonitoredItem.cs: 2 sites - Diagnostics/DiagnosticsNodeManager.cs: 1 site - NodeManager/MasterNodeManager.cs: 2 sites - NodeManager/MonitoredItem/SamplingGroupMonitoredItemManager.cs: 1 site (chained) - Aggregates/AggregateCalculator.cs: 1 site - Aggregates/MinMaxAggregateCalculator.cs: 10 sites - Aggregates/StartEndAggregateCalculator.cs: 5 sites Build green across all TFMs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…chains (PR-2 step 5/N) Replaces 'value.X = Y;' setter-mutation patterns in the Read service implementation with With* calls. The redundant pre-init setter block (WrappedValue=default, ServerTimestamp/SourceTimestamp=MinValue, StatusCode=Good) is removed entirely since the default DataValue constructor already produces those values. After With-chain rebindings the local 'value' diverges from 'values[ii]' (which still holds the instance ReadAttribute mutated); explicit 'values[ii] = value' commits the final With-chain state back into the results array. Files migrated: - NodeManager/CustomNodeManager.cs: 5 sites - NodeManager/AsyncCustomNodeManager.cs: 5 sites Build green across all TFMs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replaces 'dv.X = Y;' patterns with chained With* calls. Files migrated: - Encoding/PubSubJsonDecoder.cs: 6 sites - Encoding/JsonDataSetMessage.cs: 13 sites (decode + encode paths) - PublishedData/DataCollector.cs: 7 sites (substitute-value + length-constraint paths) Build green across all TFMs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… 7/N) Files migrated: - Applications/Quickstarts.Servers/TestData/HistoryDataReader.cs: 5 sites - Applications/Quickstarts.Servers/TestData/HistoryArchive.cs: 4 sites - Applications/Quickstarts.Servers/SampleNodeManager/SampleNodeManager.cs: 1 site (redundant pre-init removed) - Applications/Quickstarts.Servers/SampleNodeManager/DataChangeMonitoredItem.cs: 3 sites - Applications/ConsoleReferencePublisher/PublishedValuesWrites.cs: 14 sites Build green across all TFMs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Files migrated: - Tests/Opc.Ua.Types.Tests/BuiltIn/DataValueTests.cs: 1 site (renamed WrappedValueGetterAndSetter -> WrappedValueGetterAndWithMutator) - Tests/Opc.Ua.PubSub.Tests/Encoding/MqttJsonNetworkMessageTests.cs: 4 sites Remaining setter mutations in the repo touch only the NodeState family (NodeState.ReadAttribute, BaseVariableState.ReadAttributeAsync, Node.Read) which require signature changes to ref DataValue (sync) / tuple return (async) — that is Step 9 (sync) and the separate PR-3 (async) territory. All BaseVariableState/Tools/NodeStateGenerator hits are 'state.WrappedValue' on BaseVariableState (a different type) and unaffected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…sk<(ServiceResult, DataValue)> (async) PR-2 step 9 + PR-3 combined per design plan. Prerequisite for flipping DataValue from class to readonly struct: the sync ReadAttribute method mutated its DataValue parameter, and the async sibling forwarded it. With value semantics, mutations must rebind via With* and the parameter must either be passed by reference (sync) or returned (async). Foundation: - NodeState.ReadAttribute(... DataValue) -> ref DataValue value (sync) - NodeState.ReadAttributeAsync(...) -> ValueTask<(ServiceResult, DataValue)> with DataValue seed input (async) - NodeState.ReadChildAttribute -> ref DataValue (cascade) - Node.Read / INode.Read -> ref DataValue value - BaseVariableState.ReadAttributeAsync rewritten with locals so the internal mutation chain assembles a single DataValue at the end via With* mutators across async boundaries Callers updated: - Libraries/Opc.Ua.Server: CustomNodeManager, AsyncCustomNodeManager, DiagnosticsNodeManager, MonitoredNode, SamplingGroupMonitoredItemManager - Applications/Quickstarts.Servers: MemoryBufferNodeManager, SampleNodeManager, DataChangeMonitoredItem - Tests: NodeStateTests, NodeTests, VariableTypeNodeTests, BaseVariableStateAsyncHooksTests, TypedBuilderTests, NodeCacheContextTests Build clean across all TFMs. Tests: 7539/7539 Types, 1202/1207 Server (5 skipped), 178/178 WotCon, 27/27 NodeCacheContext (all green). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PR-2 step 11: DataValue properties are now get-only, backed by private readonly fields. The 6-arg constructor DataValue(Variant, StatusCode, DateTimeUtc src, DateTimeUtc srv, ushort srcPico, ushort srvPico) is added so With* mutators and callers can construct fully-populated instances without setters. Changes: - DataValue: remove [DataContract], all [DataMember] attrs, all public setters; properties become expression-bodied get-only (=> m_field) - DataValue: add 6-arg ctor; update With* to use it - SerializableDataValue.ToDataValue: use 6-arg ctor - All external callers (70 files): migrate from property setters and object initializers to constructors, With* chains, or FromStatusCode factory methods Build: 0 errors, 0 warnings (all 7 TFMs) Tests: 7539/7539 Types, 178/178 WotCon (6 DurableMonitoredItem failures are pre-existing, unrelated to setter removal) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
DataValue.Equals now compares all fields (value equality), not references. Three queue tests compared published DataValues that had the overflow bit set on StatusCode against originals without it. Switch these assertions to compare WrappedValue instead of the full DataValue, matching the test intent (value payload). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ncoder/decoder API DataValue.cs: - class -> readonly struct (INullable, IFormattable, IEquatable<DataValue>) - [StructLayout(LayoutKind.Auto)] for safe field ordering - Add IsNull property - Remove ICloneable / virtual Clone / MemberwiseClone (struct can't) - Rewrite Copy() as ctor invocation with Variant.Copy() - Equals(DataValue?) -> Equals(DataValue); operator == / != take DataValue - Static IsGood/IsNotGood/IsUncertain/IsNotUncertain/IsBad/IsNotBad take DataValue (not Nullable<DataValue>) with IsNull short-circuit - m_value now readonly Variant.cs: Equals(DataValue) signature (was Equals(DataValue?)). NodeState.cs / BaseVariableState.cs: remove 'if (value == null) return BadStructureMissing' checks. With a struct, null is impossible; default DataValue is a valid seed for ReadAttribute(Async). Encoder/decoder signatures take/return DataValue (non-nullable): - IEncoder.WriteDataValue(string?, DataValue) — was DataValue? - IDecoder.ReadDataValue / ReadDataValueArray — was DataValue? / ArrayOf<DataValue?> - Binary/Json/Xml encoders + decoders + parser updated - Wire compatibility preserved: WriteDataValue treats value.IsNull the same as the old 'value == null' path (writes null literal / mask=0) TypeInfo.cs / VariantHelper.cs: handle DataValue as value type instead of reference type in default-value + value-extraction switches. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…one, queue/aggregator TryGet patterns) - DataValue struct flip + encoder/decoder API changes (foundation) - NodeState/BaseVariableState: remove null check on ref DataValue param - Aggregate: GetProcessedValue(bool) -> TryGetProcessedValue(bool, out DataValue) - Queue: PeekLastValue/PeekOldestValue -> TryPeekLast/Oldest(out DataValue) - Server MonitoredItem: m_lastValue: DataValue? -> DataValue - DataChangeQueueHandler: m_overflow: DataValue? -> DataValue + m_overflowPending flag - ServerUtils.Event.Value: DataValue? -> DataValue - CustomNodeManager: oldValue: DataValue? -> DataValue (use IsNull check downstream) - ClientBase.ValidateDataValue: null check -> IsNull check - NodeCacheContext: Dictionary<uint, DataValue?> -> Dictionary<uint, DataValue> - Classic MonitoredItem.LastValue: DataValue? -> DataValue - Field.Value: DataValue? -> DataValue Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…terns - AggregateCalculator: full TryGetProcessedValue rewrite; replace DataValue null checks with IsNull on parameters (CompareTimestamps, QueueRawValue, IsGood, GetSimpleBound bounds checks) - MonitoredItem: replace value == null checks with value.IsNull (struct semantics); remove dead 'if (value != null)' wrappers in semantics/ structure changed handling; ApplyFilter / ValueChanged use IsNull - DataChangeQueueHandler.SetOverflowBit: remove dead null check on ref DataValue parameter - MasterNodeManager: values.Add(default) + IsNull check instead of ??= - MonitoredNode: value = default instead of value = null - SamplingGroup: values.Add(default) + IsNull check - StoredMonitoredItem.LastValue: drop null! initializer - ServerUtils: Event.Value initializer = default - AuditEvents: remove ?. null-conditional on DataValue - IAggregateCalculator: clean up duplicate XML comment Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…le Write IUaPubSubDataStore: - ReadPublishedDataItem(NodeId, uint) -> bool TryReadPublishedDataItem(NodeId, uint, out DataValue) - WritePublishedDataItem default param: DataValue? = null -> DataValue = default UaPubSubDataStore: implementation matches new interface; drop .GetValueOrDefault() on DataValue (no longer Nullable) DataCollector: use TryReadPublishedDataItem in PubSub data collection flow Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…PIs to TryGet patterns
- IHistoryDataSource: FirstRaw/NextRaw -> TryFirstRaw/TryNextRaw (out DataValue)
- HistoryFile: same impl
- HistoryDataReader: use TryFirstRaw/TryNextRaw + AddValue(IsNull check)
- HistoryArchive.HistoryEntry.Value: DataValue = null! -> DataValue
- DurableMonitoredItemQueueFactory: drop !-forgiving on decoded DataValue
- UaPubSubDataStoreTests + MessagesHelper: rewrite ReadPublishedDataItem calls
to TryReadPublishedDataItem(out)
- AsyncCustomNodeManagerTests: List<DataValue> { null } -> { default }
- AggregateCalculatorTests (+6 variants): GetProcessedValue -> TryGetProcessedValue
- DurableMonitoredItemTests: PeekLastValue/PeekOldestValue -> TryPeekLastValue/Oldest
- MonitoredItemValueChangedTests: pass default (not null) for DataValue args;
rename ValueNullThrowsArgumentNullException to DefaultValueThrowsArgumentException
- ServerFixtureUtils: SortedDictionary<uint, DataValue> initializers use default
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ilds clean - AggregateCalculator*Tests (6 files): Assert.That(result, Is.Not.Null/Is.Null) -> .IsNull check - AsyncCustomNodeManagerTests: keep reference-type asserts (BaseObjectState, NodeState) - DurableMonitoredItemTests: queue.PeekLastValue/OldestValue with Is.EqualTo -> TryPeek+compare - DurableSubscriptionTest: 'as DataValue' -> explicit (DataValue) cast + InstanceOf check - ManagedSessionReconnectIntegrationTests / TypeSystemClientTest / ClientTest / ReconnectWithSavedSessionSecretsTest: known DataValue variables Assert.That(x, Is.Not.Null) -> Assert.That(x.IsNull, Is.False) - Client.ComplexTypes.Tests.TypeSystemClientTest: same Full UA.slnx builds clean: 0 errors across all 7 TFMs (net472/net48/netstandard2.0/2.1/net8/net9/net10). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
DataValue static helpers (IsGood/IsBad/IsUncertain/...) now just check StatusCode without the IsNull short-circuit. This matches OPC UA spec where status is the only quality indicator. Default DataValue has StatusCode=Good (code 0), so IsGood(default) returns true. Test updates for new struct semantics: - DataValueTests.IsGoodWithNullReturnsFalse -> IsGoodWithDefaultReturnsTrue - DataValueTests.IsBadWithNullReturnsTrue -> IsBadWithDefaultReturnsFalse - IsNotGood/IsNotBad/IsNotUncertain similar renames + expected value flips - DataValueTests.EqualityOperatorLeftNull*/RightNull*: default == default == new() now - NodeStateTests.ReadAttributeReturnsBadForNullDataValue -> ReadAttributeAcceptsDefaultDataValue (default is valid seed) - XmlParserTests.ReadDataValueMissingFieldReturnsDefault assertion fixed Tests: - 7539/7539 Types pass - 1202/1207 Server pass (5 skipped, 0 failed) - 178/178 WotCon pass Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…Struct benchmarks - Delete Stack/Opc.Ua.Types/BuiltIn/DataValueStruct.cs (experimental sibling type from PR-1, no longer needed since DataValue is now a readonly struct) - Delete BinaryEncoder.WriteDataValueStruct + WriteDataValueStructArray transitional shims - Delete BinaryDecoder.ReadDataValueStruct + ReadDataValueStructArray transitional shims - Delete _Struct benchmark variants (Decode/Encode/Dispatch/Allocate/ Copy/Equals/RoundTrip), DispatchStructFrame1-5, BuildStructPayload, m_structValues, DecodeAndEncodeMatchStructRoundTrip, ClassAndStructProduceIdenticalWireBytes from DataValueBenchmarks.cs - Update DataValueBenchmarkPayloads.cs comment to reflect unified type - Update plans/25-datavalue-struct-vs-class.md status header to MIGRATED Build: 0 errors all 7 TFMs. Tests: 7539/7539 Types, 3/3 DataValueBenchmark NUnit entry points pass. PR-5 closes out the 5-PR migration of DataValue from class to readonly struct. Branch dvstruct contains 13 atomic commits (31f7cfb .. now) implementing the complete migration. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…erator template
- Variant.ValueIsDefaultOrNull: special-case DataValue to return false. A
Variant carrying a DataValue value is always meaningful; even a DataValue
with all-default fields encodes as {} rather than being elided. Restores
semantics from the pre-struct era when DataValue did not implement INullable.
- JsonEncoder.WriteDataValue (no-fieldname overload, used inside Variant
contents): emit {} for default DataValue instead of JSON null.
- PubSubJsonEncoder.WriteDataValue: no longer short-circuits when IsNull.
A DataValue with StatusCode.Good is IsNull == true but is a meaningful
payload that must still be emitted.
- TypeSourceGenerator.s_notDefaultChecks: DataValue uses '!X.IsNull' instead
of '!(X is null)' for the struct type.
- UaPubSubDataStoreTests: pass default(DataValue) instead of null to
disambiguate overload resolution against the (Variant, StatusCode?, ...)
overload now that DataValue is non-nullable.
- EncoderTests.EncodeDataValueWithoutValuePropertyTest: move result.IsNull
check into the equality branch; for the picosecond-without-timestamp cases
the round-tripped DataValue is legitimately all-default per spec.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add public static readonly DataValue.Null field (matches NodeId, ExtensionObject, etc. patterns). Use it in TypeInfo.GetDefaultValue and SampleNodeManager.Read instead of new DataValue()/default(DataValue). - Add instance properties IsGood, IsNotGood, IsUncertain, IsNotUncertain, IsBad, IsNotBad on DataValue. The old static methods are removed from DataValue itself and re-introduced as [Obsolete] extension methods on a new DataValueExtensions class so existing call sites still compile. - Update NodeCacheContext + DataValueTests to use the new instance properties instead of the static methods. - McpServer AttributeServiceTools: use ToArrayOf() extension instead of .ToArray() when building ArrayOf<DataValue>. - DataChangeQueueHandler: split multi-statement initialisation lines (m_overflow = default; m_overflowPending = false;) into one per line. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… summary - Reorder DataValue members in canonical order: static Null field, constructors, static factories, properties (IsNull, WrappedValue, StatusCode, timestamps, IsGood/IsBad), methods (With*, Equals, operators, GetHashCode, ToString, Copy), then private instance fields. - Move the multi-line With<Property>() explanation block from above the mutators into the XML <remarks> on the DataValue struct itself. - Update the leading summary 'A class that stores' -> 'A struct that stores' to reflect the type kind. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…t empty)
DataValue.IsNull now distinguishes default(DataValue) from an explicitly
constructed but empty DataValue, addressing reviewer feedback:
'default should create Null datavalue (isNull == true). But an
empty DataValue should be marked appropriately (VariantNull, no
timestamps or picoseconds or status, but still !IsNull). We might
need a sentinal even if it consumes 4 bytes.'
Implementation:
- Add private readonly bool m_set field. All explicit constructors set
it to true. default(DataValue) leaves it false.
- IsNull now returns !m_set; an explicitly constructed empty DataValue
(e.g. new DataValue(Variant.Null)) is !IsNull and round-trips through
encoders as a real but empty DataValue instead of being elided.
- Revert the workaround special-case in Variant.ValueIsDefaultOrNull
for BuiltInType.DataValue — the INullable.IsNull path now returns the
right answer for both default and explicitly-constructed DataValues.
- XmlParser/XmlDecoder/BinaryDecoder.ReadDataValue: when the wire
representation indicates 'absent' (missing field, encoding byte 0)
return DataValue.Null instead of new DataValue().
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ue? wrappers - IMonitoredItem.QueueValue(DataValue, ...) becomes QueueValue(in DataValue, ...). Same for IDataChangeMonitoredItem2. - IDataChangeMonitoredItemQueueHandler.QueueValue(DataValue, ...) and the implementing DataChangeQueueHandler.QueueValue use 'in' too. - MonitoredItem.QueueValue and DataChangeMonitoredItem.QueueValue: 'in' parameter, with a local DataValue copy used for in-method mutation (Copy/With*) since 'in' params cannot be reassigned. - DataChangeMonitoredItem.Publish: 'in DataValue value'; rewrite the semantics-changed / structure-changed branches to use a local plus the new IsNull semantics instead of Nullable<DataValue>.WithStatus(...). - m_lastValue: change DataValue? -> DataValue; assignments to null become DataValue.Null. Reads no longer need '?? default'. DataValue is now itself nullable (IsNull == true for default), so Nullable<DataValue> is redundant and the 'in' modifier avoids copying the 56-byte struct on hot subscription paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds a dedicated #### DataValue section under 'Improved Type safety' that covers: - DataValue is now a readonly struct (cannot compare with null; use IsNull or DataValue.Null). - Property setters were removed; use the With<Property>() fluent mutators. - IsGood/IsBad/etc are instance properties; static helpers moved to an Obsolete extension class (DataValueExtensions). - Nullable<DataValue> is redundant — use DataValue + IsNull instead. - IsNull sentinel semantics: default(DataValue) vs explicitly constructed empty DataValue; decoders return DataValue.Null for absent fields. - Prefer 'in DataValue' for synchronous parameters. - Obsolete object?-returning GetValue helpers; use WrappedValue.TryGetValue. - DataValue.FromStatusCode factories. Includes a before/after code example and a note about the subtle change in IsNull behaviour for code that historically used 'new DataValue()' as a sentinel for 'empty'. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- PooledNotificationDispatchTests: DataValue is now a readonly struct; replace Is.Null with .IsNull, use positional constructor - Docs/perf/PooledNotificationBenchmarks.md: updated with post-merge numbers showing complementary effect of both optimizations: - DataValue struct reduced baseline from 128 KB to 104 KB (-19%) - Pooled path unchanged at 408 B (256x reduction vs new baseline) - Gen0 baseline dropped from 29.75 to 24.17 (-19%) - Pooled Gen0 unchanged at 0.092
The PR-3793 'IPooledEncodeable activator pooling' commit landed on master
with a test that uses the old class-style DataValue object initialiser
({ StatusCode = ..., SourceTimestamp = ... }) and compares Value to null.
Both fail after DataValue became a readonly struct on this branch:
- Use the (Variant, StatusCode, DateTimeUtc) constructor.
- Use .IsNull instead of Is.Null since DataValue is a value type now.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
# Conflicts: # Tests/Opc.Ua.Client.Tests/Subscription/PooledNotificationDispatchTests.cs
…' parameter
Addresses six review threads:
- MigrationGuide.md: remove the NOTE about default vs explicit-empty
IsNull semantics (per reviewer 'Remove this NOTE').
- MonitoredItem.cs: ApplyFilter and the public static ValueChanged now
take 'in DataValue value' and 'in DataValue lastValue'. Avoids copying
the 56-byte struct on the hot subscription filter path.
- JsonDecoder.cs: TryGetDataValueFromElement constructs the result with
the full 6-argument DataValue ctor (value, status, ts, ts, picos, picos)
in one shot instead of building it via three intermediate With* clones.
- XmlDecoder.cs / XmlParser.cs: ReadDataValue does the same — reads each
field into a local first, then builds the final DataValue with the
6-argument ctor in one shot.
- NodeState.cs: ReadAttribute's final DataValue rebind also moves to the
6-argument ctor (was chained .WithSourcePicoseconds.WithServerPicoseconds).
- JsonEncoder.cs: restore the JSON null literal write for the private
WriteDataValue overload when value.IsNull (i.e. for default(DataValue)
appearing as a Variant element / array slot). An explicitly constructed
empty DataValue (IsNull == false, all fields default) still writes '{}'.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Convert DataValue from class to readonly struct for further GC relief
…eration Add configurable UseTypeDefinitionModellingRules option that makes the source generator use the modelling rules from the referenced type definition rather than the overridden rules on instance definitions for structural code generation decisions (child inclusion, optional vs mandatory classification, emission style). - Add TypeDefinitionModellingRule property to HierarchyNode to capture the type definition's original rule before instance overrides - Populate during hierarchy merge; update through type-hierarchy merges (base->derived) but freeze once explicit instance overrides appear - Add GetEffectiveModellingRule() helpers in NodeStateGenerator - Update ~9 structural check sites to use effective modelling rule - Enabled by default for stack generation (StackGeneration.cs) - Disabled by default for Roslyn source generator (configurable via ModelSourceGeneratorUseTypeDefinitionModellingRules MSBuild property) - Runtime ModellingRuleId values in generated code are NOT affected
marcschier
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3799 +/- ##
=======================================
Coverage 73.13% 73.14%
=======================================
Files 720 720
Lines 136593 136647 +54
Branches 22902 22913 +11
=======================================
+ Hits 99898 99945 +47
- Misses 29917 29923 +6
- Partials 6778 6779 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Factory methods (LoadTemplate_ReplaceChild) must use the instance's own modelling rule for the mandatory/optional list classification so that children the instance explicitly marks Mandatory are always created. The effective (type-definition) rule is used only for the type-class structure (fields, properties, optional init). This fixes NullReferenceException during server startup when UseTypeDefinitionModellingRules is enabled.
…downgrade - Runtime ModellingRuleId now uses the effective modelling rule instead of the raw instance rule, so the generated node state reflects the type-definition semantics. - GetEffectiveModellingRule never downgrades a type-definition Mandatory to Optional or any placeholder variant, regardless of whether UseTypeDefinitionModellingRules is enabled.
Replace the simple Mandatory-cannot-be-downgraded check with proper OPC UA standard promotion rules. When not opt-in, instances may only promote the type-definition modelling rule: - Optional -> Mandatory - OptionalPlaceholder -> Mandatory | MandatoryPlaceholder - MandatoryPlaceholder -> Mandatory Any other instance override (demotion) is rejected and the type- definition rule is returned instead. When opt-in, the type-definition rule is always used unconditionally. Extracted into a static IsValidPromotion helper for clarity.
Update XML doc comments on GeneratorOptions, ModelCompilationOptions and the readme to describe the OPC UA modelling rule promotion enforcement and the behavior difference between opt-in and default.
# Conflicts: # Libraries/Opc.Ua.Server/NodeManager/MasterNodeManager.cs
romanett
approved these changes
May 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
When the source generator creates code for "instance" definitions (e.g., the
Serverobject which hasServerTypeas its type definition), it currently uses the modelling rules specified on the instance's children, which may differ from the type definition's children. For example:ServerTypedefinesUrisVersionasModellingRule="Optional"Serverinstance overrides it toModellingRule="Mandatory"This PR introduces two layers of modelling rule enforcement:
1. OPC UA promotion semantics (always active)
Regardless of any configuration, the generator now enforces OPC UA modelling rule promotion semantics. Instances may only promote the type definition's rule:
OptionalMandatoryOptionalPlaceholderMandatory,MandatoryPlaceholderMandatoryPlaceholderMandatoryAny other instance override (demotion) is silently rejected and the type definition's rule is used instead. This applies to both structural code generation decisions and the emitted runtime
ModellingRuleId.2.
UseTypeDefinitionModellingRulesoption (opt-in)When enabled, the generator uses the type definition's modelling rules unconditionally — instance overrides are ignored entirely (even valid promotions). This is useful for stack generation where the type definition's semantics should be preserved exactly.
Opc.Ua.SourceGeneration.Stack)Opc.Ua.SourceGeneration), configurable viaModelSourceGeneratorUseTypeDefinitionModellingRulesMSBuild propertyKey design decisions
TypeDefinitionModellingRuleproperty onHierarchyNodecaptures the type definition's original rule before instance overrides, updated through base→derived type-hierarchy merges but frozen once explicit instance overrides appearGetEffectiveModellingRule()helper withIsValidPromotion()centralizes the rule resolution logic across all structural check sites and runtimeModellingRuleIdemissionLoadTemplate_ReplaceChild) uses the instance's own modelling rule for mandatory/optional list classification so children the instance marks Mandatory are always createdRelated Issues
Types of changes
Checklist
Further comments
Files changed:
Schema/ModelDesign.csTypeDefinitionModellingRuleproperty toHierarchyNodeSchema/ModelDesignValidator.csTypeDefinitionModellingRuleduring hierarchy merge; updates through base→derived type merges, freezes on instance overridesGeneratorOptions.csUseTypeDefinitionModellingRulesoption with full XML docNodeStateGenerator.csGetEffectiveModellingRule()+IsValidPromotion()helpers; updated structural check sites and runtimeModellingRuleIdemission;NodeToGenerateextended withTypeDefinitionModellingRuleStackGeneration.csModelCompilationOptions.csModelCompilation.csGeneratorOptionsOPCFoundation.Opc.Ua.SourceGeneration.propsCompilerVisiblePropertyreadme.md