dohertj2/lmxopcua
1
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(analyzers): resolve Low code-review findings (Analyzers-002,003,004,005,007)

Browse Source 
- Analyzers-002: drop the three dead AlarmSurfaceInvoker entries from
  the wrapper-method allow-list and from the diagnostic message.
- Analyzers-003: bail out of AnalyzeInvocation when the semantic model
  is null (was previously emitting a false positive).
- Analyzers-004: resolve guarded-interface + wrapper-method symbols
  once via CompilationStartAction and compare with SymbolEqualityComparer
  instead of formatting fully-qualified names on every invocation.
- Analyzers-005: add regression tests for default-interface-method
  reads (ReadAtTimeAsync / ReadEventsAsync on a concrete driver), with
  + without an override, and inside a CapabilityInvoker.ExecuteAsync
  lambda.
- Analyzers-007: rewrite the analyzer remarks to accurately describe
  the symbol-identity guarded-call detection, DIM handling, and the
  wrapper-lambda match heuristic.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-23 05:38:37 -04:00
parent 0da4f3b63a
commit 0993fa5a19
3 changed files with 420 additions and 79 deletions
Download Patch File Download Diff File Expand all files Collapse all files

22
code-reviews/Analyzers/findings.md
View File

@@ -7,7 +7,7 @@
| Review date | 2026-05-22 |
| Commit reviewed | `76d35d1` |
| Status | Reviewed |
| Open findings | 5 |
| Open findings | 0 |
## Checklist coverage
@@ -48,13 +48,13 @@
| Severity | Low |
| Category | Correctness & logic bugs |
| Location | `src/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers/UnwrappedCapabilityCallAnalyzer.cs:46-50,130` |
| Status | Open |
| Status | Resolved |
**Description:** `AlarmSurfaceInvoker` is listed in `WrapperTypes`, but `AlarmSurfaceInvoker`'s public methods (`SubscribeAsync`, `UnsubscribeAsync`, `AcknowledgeAsync`) take no lambda arguments at all — callers pass `IReadOnlyList<...>` / `IAlarmSubscriptionHandle`, and the invoker builds the resilience lambdas internally. `IsInsideWrapperLambda` only ever returns `true` when it finds an `AnonymousFunctionExpressionSyntax` argument in the outer call's argument list. Because no `AlarmSurfaceInvoker` call site can have a lambda argument, the `AlarmSurfaceInvoker` entry in `WrapperTypes` is effectively dead — it can never satisfy the suppression condition. Guarded `IAlarmSource` calls written inside `AlarmSurfaceInvoker.cs` are in fact suppressed correctly, but only because they sit inside `CapabilityInvoker.ExecuteAsync` lambdas (the `CapabilityInvoker` entry does the work). The dead entry is misleading and suggests the analyzer recognises an `AlarmSurfaceInvoker` "lambda home" that does not exist.
**Recommendation:** Either remove `AlarmSurfaceInvoker` from `WrapperTypes` (its calls are already covered transitively by the `CapabilityInvoker` match) and update the XML doc, or — if the intent is to allow `IAlarmSource` calls anywhere inside `AlarmSurfaceInvoker` regardless of lambda nesting — add an explicit "call site is lexically within the `AlarmSurfaceInvoker` type declaration" check rather than relying on a lambda-argument scan that never fires.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-23 — Removed the three dead `AlarmSurfaceInvoker` entries from `WrapperMethodKeys` (renamed from `WrapperMethods`); calls inside `AlarmSurfaceInvoker` methods remain correctly suppressed via the transitive `CapabilityInvoker.ExecuteAsync` lambda match. Updated XML docs + diagnostic message to drop the misleading `AlarmSurfaceInvoker.*` reference. Pinned the transitive coverage with regression test `GuardedCall_InsideAlarmSurfaceInvokerMethod_WrappedByCapabilityInvoker_PassesCleanly`.
### Analyzers-003
@@ -63,13 +63,13 @@
| Severity | Low |
| Category | Error handling & resilience |
| Location | `src/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers/UnwrappedCapabilityCallAnalyzer.cs:80,114-116` |
| Status | Open |
| Status | Resolved |
**Description:** `IsInsideWrapperLambda` is passed `context.Operation.SemanticModel` and returns `false` when that model is `null`. A `false` return means "not wrapped", so a null semantic model produces a false-positive diagnostic rather than silently skipping the call. For `RegisterOperationAction` the `SemanticModel` is non-null in normal compilation, so this is low-risk in practice, but the failure mode is the wrong direction — a tooling/IDE edge case where the model is unavailable would flag correct code. Separately, the analyzer has no defensive guard against partially-bound / malformed call sites: `method.ContainingType`, `method.ReturnType`, and `iface.GetMembers()` are dereferenced without null checks. `IInvocationOperation.TargetMethod` is non-null by contract and `ContainingType` is non-null for an ordinary method, so a hard crash is unlikely, but an analyzer that throws on malformed in-progress syntax degrades the IDE experience for the whole solution.
**Recommendation:** When `semanticModel is null` in `AnalyzeInvocation`, return early (skip the call) instead of letting `IsInsideWrapperLambda` report it as unwrapped, so unavailable semantics never produce a false positive.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-23 — `AnalyzeInvocation` now returns early when `context.Operation.SemanticModel is null` instead of reporting the call as unwrapped; added defensive null guards for `TargetMethod`, `ContainingType`, and `ReturnType` so an analyzer crash on partially-bound IDE syntax cannot leak. The `MethodImplementsInterfaceMember` helper iterates members filtered by `is IMethodSymbol` so non-method members never throw on `.GetMembers()` dereferences.
### Analyzers-004
@@ -78,13 +78,13 @@
| Severity | Low |
| Category | Performance & resource management |
| Location | `src/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers/UnwrappedCapabilityCallAnalyzer.cs:95-112` |
| Status | Open |
| Status | Resolved |
**Description:** `ImplementsGuardedInterface` runs on every invocation operation in the compilation (every keystroke in the IDE). For each candidate it allocates via `AllInterfaces.Concat(new[] { method.ContainingType })`, builds a fully-qualified display string per interface and calls `string.Replace("global::", ...)`, then for matching interfaces iterates `iface.GetMembers().OfType<IMethodSymbol>()` calling `FindImplementationForInterfaceMember` per member. The `GuardedInterfaces` / `WrapperTypes` lookups are `string[].Contains` (linear scan) rather than a hash set. None of this is catastrophic — the interface sets are tiny — but the work is repeated for every invocation including the overwhelming majority that target non-guarded methods, and the FQN string formatting plus `Replace` allocation on the hot path is avoidable.
**Recommendation:** Move to `RegisterCompilationStartAction`: resolve the guarded interface and wrapper-type symbols once via `Compilation.GetTypeByMetadataName`, capture them, and compare invocation symbols by `SymbolEqualityComparer` identity. Replace the `string[]` membership checks with a `HashSet`. This also makes the analyzer correctly no-op in compilations that do not reference `Core.Abstractions`.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-23 — Refactored to `RegisterCompilationStartAction` that resolves all guarded-interface and wrapper-method symbols once via `Compilation.GetTypeByMetadataName`, stores them in `HashSet<INamedTypeSymbol>` / `Dictionary<INamedTypeSymbol, HashSet<string>>` using `SymbolEqualityComparer.Default`, then registers the per-invocation action only when guarded types are present (the analyzer is a no-op when none are referenced). The hot path now uses `SymbolEqualityComparer.Default.Equals` instead of FQN-string formatting + `Replace`. Pinned the cold-compilation no-op with regression test `Compilation_WithoutGuardedInterfaceReferences_EmitsNoDiagnostics`.
### Analyzers-005
@@ -93,13 +93,13 @@
| Severity | Low |
| Category | Design-document adherence |
| Location | `src/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers/UnwrappedCapabilityCallAnalyzer.cs:33-43` |
| Status | Open |
| Status | Resolved |
**Description:** `CapabilityInvoker`'s XML doc (`src/Core/.../Resilience/CapabilityInvoker.cs:15-17`) enumerates the routed capability surface as `IReadable`, `IWritable`, `ITagDiscovery`, `ISubscribable`, `IHostConnectivityProbe`, `IAlarmSource`, and all four `IHistoryProvider` reads — matching the analyzer's `GuardedInterfaces` set. However `IHistoryProvider` exposes five async methods, and two of them (`ReadAtTimeAsync`, `ReadEventsAsync`) are C# default-interface-method implementations. When a driver does not override a DIM and a caller invokes it through a concrete driver reference, `FindImplementationForInterfaceMember` returns the interface's own default method symbol; the second equality branch (`method.OriginalDefinition` == `member`) still catches the interface-typed-receiver case, so detection holds — but this DIM interaction is undocumented and untested, and a future driver that overrides one DIM but not the other creates an asymmetric guarded surface that nobody has verified.
**Recommendation:** Add explicit test cases (see Analyzers-006) for `IHistoryProvider` calls via both an interface-typed receiver and a concrete driver that (a) overrides and (b) inherits the default `ReadAtTimeAsync` / `ReadEventsAsync`. If a gap is found, handle DIM members explicitly. Add a short remark to the analyzer XML doc noting the default-interface-method consideration.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-23 — Extended the test stub `IHistoryProvider` with both DIM members (`ReadAtTimeAsync` + `ReadEventsAsync`) and added four regression tests pinning the DIM behaviour for (a) concrete driver inheriting the DIM (`Direct_ReadAtTimeAsync_OnConcreteDriverInheritingDIM_TripsDiagnostic`, `Direct_ReadEventsAsync_OnConcreteDriverInheritingDIM_TripsDiagnostic`), (b) concrete driver overriding the DIM (`Direct_ReadAtTimeAsync_OnConcreteDriverOverridingDIM_TripsDiagnostic`), and (c) DIM call correctly wrapped (`Wrapped_ReadAtTimeAsync_DIM_InsideCapabilityInvokerLambda_PassesCleanly`). Confirmed no gap: both DIM branches (interface-typed receiver routing to `method.OriginalDefinition == member`, concrete-receiver-with-override routing to `FindImplementationForInterfaceMember`) match correctly. Added a dedicated DIM paragraph to the analyzer's XML remarks.
### Analyzers-006
@@ -130,10 +130,10 @@
| Severity | Low |
| Category | Documentation & comments |
| Location | `src/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers/UnwrappedCapabilityCallAnalyzer.cs:21-26` |
| Status | Open |
| Status | Resolved |
**Description:** The `<remarks>` block states the analyzer "matches by receiver-interface identity using Roslyn's semantic model, not by method name". This is accurate for the guarded-call detection (`ImplementsGuardedInterface` uses symbols), but the wrapper detection in `IsInsideWrapperLambda` is described in the same block as walking the syntax tree and checking enclosing invocations by containing type — and that detection is in fact looser than the prose implies (see Analyzers-001): it does not verify the lambda is bound to the resilience `callSite` parameter. The XML doc reads as if the wrapper match is precise. The `<remarks>` also notes the rule does not enforce the capability argument matches the method, but omits the more important current limitation — that a lambda in any argument position of a wrapper-typed call suppresses the diagnostic.
**Recommendation:** Tighten the `<remarks>` to state precisely what `IsInsideWrapperLambda` checks today (textual containment within a lambda argument of a `CapabilityInvoker` / `AlarmSurfaceInvoker`-typed invocation), and note the known limitation that it does not bind the lambda to the `callSite` parameter. Keep the doc in sync if Analyzers-001 is fixed.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-23 — Rewrote the analyzer's `<remarks>` into five precise paragraphs: (1) guarded-call detection uses symbol identity, (2) DIM handling (covers Analyzers-005), (3) wrapper-lambda detection matches both containing-type symbol AND method name, with the lambda-not-bound-to-callSite-parameter limitation called out explicitly, (4) why `AlarmSurfaceInvoker` is not a wrapper home (covers Analyzers-002 narrative), (5) the existing capability-argument-not-enforced caveat. The doc is now in sync with the post-Analyzers-001/-002/-004 implementation.