diff --git a/ZB.MOM.WW.OtOpcUa.slnx b/ZB.MOM.WW.OtOpcUa.slnx index fb6c76e..2c74aa2 100644 --- a/ZB.MOM.WW.OtOpcUa.slnx +++ b/ZB.MOM.WW.OtOpcUa.slnx @@ -18,6 +18,7 @@ + @@ -42,5 +43,6 @@ + diff --git a/src/ZB.MOM.WW.OtOpcUa.Analyzers/AnalyzerReleases.Shipped.md b/src/ZB.MOM.WW.OtOpcUa.Analyzers/AnalyzerReleases.Shipped.md new file mode 100644 index 0000000..36aa555 --- /dev/null +++ b/src/ZB.MOM.WW.OtOpcUa.Analyzers/AnalyzerReleases.Shipped.md @@ -0,0 +1,10 @@ +; Shipped analyzer releases. +; See https://github.com/dotnet/roslyn-analyzers/blob/main/src/Microsoft.CodeAnalysis.Analyzers/ReleaseTrackingAnalyzers.Help.md + +## Release 1.0 + +### New Rules + +Rule ID | Category | Severity | Notes +--------|----------|----------|------- +OTOPCUA0001 | OtOpcUa.Resilience | Warning | Direct driver-capability call bypasses CapabilityInvoker diff --git a/src/ZB.MOM.WW.OtOpcUa.Analyzers/AnalyzerReleases.Unshipped.md b/src/ZB.MOM.WW.OtOpcUa.Analyzers/AnalyzerReleases.Unshipped.md new file mode 100644 index 0000000..d4926e8 --- /dev/null +++ b/src/ZB.MOM.WW.OtOpcUa.Analyzers/AnalyzerReleases.Unshipped.md @@ -0,0 +1,2 @@ +; Unshipped analyzer release. +; See https://github.com/dotnet/roslyn-analyzers/blob/main/src/Microsoft.CodeAnalysis.Analyzers/ReleaseTrackingAnalyzers.Help.md diff --git a/src/ZB.MOM.WW.OtOpcUa.Analyzers/UnwrappedCapabilityCallAnalyzer.cs b/src/ZB.MOM.WW.OtOpcUa.Analyzers/UnwrappedCapabilityCallAnalyzer.cs new file mode 100644 index 0000000..940db71 --- /dev/null +++ b/src/ZB.MOM.WW.OtOpcUa.Analyzers/UnwrappedCapabilityCallAnalyzer.cs @@ -0,0 +1,143 @@ +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Linq; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Operations; + +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. +/// +/// +/// 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 +/// + checks whether any enclosing InvocationExpressionSyntax targets a member whose +/// containing type is CapabilityInvoker or AlarmSurfaceInvoker. The rule is +/// intentionally narrow: it does NOT try to enforce 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. +/// +[DiagnosticAnalyzer(Microsoft.CodeAnalysis.LanguageNames.CSharp)] +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 = + [ + "ZB.MOM.WW.OtOpcUa.Core.Abstractions.IReadable", + "ZB.MOM.WW.OtOpcUa.Core.Abstractions.IWritable", + "ZB.MOM.WW.OtOpcUa.Core.Abstractions.ITagDiscovery", + "ZB.MOM.WW.OtOpcUa.Core.Abstractions.ISubscribable", + "ZB.MOM.WW.OtOpcUa.Core.Abstractions.IHostConnectivityProbe", + "ZB.MOM.WW.OtOpcUa.Core.Abstractions.IAlarmSource", + "ZB.MOM.WW.OtOpcUa.Core.Abstractions.IHistoryProvider", + ]; + + /// Wrapper types whose lambda arguments are the allowed home for guarded calls. + private static readonly string[] WrapperTypes = + [ + "ZB.MOM.WW.OtOpcUa.Core.Resilience.CapabilityInvoker", + "ZB.MOM.WW.OtOpcUa.Core.Resilience.AlarmSurfaceInvoker", + ]; + + 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.", + category: "OtOpcUa.Resilience", + defaultSeverity: DiagnosticSeverity.Warning, + isEnabledByDefault: true, + description: "Phase 6.1 Stream A requires every IReadable/IWritable/ITagDiscovery/ISubscribable/IHostConnectivityProbe/IAlarmSource/IHistoryProvider call to route through the shared Polly pipeline. Direct calls skip the pipeline + lose per-host isolation, retry semantics, and telemetry. If the caller is Core/Server/Driver dispatch code, wrap the call in CapabilityInvoker.ExecuteAsync. If the caller is a unit test invoking the driver directly to test its wire-level behavior, either suppress with a pragma or move the suppression into a NoWarn for the test project."); + + public override ImmutableArray SupportedDiagnostics { get; } = ImmutableArray.Create(Rule); + + public override void Initialize(AnalysisContext context) + { + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.EnableConcurrentExecution(); + context.RegisterOperationAction(AnalyzeInvocation, OperationKind.Invocation); + } + + private static void AnalyzeInvocation(OperationAnalysisContext context) + { + var invocation = (Microsoft.CodeAnalysis.Operations.IInvocationOperation)context.Operation; + var method = invocation.TargetMethod; + + // 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; + + var diag = Diagnostic.Create(Rule, invocation.Syntax.GetLocation(), $"{method.ContainingType.Name}.{method.Name}"); + context.ReportDiagnostic(diag); + } + + private static bool IsAsyncReturningType(ITypeSymbol type) + { + var name = type.OriginalDefinition.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat); + return name is "global::System.Threading.Tasks.Task" + or "global::System.Threading.Tasks.Task" + or "global::System.Threading.Tasks.ValueTask" + or "global::System.Threading.Tasks.ValueTask"; + } + + private static bool ImplementsGuardedInterface(IMethodSymbol method) + { + foreach (var iface in method.ContainingType.AllInterfaces.Concat(new[] { method.ContainingType })) + { + 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; + } + } + return false; + } + + private static bool IsInsideWrapperLambda(SyntaxNode startNode, SemanticModel? semanticModel, System.Threading.CancellationToken ct) + { + if (semanticModel is null) return false; + + 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 CapabilityInvoker.Execute* / AlarmSurfaceInvoker.* call. + if (node is not InvocationExpressionSyntax outer) continue; + + var sym = semanticModel.GetSymbolInfo(outer, ct).Symbol as IMethodSymbol; + if (sym is null) continue; + + var outerTypeFqn = sym.ContainingType.OriginalDefinition.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat) + .Replace("global::", string.Empty); + if (!WrapperTypes.Contains(outerTypeFqn)) 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. + foreach (var arg in outer.ArgumentList.Arguments) + { + if (arg.Expression is not AnonymousFunctionExpressionSyntax lambda) continue; + if (lambda.Span.Contains(startNode.Span)) return true; + } + } + return false; + } +} diff --git a/src/ZB.MOM.WW.OtOpcUa.Analyzers/ZB.MOM.WW.OtOpcUa.Analyzers.csproj b/src/ZB.MOM.WW.OtOpcUa.Analyzers/ZB.MOM.WW.OtOpcUa.Analyzers.csproj new file mode 100644 index 0000000..b875e96 --- /dev/null +++ b/src/ZB.MOM.WW.OtOpcUa.Analyzers/ZB.MOM.WW.OtOpcUa.Analyzers.csproj @@ -0,0 +1,24 @@ + + + + + netstandard2.0 + enable + latest + false + true + true + ZB.MOM.WW.OtOpcUa.Analyzers + + + + + + + + + + + + diff --git a/src/ZB.MOM.WW.OtOpcUa.Server/ZB.MOM.WW.OtOpcUa.Server.csproj b/src/ZB.MOM.WW.OtOpcUa.Server/ZB.MOM.WW.OtOpcUa.Server.csproj index 17f2eee..6014c93 100644 --- a/src/ZB.MOM.WW.OtOpcUa.Server/ZB.MOM.WW.OtOpcUa.Server.csproj +++ b/src/ZB.MOM.WW.OtOpcUa.Server/ZB.MOM.WW.OtOpcUa.Server.csproj @@ -30,6 +30,8 @@ + diff --git a/tests/ZB.MOM.WW.OtOpcUa.Analyzers.Tests/UnwrappedCapabilityCallAnalyzerTests.cs b/tests/ZB.MOM.WW.OtOpcUa.Analyzers.Tests/UnwrappedCapabilityCallAnalyzerTests.cs new file mode 100644 index 0000000..a43dbba --- /dev/null +++ b/tests/ZB.MOM.WW.OtOpcUa.Analyzers.Tests/UnwrappedCapabilityCallAnalyzerTests.cs @@ -0,0 +1,195 @@ +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.Diagnostics; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Analyzers; + +namespace ZB.MOM.WW.OtOpcUa.Analyzers.Tests; + +/// +/// Compile-a-snippet-and-run-the-analyzer tests. Avoids +/// Microsoft.CodeAnalysis.CSharp.Analyzer.Testing.XUnit because it pins to xunit v2 + +/// this project uses xunit.v3 like the rest of the solution. Hand-rolled harness is 15 +/// lines + makes the assertion surface obvious at the test-author level. +/// +[Trait("Category", "Unit")] +public sealed class UnwrappedCapabilityCallAnalyzerTests +{ + /// Minimal stubs for the guarded interfaces + the two wrapper types. Keeps the + /// analyzer tests independent of the real OtOpcUa project references so a drift in those + /// signatures doesn't secretly mute the analyzer check. + private const string StubSources = """ +namespace ZB.MOM.WW.OtOpcUa.Core.Abstractions { + using System.Threading; + using System.Threading.Tasks; + using System.Collections.Generic; + public interface IReadable { + ValueTask> ReadAsync(IReadOnlyList tags, CancellationToken ct); + } + public interface IWritable { + ValueTask WriteAsync(IReadOnlyList ops, CancellationToken ct); + } + public interface ITagDiscovery { + Task DiscoverAsync(CancellationToken ct); + } + public enum DriverCapability { Read, Write, Discover } +} +namespace ZB.MOM.WW.OtOpcUa.Core.Resilience { + using System; + using System.Threading; + using System.Threading.Tasks; + using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + public sealed class CapabilityInvoker { + public ValueTask ExecuteAsync(DriverCapability c, string host, Func> call, CancellationToken ct) => throw null!; + public ValueTask ExecuteAsync(DriverCapability c, string host, Func call, CancellationToken ct) => throw null!; + public ValueTask ExecuteWriteAsync(string host, bool isIdempotent, Func> call, CancellationToken ct) => throw null!; + } +} +"""; + + [Fact] + public async Task Direct_ReadAsync_Call_InServerNamespace_TripsDiagnostic() + { + const string userSrc = """ +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 BadCaller { + public async Task DoIt(IReadable driver) { + var _ = await driver.ReadAsync(new List(), CancellationToken.None); + } + } +} +"""; + var diags = await Compile(userSrc); + diags.Length.ShouldBe(1); + diags[0].Id.ShouldBe(UnwrappedCapabilityCallAnalyzer.DiagnosticId); + diags[0].GetMessage().ShouldContain("ReadAsync"); + } + + [Fact] + public async Task Wrapped_ReadAsync_InsideCapabilityInvokerLambda_PassesCleanly() + { + 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.Server { + public sealed class GoodCaller { + public async Task DoIt(IReadable driver, CapabilityInvoker invoker) { + var _ = await invoker.ExecuteAsync(DriverCapability.Read, "h1", + async ct => await driver.ReadAsync(new List(), ct), CancellationToken.None); + } + } +} +"""; + var diags = await Compile(userSrc); + diags.ShouldBeEmpty(); + } + + [Fact] + public async Task DirectWrite_WithoutWrapper_TripsDiagnostic() + { + const string userSrc = """ +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 BadWrite { + public async Task DoIt(IWritable driver) { + await driver.WriteAsync(new List(), CancellationToken.None); + } + } +} +"""; + var diags = await Compile(userSrc); + diags.Length.ShouldBe(1); + diags[0].GetMessage().ShouldContain("WriteAsync"); + } + + [Fact] + public async Task Discovery_Call_WithoutWrapper_TripsDiagnostic() + { + const string userSrc = """ +using System.Threading; +using System.Threading.Tasks; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Server { + public sealed class BadDiscover { + public async Task DoIt(ITagDiscovery driver) { + await driver.DiscoverAsync(CancellationToken.None); + } + } +} +"""; + var diags = await Compile(userSrc); + diags.Length.ShouldBe(1); + diags[0].GetMessage().ShouldContain("DiscoverAsync"); + } + + [Fact] + public async Task Call_OutsideOfLambda_ButInsideInvokerCall_StillTripsDiagnostic() + { + // Precompute the read *outside* the lambda, then pass the awaited result — that does NOT + // actually wrap the ReadAsync call in the resilience pipeline, so the analyzer must + // still flag it (regression guard: a naive "any mention of ExecuteAsync nearby" rule + // would silently let this pattern through). + 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.Server { + public sealed class SneakyCaller { + public async Task DoIt(IReadable driver, CapabilityInvoker invoker) { + var result = await driver.ReadAsync(new List(), CancellationToken.None); // not inside any lambda + await invoker.ExecuteAsync(DriverCapability.Read, "h1", + async ct => { await Task.Yield(); }, CancellationToken.None); + _ = result; + } + } +} +"""; + var diags = await Compile(userSrc); + diags.Length.ShouldBe(1); + } + + private static async Task> Compile(string userSource) + { + var syntaxTrees = new[] + { + CSharpSyntaxTree.ParseText(StubSources), + 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", + 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(); + } +} diff --git a/tests/ZB.MOM.WW.OtOpcUa.Analyzers.Tests/ZB.MOM.WW.OtOpcUa.Analyzers.Tests.csproj b/tests/ZB.MOM.WW.OtOpcUa.Analyzers.Tests/ZB.MOM.WW.OtOpcUa.Analyzers.Tests.csproj new file mode 100644 index 0000000..6b18cc4 --- /dev/null +++ b/tests/ZB.MOM.WW.OtOpcUa.Analyzers.Tests/ZB.MOM.WW.OtOpcUa.Analyzers.Tests.csproj @@ -0,0 +1,26 @@ + + + + net10.0 + enable + enable + latest + false + true + ZB.MOM.WW.OtOpcUa.Analyzers.Tests + + + + + + + + + + + + + + + +