From 0993fa5a198f0b3578999f58d94e129f1c423a7e Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 23 May 2026 05:38:37 -0400 Subject: [PATCH] fix(analyzers): resolve Low code-review findings (Analyzers-002,003,004,005,007) - 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) --- code-reviews/Analyzers/findings.md | 22 +- .../UnwrappedCapabilityCallAnalyzer.cs | 234 ++++++++++++----- .../UnwrappedCapabilityCallAnalyzerTests.cs | 243 ++++++++++++++++++ 3 files changed, 420 insertions(+), 79 deletions(-) diff --git a/code-reviews/Analyzers/findings.md b/code-reviews/Analyzers/findings.md index 59eaa85..1ec5dda 100644 --- a/code-reviews/Analyzers/findings.md +++ b/code-reviews/Analyzers/findings.md @@ -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()` 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` / `Dictionary>` 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 `` 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 `` 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 `` 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 `` 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. diff --git a/src/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers/UnwrappedCapabilityCallAnalyzer.cs b/src/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers/UnwrappedCapabilityCallAnalyzer.cs index ddffa50..af964b8 100644 --- a/src/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers/UnwrappedCapabilityCallAnalyzer.cs +++ b/src/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers/UnwrappedCapabilityCallAnalyzer.cs @@ -10,23 +10,56 @@ namespace ZB.MOM.WW.OtOpcUa.Analyzers; /// /// Diagnostic analyzer that flags direct invocations of Phase 6.1-wrapped driver-capability -/// methods when the call is NOT already running inside a CapabilityInvoker.ExecuteAsync, -/// CapabilityInvoker.ExecuteWriteAsync, or AlarmSurfaceInvoker.*Async lambda. -/// The wrapping is what gives us per-host breaker isolation, retry semantics, bulkhead-depth -/// accounting, and alarm-ack idempotence guards — raw calls bypass all of that. +/// methods when the call is NOT already running inside a CapabilityInvoker.ExecuteAsync +/// or CapabilityInvoker.ExecuteWriteAsync lambda. The wrapping is what gives us +/// per-host breaker isolation, retry semantics, bulkhead-depth accounting, and alarm-ack +/// idempotence guards — raw calls bypass all of that. /// /// -/// The analyzer matches by receiver-interface identity using Roslyn's semantic model, not by -/// method name, so a driver with an unusually-named method implementing IReadable.ReadAsync -/// still trips the rule. Lambda-context detection walks up the syntax tree from the call site -/// and checks whether any enclosing InvocationExpressionSyntax targets one of the -/// specific wrapper methods listed in WrapperMethods (type + method name pair). -/// Matching by method name as well as containing type ensures that a future overload on -/// CapabilityInvoker that takes a predicate/selector lambda does not silently widen the -/// suppression scope. The rule is intentionally narrow: it does NOT try to enforce that the -/// capability argument matches the method (e.g. ReadAsync wrapped in -/// ExecuteAsync(DriverCapability.Write, ...) still passes) — that'd require flow -/// analysis beyond single-expression scope. +/// +/// Guarded-call detection uses Roslyn's semantic model: the analyzer compares the invoked +/// method's containing-type symbol (and every implemented interface symbol) against the +/// GuardedInterfaceTypes set resolved once per compilation, then verifies the +/// method either is the interface member or is the concrete implementation discovered via +/// . Matching by symbol +/// identity means a driver with an unusually-named method implementing +/// IReadable.ReadAsync still trips the rule. +/// +/// +/// Default-interface-method handling: IHistoryProvider.ReadAtTimeAsync and +/// IHistoryProvider.ReadEventsAsync ship as DIM bodies. When a driver inherits the +/// DIM (no override), FindImplementationForInterfaceMember returns the interface's +/// own method symbol, which still equals for an interface-typed +/// receiver. When a driver overrides the DIM, the override symbol equals +/// directly. Both paths are covered. +/// +/// +/// Wrapper-lambda detection: the analyzer walks up the syntax tree from the call +/// site and looks for an enclosing InvocationExpressionSyntax bound to +/// CapabilityInvoker.ExecuteAsync / CapabilityInvoker.ExecuteWriteAsync +/// whose argument list contains a lambda whose span contains the call. The match keys on +/// both the containing-type symbol AND the method name — a future overload on +/// CapabilityInvoker that took a non-callSite lambda (predicate, selector, +/// etc.) would still suppress the diagnostic; matching the method name is the strongest +/// containment check the analyzer can make without doing parameter-position binding on +/// every invocation in the IDE hot path. The known limitation is that any lambda +/// argument to ExecuteAsync / ExecuteWriteAsync suppresses the diagnostic +/// even if it is not bound to the callSite parameter — today both overload pairs +/// have exactly one lambda parameter (callSite), so the limitation is theoretical. +/// +/// +/// Why AlarmSurfaceInvoker is NOT a wrapper home: its public methods take +/// IReadOnlyList<...> / IAlarmSubscriptionHandle — no lambda +/// arguments — so no call to it can ever satisfy the lambda-containment check. Calls +/// inside AlarmSurfaceInvoker's own implementation are covered transitively +/// because the surface routes through the inner CapabilityInvoker.ExecuteAsync +/// lambda. +/// +/// +/// The rule does NOT enforce that the capability argument matches the method (e.g. +/// ReadAsync wrapped in ExecuteAsync(DriverCapability.Write, ...) still +/// passes) — that would require flow analysis beyond single-expression scope. +/// /// [DiagnosticAnalyzer(Microsoft.CodeAnalysis.LanguageNames.CSharp)] public sealed class UnwrappedCapabilityCallAnalyzer : DiagnosticAnalyzer @@ -34,7 +67,7 @@ public sealed class UnwrappedCapabilityCallAnalyzer : DiagnosticAnalyzer public const string DiagnosticId = "OTOPCUA0001"; /// Interfaces whose methods must be called through the capability invoker. - private static readonly string[] GuardedInterfaces = + private static readonly string[] GuardedInterfaceMetadataNames = [ "ZB.MOM.WW.OtOpcUa.Core.Abstractions.IReadable", "ZB.MOM.WW.OtOpcUa.Core.Abstractions.IWritable", @@ -46,24 +79,24 @@ public sealed class UnwrappedCapabilityCallAnalyzer : DiagnosticAnalyzer ]; /// - /// Wrapper types paired with the method names that take a resilience callSite lambda. - /// Only a lambda bound to one of these specific methods is treated as a valid wrapper home; - /// other lambdas on the same type (e.g. future predicate/selector overloads) do not suppress - /// the diagnostic. + /// Wrapper-method (containing-type metadata name, method name) pairs. Only a lambda bound + /// to one of these specific methods is treated as a valid wrapper home; other lambdas on + /// the same type (e.g. future predicate/selector overloads) do not suppress the diagnostic + /// unless they take a lambda in an argument position. AlarmSurfaceInvoker is not in + /// this list because none of its public methods accept lambda arguments — calls inside its + /// own implementation are covered transitively by the CapabilityInvoker.ExecuteAsync + /// match. /// - private static readonly (string TypeFqn, string MethodName)[] WrapperMethods = + private static readonly (string TypeMetadataName, string MethodName)[] WrapperMethodKeys = [ ("ZB.MOM.WW.OtOpcUa.Core.Resilience.CapabilityInvoker", "ExecuteAsync"), ("ZB.MOM.WW.OtOpcUa.Core.Resilience.CapabilityInvoker", "ExecuteWriteAsync"), - ("ZB.MOM.WW.OtOpcUa.Core.Resilience.AlarmSurfaceInvoker", "SubscribeAsync"), - ("ZB.MOM.WW.OtOpcUa.Core.Resilience.AlarmSurfaceInvoker", "UnsubscribeAsync"), - ("ZB.MOM.WW.OtOpcUa.Core.Resilience.AlarmSurfaceInvoker", "AcknowledgeAsync"), ]; private static readonly DiagnosticDescriptor Rule = new( id: DiagnosticId, title: "Driver capability call must be wrapped in CapabilityInvoker", - messageFormat: "Call to '{0}' is not wrapped in CapabilityInvoker.ExecuteAsync / ExecuteWriteAsync / AlarmSurfaceInvoker.*. Without the wrapping, Phase 6.1 resilience (retry, breaker, bulkhead, tracker telemetry) is bypassed for this call.", + messageFormat: "Call to '{0}' is not wrapped in CapabilityInvoker.ExecuteAsync / ExecuteWriteAsync. Without the wrapping, Phase 6.1 resilience (retry, breaker, bulkhead, tracker telemetry) is bypassed for this call.", category: "OtOpcUa.Resilience", defaultSeverity: DiagnosticSeverity.Warning, isEnabledByDefault: true, @@ -75,20 +108,63 @@ public sealed class UnwrappedCapabilityCallAnalyzer : DiagnosticAnalyzer { context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); context.EnableConcurrentExecution(); - context.RegisterOperationAction(AnalyzeInvocation, OperationKind.Invocation); + context.RegisterCompilationStartAction(OnCompilationStart); } - private static void AnalyzeInvocation(OperationAnalysisContext context) + private static void OnCompilationStart(CompilationStartAnalysisContext context) { - var invocation = (Microsoft.CodeAnalysis.Operations.IInvocationOperation)context.Operation; + // Resolve the guarded interfaces and wrapper types ONCE per compilation. If none of the + // guarded interfaces are referenced (e.g. an unrelated project building against this + // analyzer package), there's nothing to flag — register no further callbacks so the + // analyzer is a true no-op on cold compilations. + var guardedSet = new HashSet(SymbolEqualityComparer.Default); + foreach (var metadataName in GuardedInterfaceMetadataNames) + { + var sym = context.Compilation.GetTypeByMetadataName(metadataName); + if (sym is not null) guardedSet.Add(sym); + } + if (guardedSet.Count == 0) return; + + // Wrapper methods: build a lookup keyed by (containing-type-symbol, method-name). Matching + // both means a future predicate/selector overload on CapabilityInvoker doesn't silently + // widen the suppression scope. + var wrapperMethodsByType = new Dictionary>(SymbolEqualityComparer.Default); + foreach (var (typeMetadataName, methodName) in WrapperMethodKeys) + { + var typeSym = context.Compilation.GetTypeByMetadataName(typeMetadataName); + if (typeSym is null) continue; + if (!wrapperMethodsByType.TryGetValue(typeSym, out var names)) + { + names = new HashSet(); + wrapperMethodsByType[typeSym] = names; + } + names.Add(methodName); + } + + var state = new AnalyzerState(guardedSet, wrapperMethodsByType); + context.RegisterOperationAction(ctx => AnalyzeInvocation(ctx, state), OperationKind.Invocation); + } + + private static void AnalyzeInvocation(OperationAnalysisContext context, AnalyzerState state) + { + var invocation = (IInvocationOperation)context.Operation; + + // Defensive: a null SemanticModel means the IDE asked for analysis on a half-bound tree. + // We can't tell whether the call is inside a wrapper lambda without it, so SKIP rather + // than report — emitting a diagnostic in that state would be a false positive on code + // that is in fact correctly wrapped. + if (context.Operation.SemanticModel is null) return; + var method = invocation.TargetMethod; + if (method?.ContainingType is null) return; + if (method.ReturnType is null) return; // Narrow the rule to async wire calls. Synchronous accessors like // IHostConnectivityProbe.GetHostStatuses() are pure in-memory snapshots + would never // benefit from the Polly pipeline; flagging them just creates false-positives. if (!IsAsyncReturningType(method.ReturnType)) return; - if (!ImplementsGuardedInterface(method)) return; - if (IsInsideWrapperLambda(invocation.Syntax, context.Operation.SemanticModel, context.CancellationToken)) return; + if (!ImplementsGuardedInterface(method, state.GuardedInterfaces)) return; + if (IsInsideWrapperLambda(invocation.Syntax, context.Operation.SemanticModel, state.WrapperMethodsByType, context.CancellationToken)) return; var diag = Diagnostic.Create(Rule, invocation.Syntax.GetLocation(), $"{method.ContainingType.Name}.{method.Name}"); context.ReportDiagnostic(diag); @@ -103,59 +179,67 @@ public sealed class UnwrappedCapabilityCallAnalyzer : DiagnosticAnalyzer or "global::System.Threading.Tasks.ValueTask"; } - private static bool ImplementsGuardedInterface(IMethodSymbol method) + private static bool ImplementsGuardedInterface(IMethodSymbol method, HashSet guarded) { - foreach (var iface in method.ContainingType.AllInterfaces.Concat(new[] { method.ContainingType })) + // The method may be defined directly on a guarded interface (interface-typed receiver) or + // be a concrete implementation. Walk every interface the containing type implements (plus + // the containing type itself, to catch the interface-typed-receiver case) and look for a + // member whose implementation in the containing type equals `method`, OR a member whose + // original definition equals `method.OriginalDefinition` (covers the DIM inherit case + // where FindImplementationForInterfaceMember returns the interface method itself). + var containingType = method.ContainingType; + foreach (var iface in containingType.AllInterfaces) { - var ifaceFqn = iface.OriginalDefinition.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat) - .Replace("global::", string.Empty); - if (!GuardedInterfaces.Contains(ifaceFqn)) continue; - - foreach (var member in iface.GetMembers().OfType()) - { - var impl = method.ContainingType.FindImplementationForInterfaceMember(member); - if (SymbolEqualityComparer.Default.Equals(impl, method) || - SymbolEqualityComparer.Default.Equals(method.OriginalDefinition, member)) - return true; - } + if (!guarded.Contains(iface.OriginalDefinition)) continue; + if (MethodImplementsInterfaceMember(method, containingType, iface)) return true; + } + // Interface-typed receiver: containingType *is* the guarded interface. + if (guarded.Contains(containingType.OriginalDefinition)) + { + if (MethodImplementsInterfaceMember(method, containingType, containingType)) return true; } return false; } - private static bool IsInsideWrapperLambda(SyntaxNode startNode, SemanticModel? semanticModel, System.Threading.CancellationToken ct) + private static bool MethodImplementsInterfaceMember(IMethodSymbol method, INamedTypeSymbol containingType, INamedTypeSymbol iface) { - if (semanticModel is null) return false; + foreach (var member in iface.GetMembers()) + { + if (member is not IMethodSymbol memberMethod) continue; + var impl = containingType.FindImplementationForInterfaceMember(memberMethod); + if (SymbolEqualityComparer.Default.Equals(impl, method)) return true; + if (SymbolEqualityComparer.Default.Equals(method.OriginalDefinition, memberMethod)) return true; + } + return false; + } + private static bool IsInsideWrapperLambda( + SyntaxNode startNode, + SemanticModel semanticModel, + Dictionary> wrapperMethodsByType, + System.Threading.CancellationToken ct) + { for (var node = startNode.Parent; node is not null; node = node.Parent) { - // We only care about an enclosing invocation — the call we're auditing must literally - // live inside a lambda (ParenthesizedLambda / SimpleLambda / AnonymousMethod) that is - // an argument of a specific CapabilityInvoker.Execute* / AlarmSurfaceInvoker.*Async - // method. Matching both the containing type AND the method name closes the gap where a - // future overload on the same type that takes a predicate/selector lambda would - // otherwise incorrectly suppress the diagnostic. + // The call must literally live inside a lambda (ParenthesizedLambda / SimpleLambda / + // AnonymousMethod) that is an argument of one of the wrapper methods. Match both the + // containing-type symbol AND the method name — a future overload that took a + // predicate/selector lambda on the same type would still suppress the diagnostic IF it + // had the same method name, but a brand-new method (e.g. a hypothetical + // CapabilityInvoker.WithFilterAsync) would not. Today both wrapper method pairs have + // exactly one lambda parameter (the callSite), so the limitation is theoretical. if (node is not InvocationExpressionSyntax outer) continue; var sym = semanticModel.GetSymbolInfo(outer, ct).Symbol as IMethodSymbol; - if (sym is null) continue; + if (sym?.ContainingType is null) continue; - var outerTypeFqn = sym.ContainingType.OriginalDefinition.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat) - .Replace("global::", string.Empty); - var methodName = sym.Name; - var isWrapperMethod = false; - foreach (var (typeFqn, name) in WrapperMethods) - { - if (typeFqn == outerTypeFqn && name == methodName) - { - isWrapperMethod = true; - break; - } - } - if (!isWrapperMethod) continue; + var containingDef = sym.ContainingType.OriginalDefinition; + if (!wrapperMethodsByType.TryGetValue(containingDef, out var methodNames)) continue; + if (!methodNames.Contains(sym.Name)) continue; - // The call is wrapped IFF our startNode is transitively inside one of the outer - // call's argument lambdas. Walk the outer invocation's argument list + check whether - // any lambda body contains the startNode's position. + // The call is wrapped IFF startNode is transitively inside one of the outer call's + // argument lambdas. Walk the outer invocation's argument list + check whether any + // lambda body contains the startNode's position. foreach (var arg in outer.ArgumentList.Arguments) { if (arg.Expression is not AnonymousFunctionExpressionSyntax lambda) continue; @@ -164,4 +248,18 @@ public sealed class UnwrappedCapabilityCallAnalyzer : DiagnosticAnalyzer } return false; } + + private sealed class AnalyzerState + { + public HashSet GuardedInterfaces { get; } + public Dictionary> WrapperMethodsByType { get; } + + public AnalyzerState( + HashSet guardedInterfaces, + Dictionary> wrapperMethodsByType) + { + GuardedInterfaces = guardedInterfaces; + WrapperMethodsByType = wrapperMethodsByType; + } + } } diff --git a/tests/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers.Tests/UnwrappedCapabilityCallAnalyzerTests.cs b/tests/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers.Tests/UnwrappedCapabilityCallAnalyzerTests.cs index 0e2efd2..a2f48fe 100644 --- a/tests/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers.Tests/UnwrappedCapabilityCallAnalyzerTests.cs +++ b/tests/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers.Tests/UnwrappedCapabilityCallAnalyzerTests.cs @@ -51,11 +51,14 @@ namespace ZB.MOM.WW.OtOpcUa.Core.Abstractions { IReadOnlyList GetHostStatuses(); } public class HistoryReadResult { } + public class HistoricalEventsResult { } public interface IHistoryProvider { Task ReadRawAsync(string fullRef, DateTime start, DateTime end, uint max, CancellationToken ct); Task ReadProcessedAsync(string fullRef, DateTime start, DateTime end, TimeSpan interval, CancellationToken ct); Task ReadAtTimeAsync(string fullRef, IReadOnlyList timestamps, CancellationToken ct) => throw new NotSupportedException(); + Task ReadEventsAsync(string? sourceName, DateTime start, DateTime end, int maxEvents, CancellationToken ct) + => throw new NotSupportedException(); } public enum DriverCapability { Read, Write, Discover, AlarmSubscribe, AlarmAcknowledge } } @@ -586,6 +589,246 @@ namespace ZB.MOM.WW.OtOpcUa.Server { diags.ShouldBeEmpty(); } + // ======================================================================= + // Analyzers-002 — AlarmSurfaceInvoker has no lambda parameters; calls inside + // its own method bodies are covered transitively by the CapabilityInvoker + // match because the actual wrapping lambda lives on _invoker.ExecuteAsync. + // ======================================================================= + + [Fact] + public async Task GuardedCall_InsideAlarmSurfaceInvokerMethod_WrappedByCapabilityInvoker_PassesCleanly() + { + // Mirrors the real AlarmSurfaceInvoker pattern: it owns an inner CapabilityInvoker + // and routes IAlarmSource calls through CapabilityInvoker.ExecuteAsync. The transitive + // CapabilityInvoker wrapper match is what suppresses the diagnostic — the analyzer + // does NOT need an AlarmSurfaceInvoker-typed lambda escape hatch (it has no lambda + // params anyway). + const string userSrc = """ +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Core.Resilience; + +namespace ZB.MOM.WW.OtOpcUa.Core.Resilience.Test { + public sealed class FakeAlarmSurface { + private readonly CapabilityInvoker _invoker; + private readonly IAlarmSource _source; + public FakeAlarmSurface(CapabilityInvoker i, IAlarmSource s) { _invoker = i; _source = s; } + public async Task SubscribeAsync(IReadOnlyList ids, CancellationToken ct) { + return await _invoker.ExecuteAsync(DriverCapability.AlarmSubscribe, "h1", + async cct => await _source.SubscribeAlarmsAsync(ids, cct), ct); + } + } +} +"""; + var diags = await Compile(userSrc); + diags.ShouldBeEmpty(); + } + + // ======================================================================= + // Analyzers-003 — false positives must not appear on valid code. We can't + // easily force a null SemanticModel through RegisterOperationAction, but we + // can pin the no-false-positive contract for unrelated invocations. + // ======================================================================= + + [Fact] + public async Task NonGuardedAsyncCall_DoesNotTrip() + { + // Unrelated async call on a non-guarded type — must not be flagged regardless of + // analyzer internals (semantic-model availability, etc.). + const string userSrc = """ +using System.Threading; +using System.Threading.Tasks; + +namespace ZB.MOM.WW.OtOpcUa.Server { + public interface IUnrelated { Task DoStuffAsync(CancellationToken ct); } + public sealed class UnrelatedCaller { + public async Task DoIt(IUnrelated x) { + await x.DoStuffAsync(CancellationToken.None); + } + } +} +"""; + var diags = await Compile(userSrc); + diags.ShouldBeEmpty(); + } + + // ======================================================================= + // Analyzers-004 — when the Core.Abstractions guarded interfaces are not + // referenced at all, the analyzer must be a no-op for the compilation. + // ======================================================================= + + [Fact] + public async Task Compilation_WithoutGuardedInterfaceReferences_EmitsNoDiagnostics() + { + // A standalone compilation that does not pull in the StubSources at all — the + // RegisterCompilationStartAction symbol-resolution fast-path must skip cleanly + // when none of the guarded types exist. + const string userSrc = """ +using System.Threading; +using System.Threading.Tasks; + +namespace SomeOther { + public interface IReadable { Task ReadAsync(CancellationToken ct); } + public sealed class Caller { + public async Task DoIt(IReadable x) { + await x.ReadAsync(CancellationToken.None); + } + } +} +"""; + var diags = await CompileWithoutStubs(userSrc); + diags.ShouldBeEmpty(); + } + + // ======================================================================= + // Analyzers-005 — IHistoryProvider default-interface-method asymmetry + // ======================================================================= + + [Fact] + public async Task Direct_ReadAtTimeAsync_OnConcreteDriverInheritingDIM_TripsDiagnostic() + { + // ConcreteHistoryDriver does NOT override ReadAtTimeAsync (it inherits the DIM). + // The call site has a concrete-receiver type — the analyzer must still flag the call + // because the bound method (ReadAtTimeAsync) is the interface's own default impl. + const string userSrc = """ +using System; +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Server { + public sealed class PartialHistoryDriver : IHistoryProvider { + public Task ReadRawAsync(string r, DateTime s, DateTime e, uint m, CancellationToken ct) => throw null!; + public Task ReadProcessedAsync(string r, DateTime s, DateTime e, TimeSpan i, CancellationToken ct) => throw null!; + // ReadAtTimeAsync + ReadEventsAsync NOT overridden — driver inherits the DIM throws. + } + public sealed class BadPartialHistoryCaller { + public async Task DoIt(PartialHistoryDriver driver) { + // Cast through the interface so the binder resolves to IHistoryProvider.ReadAtTimeAsync + // (the DIM). FindImplementationForInterfaceMember returns the interface method itself + // when nothing overrides it. + var iface = (IHistoryProvider)driver; + _ = await iface.ReadAtTimeAsync("tag", new List(), CancellationToken.None); + } + } +} +"""; + var diags = await Compile(userSrc); + diags.Length.ShouldBe(1); + diags[0].GetMessage().ShouldContain("ReadAtTimeAsync"); + } + + [Fact] + public async Task Direct_ReadEventsAsync_OnConcreteDriverInheritingDIM_TripsDiagnostic() + { + // Same as above but for ReadEventsAsync — confirms both DIMs are caught when the + // driver doesn't override them. + const string userSrc = """ +using System; +using System.Threading; +using System.Threading.Tasks; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Server { + public sealed class PartialHistoryDriver2 : IHistoryProvider { + public Task ReadRawAsync(string r, DateTime s, DateTime e, uint m, CancellationToken ct) => throw null!; + public Task ReadProcessedAsync(string r, DateTime s, DateTime e, TimeSpan i, CancellationToken ct) => throw null!; + } + public sealed class BadPartialHistoryEventsCaller { + public async Task DoIt(PartialHistoryDriver2 driver) { + var iface = (IHistoryProvider)driver; + _ = await iface.ReadEventsAsync(null, DateTime.MinValue, DateTime.MaxValue, 100, CancellationToken.None); + } + } +} +"""; + var diags = await Compile(userSrc); + diags.Length.ShouldBe(1); + diags[0].GetMessage().ShouldContain("ReadEventsAsync"); + } + + [Fact] + public async Task Direct_ReadAtTimeAsync_OnConcreteDriverOverridingDIM_TripsDiagnostic() + { + // Driver explicitly overrides ReadAtTimeAsync — the concrete-receiver call goes through + // the override; FindImplementationForInterfaceMember returns the override method, which + // SymbolEqualityComparer matches against method. + const string userSrc = """ +using System; +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Server { + public sealed class FullHistoryDriver : IHistoryProvider { + public Task ReadRawAsync(string r, DateTime s, DateTime e, uint m, CancellationToken ct) => throw null!; + public Task ReadProcessedAsync(string r, DateTime s, DateTime e, TimeSpan i, CancellationToken ct) => throw null!; + public Task ReadAtTimeAsync(string r, IReadOnlyList ts, CancellationToken ct) => throw null!; + } + public sealed class BadFullHistoryCaller { + public async Task DoIt(FullHistoryDriver driver) { + _ = await driver.ReadAtTimeAsync("tag", new List(), CancellationToken.None); + } + } +} +"""; + var diags = await Compile(userSrc); + diags.Length.ShouldBe(1); + diags[0].GetMessage().ShouldContain("ReadAtTimeAsync"); + } + + [Fact] + public async Task Wrapped_ReadAtTimeAsync_DIM_InsideCapabilityInvokerLambda_PassesCleanly() + { + // DIM wrapped properly — must not trip. + const string userSrc = """ +using System; +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Core.Resilience; + +namespace ZB.MOM.WW.OtOpcUa.Server { + public sealed class GoodHistoryAtTime { + public async Task DoIt(IHistoryProvider provider, CapabilityInvoker invoker) { + _ = await invoker.ExecuteAsync(DriverCapability.Read, "h1", + async ct => await provider.ReadAtTimeAsync("tag", new List(), ct), + CancellationToken.None); + } + } +} +"""; + var diags = await Compile(userSrc); + diags.ShouldBeEmpty(); + } + + private static async Task> CompileWithoutStubs(string userSource) + { + var syntaxTrees = new[] { CSharpSyntaxTree.ParseText(userSource) }; + var references = AppDomain.CurrentDomain.GetAssemblies() + .Where(a => !a.IsDynamic && !string.IsNullOrEmpty(a.Location)) + .Select(a => MetadataReference.CreateFromFile(a.Location)) + .Cast() + .ToList(); + + var compilation = CSharpCompilation.Create( + assemblyName: "AnalyzerTestAssembly_NoStubs", + syntaxTrees: syntaxTrees, + references: references, + options: new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary)); + + var withAnalyzers = compilation.WithAnalyzers( + ImmutableArray.Create(new UnwrappedCapabilityCallAnalyzer())); + + var allDiags = await withAnalyzers.GetAnalyzerDiagnosticsAsync(); + return allDiags.Where(d => d.Id == UnwrappedCapabilityCallAnalyzer.DiagnosticId).ToImmutableArray(); + } + private static async Task> Compile(string userSource) { var syntaxTrees = new[]