Commit Graph
4 Commits
Author SHA1 Message Date
romanettandClaude Opus 5.5 9b034f8291 Enable nullable reference types centrally; migrate all unset and annotations-only projects (#4700)
# Description

Nullable reference types are now enabled once, in `common.props`, and
every project in the repository builds warning-clean with them.

**Configuration**
- `<Nullable>enable</Nullable>` lives in `common.props`. All per-project
`<Nullable>` settings are removed: 178 project files (enable,
annotations, and the `disable` in the three Aot test projects).
- 419 per-file `#nullable enable`/`restore` directives and the 4
`#nullable disable` directives are removed.
- Kept on purpose:
- The 5 `<auto-generated>` schema files (`UANodeSet.cs`, `UA Type
Dictionary.cs`, `OPCBinarySchema.cs`, `SecuredApplication.cs`,
`PInvokeHelper.cs`) keep `#nullable enable`. The compiler ignores the
project setting in generated code, so the directive is required there.
- `tests/Opc.Ua.Interop.LegacyPeer` keeps `disable`. It builds against
the 1.5.378 packages and does not import the repository props.
- `docs/DeveloperGuide.md` and `.github/copilot-instructions.md` now
describe the central setting.

**Migration** (the 43 unset and 13 annotations-only projects, roughly
10k diagnostics)
- **Production code: annotated at the declarations.** Covers
`tools/Opc.Ua.SourceGeneration.Core`, `src/Opc.Ua.Lds.Server` and the
LDS sample.
- Members, parameters and returns are nullable where null actually
flows.
- Null-check helpers carry `[NotNullWhen]` (`IsNull`, `IsValidXmlName`,
`IsMethodTypeDesign`).
- `CoreUtils.Format`, `Utils.Format`, `ITemplateWriter.WriteLine` take
`params object?[]`. `Template.AddReplacement` takes `string?` (rendering
already skips null replacements).
- `IRegisteredServerStore.RegisterAsync` returns
`Task<RegistrationEntry?>`: it returns null when a registration is
removed.
- Dereferences backed by an invariant (for example, the validated model)
use `!`, so runtime behaviour is unchanged.
- **Behavioural changes in production code** (small, deliberate):
- `NodeSetToModelDesign` throws `InvalidOperationException` when the
file is not a NodeSet. Before, it threw a NullReferenceException on the
next line.
- `ModelDesignValidator` starts its lookup tables empty instead of null.
A redundant `if (m_nodes == null) return;` guard is gone.
- **Test code: minimal annotations.**
  - `?` on values that can be null, `!` where the test asserts non-null.
  - `= null!` for fixture members assigned in SetUp.
- `TryGetStructure<T>(out T? x)` where the inferred `T?` broke the
`IEncodeable` constraint.
- A few redundant `== null` checks on structs (`NodeId`, `ArrayOf<T>`)
folded into the adjacent `IsNull`/`Count` checks.

**Notes for reviewers**
- The diff is large (~1,500 files) but almost entirely mechanical: `?`,
`!` and directive removal. The non-annotation changes are listed above.
- Known follow-up, not done here:
`IModelDesign.Namespaces/TargetNamespace/NamespaceUris` stay
non-nullable even though the validator returns null before it loads a
dictionary. Making them nullable would ripple through every generator
call site.

## Related Issues

- Fixes #4439

## Checklist

- [ ] I have signed the
[CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf)
and read the
[CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md)
doc.
- [ ] I have added tests that prove my fix is effective or that my
feature works and increased code coverage.
- [x] I have added all necessary documentation.
- [x] 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.

The "no new warnings" box covers net10.0 and net48: full non-incremental
builds of `UA.slnx`, `tools/SourceGeneration.slnx`,
`tools/MigrationAnalyzer.slnx` and the Aot test projects finished with 0
warnings and 0 errors under the repository's warnings-as-errors
settings. net9.0 and net8.0 also had no nullable diagnostics, but they
were not rebuilt after the final fixes. Tests have not been run locally
yet.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-08 08:48:13 +02:00
romanettandClaude Opus 5 105508bcbd Mint runtime NodeIds through a single DefaultNodeIdFactory (#4433)
# Description

Nodes created at runtime now get their NodeIds from a single
`DefaultNodeIdFactory` rather than from a `New` override per
NodeManager.
The factory derives a deterministic identifier from a canonical,
length-prefixed browse path, which is injective where the classic
`{parent}_{child}` concatenation was not — `A_B` + `C` and `A` + `B_C`
both produced `A_B_C` — and it also records the parent's identifier type
and qualifies cross-namespace segments by URI, so identifiers no longer
shift with namespace-table ordering.

A NodeManager selects its style declaratively instead of writing
assignment code:

```csharp
NodeIdFactory = NodeIdFactory.WithMode(NodeIdAssignmentMode.Counter);
```

`NodeIdAssignmentMode` offers `String` (the canonical path verbatim, the
default), `Numeric` / `Guid` / `Opaque` (that path through SHA-256),
`Counter` (sequential, for nodes whose browse paths repeat — per-session
diagnostics, inference jobs, rediscovered assets) and `None`. The
counter
starts above `0x40000000`, out of reach of any authored model, so a
long-running server cannot walk a runtime instance onto a type node.

`New` overrides are gone from every NodeManager the stack ships except
two whose identifiers are a domain scheme rather than an assignment
policy: `FileSystemNodeManager`, whose NodeIds encode the file path they
resolve back to, and `RoboticsNodeManager`, whose build coordinator
reserves identifiers across managers with ownership tracking.

**Namespace rule.** Every identifier is minted into the NodeManager's
own
namespace. Neither the parent's namespace nor the browse name's is
consulted: a parent can belong to a companion-specification model whose
NodeIds are fixed by its NodeSet (`DeviceSet` in the DI namespace), and
a
browse name only names the type that declared the child. NodeManagers
whose instance namespace is not their first one rebase with
`WithDefaultNamespaceIndex`.

**Types vs instances.** An authored NodeId on a node that stands on its
own is kept. A node hanging off a parent is re-minted, because it
reached
the factory through `AssignNodeIds` walking a subtree copied from a type
declaration. Keeping those would alias every instance onto the type's
own
nodes, and the predefined-node index takes the last writer, so the type
would quietly become an instance rather than the clash being reported.

New `ISystemContext.CreateInstance(...)` builds an instance of a
generated
type and rebases its subtree through `AssignInstanceNodeId` — the path
the
generated `CreateInstanceOf<Type>` helpers already took. The AI
NodeManager was the one place calling `NodeState.Create` directly, so
its
roots kept the NodeId their state object is born with (the type's own);
it now uses the shared path.

## Breaking changes

- `IAsyncNodeManager` extends `INodeIdFactory` and gains `AddNode` and
  `AddRootNotifier`. This lets the fluent surface mint and register
  through the interface instead of type-testing for
  `AsyncCustomNodeManager` and silently falling back to a concatenated
  identifier when the test failed — that fallback was itself a source of
  divergent NodeIds.
- `AsyncNodeManagerAdapter` delegates `New` to the wrapped NodeManager,
so
**`CustomNodeManager2` behaves exactly as before**: it mints nothing and
  a node keeps whatever NodeId it already has. Servers built on it are
  unaffected.

## Notes for reviewers

- `docs/NodeIdAssignment.md` is new and describes the whole mechanism:
the
  factory contract, identifier formats, type declarations vs instances,
  the generated helpers and their parameters, per-NodeManager behaviour,
  and an inventory of every member involved.
- The three commits are independently revertible.
- Two pieces that look redundant are deliberately kept, with the
reasoning
  recorded in the doc: `assignInstanceNodeIds` (it *suppresses* minting,
  where the rebase rule *forces* it) and the generator's
  `NodeId.Equals(TypeNodeIdConstant)` guard (it distinguishes a
  caller-assigned NodeId from a declaration's).
- Fluent builder tests previously passed only because of the removed
  fallback; they now run against a real `DefaultNodeIdFactory` through a
  shared `FluentTestNodeManager` helper.

## Related Issues

_No tracking issue yet._

## Checklist

- [ ] I have signed the
[CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf)
and read the
[CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md)
doc.
- [x] I have added tests that prove my fix is effective or that my
feature works and increased code coverage.
- [x] I have added all necessary documentation.
- [x] 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.

> Test runs were on **net10.0 only** — .NET Framework was not exercised
> locally. Suites run green on an idle machine: Server 4950/4955, GDS
> 1116/1170, WotCon 1225/1225, Vision 484/485, DI 424/424, AI 174/174,
> ISA95 136/136, Positioning 69/69, RobotIntent 22/22. One unrelated
> pre-existing failure remains in `Opc.Ua.Types.Tests`
> (`EveryEmbeddedExampleMatchesItsRecordedHash`) — a WoT fixture checked
> out with CRLF, fixed by
> `git add --renormalize tests/Opc.Ua.Types.Tests/Wot/Assets`.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
2026-09-08 14:37:19 +00:00
romanettandClaude Opus 5 3543d7cf6c Report Executable/UserExecutable for state machine causes (#4368)
# Description

A Part 16 cause is a Method, and whether it may fire depends on the
state the machine is in. Part 3 defines `Executable` / `UserExecutable`
as exactly that question, and Part 4 has `Call` refuse a non-executable
Method with `Bad_NotExecutable`. `FiniteStateMachineState` already knows
the answer — `IsCausePermitted(context, causeId, checkUserAccessRights)`
— but nothing outside alarm shelving connected the two. Across the
library, `OnReadExecutable` was assigned in exactly three places, all in
`AlarmConditionState`. Everywhere else a cause method kept the `true`
its node was constructed with, so a client could not tell which causes
currently apply and found out only by calling one and being refused with
`Bad_NotSupported` from `DoCause`.

## What changed

**`StateMachineBuilder.WithCause`** now answers both attributes from
`IsCausePermitted` alongside installing the call handler — on the
unified builder (via the dispatcher's `InstallCauseMethod`) and on the
`Opc.Ua.Server.Fluent` surface reached through
`INodeBuilder.AsStateMachine()`, which had the identical gap.
`Executable` ignores user rights; `UserExecutable` also runs the
machine's `OnCheckUserPermission` callback.

Per the issue's note about servers that deliberately keep methods
executable and answer at call time, both overloads take
`reportExecutable: false` to opt out.

**`ProgramStateMachineState`** wires its own `Start` / `Suspend` /
`Resume` / `Halt` / `Reset` in `OnAfterCreate`, so a Program server no
longer repeats the boilerplate the class already shipped handlers for.
The Boiler quickstart's copy of it is deleted rather than left as an
example.

**Generator-emitted `*StateMachineState` subclasses** are left alone.
The generator emits no Part 16 `StateTable` / `TransitionTable` /
`CauseMappings` overrides — the same gap that makes the DI
software-update dispatcher write `CurrentState` directly instead of
going through `SetState` (see `docs/SoftwareUpdate.md`) — so there is
nothing to derive executability from. A vendor machine from a NodeSet
gets its causes through `StateMachineBuilder.For(...).WithCause(...)`,
which the first change covers. Worth revisiting if the generator gains
table emission.

## Notes for reviewers

Two details behind the `ProgramStateMachineState` change:

- The five methods are `OptionalPlaceholder` on
`ProgramStateMachineType`, so the generated class has no typed
properties for them. They are matched on browse name **without** the
namespace index, because a subtype that redeclares them puts them in its
own namespace — the Boiler model uses `1:Start`, not `0:Start`.
- `NodeState.Create` rebuilds the child list from the type template, so
a method added by hand before it is gone by the time `OnAfterCreate`
runs. `WireCauseMethods(ISystemContext)` is public for that case.

**Behaviour change worth flagging:** a `WithCause` against a machine
whose `CauseMappings` lack that cause now reports the method
non-executable where it previously reported executable. Such a call was
already failing with `Bad_NotSupported` inside `DoCause`, so it fails
earlier with the code Part 4 specifies rather than newly failing — but
it is visible to clients that read the attribute.

## Related Issues

- Fixes #4363

## Checklist

- [x] I have signed the
[CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf)
and read the
[CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md)
doc.
- [x] I have added tests that prove my fix is effective or that my
feature works and increased code coverage.
- [x] I have added all necessary documentation.
- [x] 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

New: `StateMachineBuilderCauseExecutableTests` (definition mode, the
opt-out, `OnCheckUserPermission` affecting `UserExecutable` only,
suspended machines) and `ProgramStateMachineStateTests`, plus two cases
added to the existing fluent-builder fixture. All read through
`MethodState.ReadAttribute`, so a handler that is installed but never
consulted fails the test.

Run locally on **net10.0**: `Opc.Ua.Server.Tests` (4797),
`Opc.Ua.Core.Tests` (4374), `Opc.Ua.Di.Tests`, `Opc.Ua.Features.Tests`,
`Opc.Ua.Robotics.Intent.Tests` and the Boiler AOT tests all pass;
`UA.slnx` builds with no new warnings. The .NET Framework legs were not
run locally — leaving that checklist box to CI.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
2026-09-01 18:31:36 +02:00
Marc Schier 7cec2d614d Move repository files to the consolidated layout 2026-07-16 06:25:54 +02:00