From 76c21ffe3f6ce5513ae6d58514cea7560a5717f4 Mon Sep 17 00:00:00 2001 From: Anne Thompson Date: Fri, 11 Aug 2023 13:13:37 -0700 Subject: [PATCH] Add an analyzer to prevent use of some internal shared source types (#6642) * Initial implementation of AZC0020 * Updates * Update * refactor * whitespace * updates * Allow use of shared source types in Azure.Core * pr fb * pr fb * add back change to file missed in merge * Update tests * pr fb; + WIP for local variables * clean up * missed cleanup * missed file * Address warnings * Updates * refactor --- .../AZC0020Tests.cs | 120 +++++++++++++++++ .../AzureAnalyzerVerifier.cs | 19 ++- .../BannedTypesAnalyzer.cs | 127 ++++++++++++++++++ .../Azure.ClientSdk.Analyzers/Descriptors.cs | 9 +- 4 files changed, 270 insertions(+), 5 deletions(-) create mode 100644 src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0020Tests.cs create mode 100644 src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/BannedTypesAnalyzer.cs diff --git a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0020Tests.cs b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0020Tests.cs new file mode 100644 index 000000000..f2e2ad168 --- /dev/null +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0020Tests.cs @@ -0,0 +1,120 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System.Collections.Generic; +using System.Threading.Tasks; +using Xunit; +using Verifier = Azure.ClientSdk.Analyzers.Tests.AzureAnalyzerVerifier; + +namespace Azure.ClientSdk.Analyzers.Tests +{ + public class AZC0020Tests + { + private List<(string fileName, string source)> _sharedSourceFiles; + + public AZC0020Tests() + { + _sharedSourceFiles = new List<(string fileName, string source)>() { + + ("MutableJsonDocument.cs", @" + namespace Azure.Core.Json + { + internal sealed partial class MutableJsonDocument + { + } + } + "), + + ("MutableJsonElement.cs", @" + namespace Azure.Core.Json + { + internal partial struct MutableJsonElement + { + } + } + ") + }; + } + + [Fact] + public async Task AZC0020ProducedForMutableJsonDocumentUsage() + { + string code = @" +using System; +using Azure.Core.Json; + +namespace LibraryNamespace +{ + public class Model + { + private MutableJsonDocument {|AZC0020:_document|}; + internal MutableJsonDocument {|AZC0020:Document|} => {|AZC0020:_document|}; + internal event EventHandler {|AZC0020:_docEvent|}; + + internal MutableJsonDocument {|AZC0020:GetDocument|}(MutableJsonDocument {|AZC0020:value|}) + { + {|AZC0020:MutableJsonDocument mdoc = new MutableJsonDocument();|} + return mdoc; + } + } +}"; + await Verifier.VerifyAnalyzerAsync(code, _sharedSourceFiles); + } + + [Fact] + public async Task AZC0020ProducedForMutableJsonElementUsage() + { + string code = @" +using Azure.Core.Json; + +namespace LibraryNamespace +{ + public class Model + { + private MutableJsonElement {|AZC0020:_element|}; + internal MutableJsonElement {|AZC0020:Element|} => {|AZC0020:_element|}; + + internal MutableJsonElement {|AZC0020:GetDocument|}(MutableJsonElement {|AZC0020:value|}) + { + {|AZC0020:MutableJsonElement element = new MutableJsonElement();|} + return element; + } + } +}"; + await Verifier.VerifyAnalyzerAsync(code, _sharedSourceFiles); + } + + [Fact] + public async Task AZC0020NotProducedForAllowedTypeUsage() + { + string code = @" +using System.Text.Json; + +namespace LibraryNamespace +{ + public class Model + { + JsonElement _element; + } +}"; + await Verifier.VerifyAnalyzerAsync(code, _sharedSourceFiles); + } + + [Fact] + public async Task AZC0020NotProducedForTypeWithBannedNameInAllowedNamespace() + { + string code = @" +namespace LibraryNamespace +{ + public class MutableJsonDocument + { + } + public class Model + { + MutableJsonDocument _document; + } +}"; + await Verifier.VerifyAnalyzerAsync(code, _sharedSourceFiles); + } + } +} diff --git a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AzureAnalyzerVerifier.cs b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AzureAnalyzerVerifier.cs index 12a8d8e85..3992a80d1 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AzureAnalyzerVerifier.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AzureAnalyzerVerifier.cs @@ -1,6 +1,7 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. +// Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +using System.Collections.Generic; using System.Collections.Immutable; using System.Threading; using System.Threading.Tasks; @@ -11,7 +12,7 @@ using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.CodeAnalysis.Testing; using Microsoft.CodeAnalysis.Testing.Verifiers; -namespace Azure.ClientSdk.Analyzers.Tests +namespace Azure.ClientSdk.Analyzers.Tests { public static class AzureAnalyzerVerifier where TAnalyzer : DiagnosticAnalyzer, new() { @@ -19,8 +20,8 @@ namespace Azure.ClientSdk.Analyzers.Tests ReferenceAssemblies.Default.AddPackages(ImmutableArray.Create( new PackageIdentity("Azure.Core", "1.26.0"), new PackageIdentity("Microsoft.Bcl.AsyncInterfaces", "1.1.0"), - new PackageIdentity("System.Text.Json", "4.6.0"), new PackageIdentity("Newtonsoft.Json", "12.0.3"), + new PackageIdentity("System.Text.Json", "4.6.0"), new PackageIdentity("System.Threading.Tasks.Extensions", "4.5.3"))); public static CSharpAnalyzerTest CreateAnalyzer(string source, LanguageVersion languageVersion = LanguageVersion.Latest) @@ -37,7 +38,7 @@ namespace Azure.ClientSdk.Analyzers.Tests TestBehaviors = TestBehaviors.SkipGeneratedCodeCheck }; - public static Task VerifyAnalyzerAsync(string source, LanguageVersion languageVersion = LanguageVersion.Latest) + public static Task VerifyAnalyzerAsync(string source, LanguageVersion languageVersion = LanguageVersion.Latest) => CreateAnalyzer(source, languageVersion).RunAsync(CancellationToken.None); public static Task VerifyAnalyzerAsync(string source, params DiagnosticResult[] diagnostics) @@ -47,6 +48,16 @@ namespace Azure.ClientSdk.Analyzers.Tests return test.RunAsync(CancellationToken.None); } + public static Task VerifyAnalyzerAsync(string source, List<(string fileName, string source)> files) + { + var test = CreateAnalyzer(source); + foreach (var file in files) + { + test.TestState.Sources.Add(file); + } + return test.RunAsync(CancellationToken.None); + } + public static DiagnosticResult Diagnostic(string expectedDescriptor) => AnalyzerVerifier.Diagnostic(expectedDescriptor); } } diff --git a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/BannedTypesAnalyzer.cs b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/BannedTypesAnalyzer.cs new file mode 100644 index 000000000..2c981a34a --- /dev/null +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/BannedTypesAnalyzer.cs @@ -0,0 +1,127 @@ +using System; +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Linq; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; + +namespace Azure.ClientSdk.Analyzers +{ + [DiagnosticAnalyzer(LanguageNames.CSharp)] + public sealed class BannedTypesAnalyzer : DiagnosticAnalyzer + { + private static HashSet BannedTypes = new HashSet() + { + "Azure.Core.Json.MutableJsonDocument", + "Azure.Core.Json.MutableJsonElement", + "Azure.Core.Json.MutableJsonChange", + "Azure.Core.Json.MutableJsonChangeKind", + }; + + private static readonly string BannedTypesMessageArgs = string.Join(", ", BannedTypes); + + public override ImmutableArray SupportedDiagnostics { get; } = ImmutableArray.Create(Descriptors.AZC0020); + + public SymbolKind[] SymbolKinds { get; } = new[] + { + SymbolKind.Event, + SymbolKind.Field, + SymbolKind.Method, + SymbolKind.NamedType, + SymbolKind.Parameter, + SymbolKind.Property, + }; + + public override void Initialize(AnalysisContext context) + { + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.Analyze | GeneratedCodeAnalysisFlags.ReportDiagnostics); + context.EnableConcurrentExecution(); + context.RegisterSymbolAction(c => Analyze(c), SymbolKinds); + context.RegisterSyntaxNodeAction(c => AnalyzeNode(c), SyntaxKind.LocalDeclarationStatement); + } + + public void Analyze(SymbolAnalysisContext context) + { + if (IsAzureCore(context.Symbol.ContainingAssembly)) + { + return; + } + + switch (context.Symbol) + { + case IParameterSymbol parameterSymbol: + CheckType(parameterSymbol.Type, parameterSymbol, context.ReportDiagnostic); + break; + case IMethodSymbol methodSymbol: + CheckType(methodSymbol.ReturnType, methodSymbol, context.ReportDiagnostic); + break; + case IEventSymbol eventSymbol: + CheckType(eventSymbol.Type, eventSymbol, context.ReportDiagnostic); + break; + case IPropertySymbol propertySymbol: + CheckType(propertySymbol.Type, propertySymbol, context.ReportDiagnostic); + break; + case IFieldSymbol fieldSymbol: + CheckType(fieldSymbol.Type, fieldSymbol, context.ReportDiagnostic); + break; + case INamedTypeSymbol namedTypeSymbol: + CheckType(namedTypeSymbol.BaseType, namedTypeSymbol, context.ReportDiagnostic); + foreach (var iface in namedTypeSymbol.Interfaces) + { + CheckType(iface, namedTypeSymbol, context.ReportDiagnostic); + } + break; + } + } + + public void AnalyzeNode(SyntaxNodeAnalysisContext context) + { + if (IsAzureCore(context.ContainingSymbol.ContainingAssembly)) + { + return; + } + + if (context.Node is LocalDeclarationStatementSyntax declaration) + { + ITypeSymbol type = context.SemanticModel.GetTypeInfo(declaration.Declaration.Type).Type; + + CheckType(type, type, context.ReportDiagnostic, context.Node.GetLocation()); + } + } + + private static Diagnostic CheckType(ITypeSymbol type, ISymbol symbol, Action reportDiagnostic, Location location = default) + { + if (type is INamedTypeSymbol namedTypeSymbol) + { + if (IsBannedType(namedTypeSymbol)) + { + reportDiagnostic(Diagnostic.Create(Descriptors.AZC0020, location ?? symbol.Locations.First(), BannedTypesMessageArgs)); + } + + if (namedTypeSymbol.IsGenericType) + { + foreach (var typeArgument in namedTypeSymbol.TypeArguments) + { + CheckType(typeArgument, symbol, reportDiagnostic); + } + } + } + + return null; + } + + private static bool IsAzureCore(IAssemblySymbol assembly) + { + return + assembly.Name.Equals("Azure.Core") || + assembly.Name.Equals("Azure.Core.Experimental"); + } + + private static bool IsBannedType(INamedTypeSymbol namedTypeSymbol) + { + return BannedTypes.Contains($"{namedTypeSymbol.ContainingNamespace}.{namedTypeSymbol.Name}"); + } + } +} diff --git a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/Descriptors.cs b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/Descriptors.cs index ebcd98325..86d9e4d80 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/Descriptors.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/Descriptors.cs @@ -110,9 +110,16 @@ namespace Azure.ClientSdk.Analyzers "Invalid ServiceVersion member name.", "All parts of ServiceVersion members' names must begin with a number or uppercase letter and cannot have consecutive underscores.", "Usage", + DiagnosticSeverity.Warning, true); + + public static DiagnosticDescriptor AZC0020 = new DiagnosticDescriptor( + nameof(AZC0020), + "Avoid using banned types in public APIs", + "The Azure.Core internal shared source types {0} should not be used outside of the Azure.Core library.", + "Usage", DiagnosticSeverity.Warning, true); #endregion - + #region General public static DiagnosticDescriptor AZC0100 = new DiagnosticDescriptor( nameof(AZC0100),