From 0d855cd4c7ccdfb6ec175dd141ec8df59778e3a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Hellander?= Date: Wed, 12 Jul 2023 07:40:00 +0200 Subject: [PATCH] Update SA1121 and SA1404 to detect global using aliases --- .../SA1404CSharp10UnitTests.cs | 45 +++++ .../SA1121CSharp10UnitTests.cs | 25 +++ .../Helpers/SyntaxTreeHelpers.cs | 19 +- .../Lightup/IImportScopeWrapper.cs | 66 +++++++ .../Lightup/SemanticModelExtensions.cs | 181 ++++++++++++++++++ .../Lightup/WrapperHelper.cs | 1 + ...nalysisSuppressionMustHaveJustification.cs | 2 +- .../SA1121UseBuiltInTypeAlias.cs | 2 +- 8 files changed, 333 insertions(+), 8 deletions(-) create mode 100644 StyleCop.Analyzers/StyleCop.Analyzers/Lightup/IImportScopeWrapper.cs create mode 100644 StyleCop.Analyzers/StyleCop.Analyzers/Lightup/SemanticModelExtensions.cs diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp10/MaintainabilityRules/SA1404CSharp10UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp10/MaintainabilityRules/SA1404CSharp10UnitTests.cs index 0deae8537..cf49ae26a 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp10/MaintainabilityRules/SA1404CSharp10UnitTests.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp10/MaintainabilityRules/SA1404CSharp10UnitTests.cs @@ -3,9 +3,54 @@ namespace StyleCop.Analyzers.Test.CSharp10.MaintainabilityRules { + using System.Threading; + using System.Threading.Tasks; + using StyleCop.Analyzers.MaintainabilityRules; using StyleCop.Analyzers.Test.CSharp9.MaintainabilityRules; + using Xunit; + using static StyleCop.Analyzers.Test.Verifiers.StyleCopCodeFixVerifier< + StyleCop.Analyzers.MaintainabilityRules.SA1404CodeAnalysisSuppressionMustHaveJustification, + StyleCop.Analyzers.MaintainabilityRules.SA1404CodeFixProvider>; public class SA1404CSharp10UnitTests : SA1404CSharp9UnitTests { + [Fact] + public async Task TestUsingNameChangeInGlobalUsingInAnotherFileAsync() + { + var testCode1 = @" +global using MySuppressionAttribute = System.Diagnostics.CodeAnalysis.SuppressMessageAttribute;"; + + var testCode2 = @" +public class Foo +{ + [[|MySuppression(null, null)|]] + public void Bar() + { + + } +}"; + + var fixedCode2 = @" +public class Foo +{ + [MySuppression(null, null, Justification = """ + SA1404CodeAnalysisSuppressionMustHaveJustification.JustificationPlaceholder + @""")] + public void Bar() + { + + } +}"; + + await new CSharpTest() + { + TestSources = { testCode1, testCode2 }, + FixedSources = { testCode1, fixedCode2 }, + RemainingDiagnostics = + { + Diagnostic().WithLocation("/0/Test1.cs", 4, 32), + }, + NumberOfIncrementalIterations = 2, + NumberOfFixAllIterations = 2, + }.RunAsync(CancellationToken.None).ConfigureAwait(false); + } } } diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp10/ReadabilityRules/SA1121CSharp10UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp10/ReadabilityRules/SA1121CSharp10UnitTests.cs index e0373a85f..1ee863a71 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp10/ReadabilityRules/SA1121CSharp10UnitTests.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp10/ReadabilityRules/SA1121CSharp10UnitTests.cs @@ -42,5 +42,30 @@ class Bar FixedCode = newSource, }.RunAsync(CancellationToken.None).ConfigureAwait(false); } + + [Fact] + public async Task TestUsingNameChangeInGlobalUsingInAnotherFileAsync() + { + var source1 = @" +global using MyDouble = System.Double;"; + + var oldSource2 = @" +class TestClass +{ + private [|MyDouble|] x; +}"; + + var newSource2 = @" +class TestClass +{ + private double x; +}"; + + await new CSharpTest() + { + TestSources = { source1, oldSource2 }, + FixedSources = { source1, newSource2 }, + }.RunAsync(CancellationToken.None).ConfigureAwait(false); + } } } diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/Helpers/SyntaxTreeHelpers.cs b/StyleCop.Analyzers/StyleCop.Analyzers/Helpers/SyntaxTreeHelpers.cs index c917df7a5..cf3234e7a 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers/Helpers/SyntaxTreeHelpers.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers/Helpers/SyntaxTreeHelpers.cs @@ -72,7 +72,7 @@ public static bool IsWhitespaceOnly(this SyntaxTree tree, CancellationToken canc && TriviaHelper.IndexOfFirstNonWhitespaceTrivia(firstToken.LeadingTrivia) == -1; } - internal static bool ContainsUsingAlias(this SyntaxTree tree, ConcurrentDictionary cache) + internal static bool ContainsUsingAlias(this SyntaxTree tree, ConcurrentDictionary cache, SemanticModel semanticModel, CancellationToken cancellationToken) { if (tree == null) { @@ -85,16 +85,23 @@ internal static bool ContainsUsingAlias(this SyntaxTree tree, ConcurrentDictiona return result; } - bool generated = ContainsUsingAliasNoCache(tree); + bool generated = ContainsUsingAliasNoCache(tree, semanticModel, cancellationToken); cache.TryAdd(tree, generated); return generated; } - private static bool ContainsUsingAliasNoCache(SyntaxTree tree) + private static bool ContainsUsingAliasNoCache(SyntaxTree tree, SemanticModel semanticModel, CancellationToken cancellationToken) { - var nodes = tree.GetRoot().DescendantNodes(node => node.IsKind(SyntaxKind.CompilationUnit) || node.IsKind(SyntaxKind.NamespaceDeclaration) || node.IsKind(SyntaxKindEx.FileScopedNamespaceDeclaration)); - - return nodes.OfType().Any(x => x.Alias != null); + if (SemanticModelExtensions.SupportsGetImportScopes) + { + var scopes = semanticModel.GetImportScopes(0, cancellationToken); + return scopes.Any(x => x.Aliases.Length > 0); + } + else + { + var nodes = tree.GetRoot().DescendantNodes(node => node.IsKind(SyntaxKind.CompilationUnit) || node.IsKind(SyntaxKind.NamespaceDeclaration) || node.IsKind(SyntaxKindEx.FileScopedNamespaceDeclaration)); + return nodes.OfType().Any(x => x.Alias != null); + } } } } diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/Lightup/IImportScopeWrapper.cs b/StyleCop.Analyzers/StyleCop.Analyzers/Lightup/IImportScopeWrapper.cs new file mode 100644 index 000000000..51461dfcf --- /dev/null +++ b/StyleCop.Analyzers/StyleCop.Analyzers/Lightup/IImportScopeWrapper.cs @@ -0,0 +1,66 @@ +// Copyright (c) Tunnel Vision Laboratories, LLC. All Rights Reserved. +// Licensed under the MIT License. See LICENSE in the project root for license information. + +namespace StyleCop.Analyzers.Lightup +{ + using System; + using System.Collections.Immutable; + using Microsoft.CodeAnalysis; + + internal readonly struct IImportScopeWrapper + { + internal const string WrappedTypeName = "Microsoft.CodeAnalysis.IImportScope"; + private static readonly Type WrappedType; + + private static readonly Func> AliasesAccessor; + + private readonly object node; + + static IImportScopeWrapper() + { + WrappedType = WrapperHelper.GetWrappedType(typeof(IImportScopeWrapper)); + + AliasesAccessor = LightupHelpers.CreateSyntaxPropertyAccessor>(WrappedType, nameof(Aliases)); + } + + private IImportScopeWrapper(object node) + { + this.node = node; + } + + public ImmutableArray Aliases + { + get + { + if (this.node == null && WrappedType == null) + { + // Gracefully fall back to a collection with no values + return ImmutableArray.Empty; + } + + return AliasesAccessor(this.node); + } + } + + // NOTE: Referenced via reflection + public static IImportScopeWrapper FromObject(object node) + { + if (node == null) + { + return default; + } + + if (!IsInstance(node)) + { + throw new InvalidCastException($"Cannot cast '{node.GetType().FullName}' to '{WrappedTypeName}'"); + } + + return new IImportScopeWrapper(node); + } + + public static bool IsInstance(object obj) + { + return obj != null && LightupHelpers.CanWrapObject(obj, WrappedType); + } + } +} diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/Lightup/SemanticModelExtensions.cs b/StyleCop.Analyzers/StyleCop.Analyzers/Lightup/SemanticModelExtensions.cs new file mode 100644 index 000000000..b3edc204c --- /dev/null +++ b/StyleCop.Analyzers/StyleCop.Analyzers/Lightup/SemanticModelExtensions.cs @@ -0,0 +1,181 @@ +// Copyright (c) Tunnel Vision Laboratories, LLC. All Rights Reserved. +// Licensed under the MIT License. See LICENSE in the project root for license information. + +namespace StyleCop.Analyzers.Lightup +{ + using System; + using System.Collections.Immutable; + using System.Linq; + using System.Linq.Expressions; + using System.Reflection; + using System.Threading; + using Microsoft.CodeAnalysis; + + internal static class SemanticModelExtensions + { + private static readonly Func> GetImportScopesAccessor; + + static SemanticModelExtensions() + { + CreateGetImportScopesAccessor(out var accessor, out var isSupported); + GetImportScopesAccessor = accessor; + SupportsGetImportScopes = isSupported; + } + + public static bool SupportsGetImportScopes { get; } + + public static ImmutableArray GetImportScopes(this SemanticModel semanticModel, int position, CancellationToken cancellationToken = default) + { + return GetImportScopesAccessor(semanticModel, position, cancellationToken); + } + + private static void CreateGetImportScopesAccessor( + out Func> accessor, + out bool isSupported) + { + // Very error-prone code and potentially brittle regarding future API changes. + // Wrapping everything in a try statement just in case something breaks later on. + try + { + var semanticModelType = typeof(SemanticModel); + + var codeAnalysisWorkspacesAssembly = semanticModelType.GetTypeInfo().Assembly; + var nativeImportScopeType = codeAnalysisWorkspacesAssembly.GetType("Microsoft.CodeAnalysis.IImportScope"); + if (nativeImportScopeType is null) + { + accessor = FallbackAccessor; + isSupported = false; + } + + var methods = semanticModelType.GetTypeInfo().GetDeclaredMethods("GetImportScopes").Where(IsCorrectGetImportScopesMethod); + var method = methods.FirstOrDefault(); + if (method == null) + { + accessor = FallbackAccessor; + isSupported = false; + } + + var importScopeWrapperFromObjectMethod = typeof(IImportScopeWrapper).GetTypeInfo().GetDeclaredMethod("FromObject"); + var nativeImportScopeArrayType = typeof(ImmutableArray<>).MakeGenericType(nativeImportScopeType); + var nativeImportScopeArrayGetItemMethod = nativeImportScopeArrayType.GetTypeInfo().GetDeclaredMethod("get_Item"); + var wrapperImportScopeArrayType = typeof(ImmutableArray); + var wrapperImportScopeArrayBuilderType = typeof(ImmutableArray.Builder); + var wrapperImportScopeArrayBuilderAddMethod = wrapperImportScopeArrayBuilderType.GetTypeInfo().GetDeclaredMethod("Add"); + var wrapperImportScopeArrayBuilderToImmutableMethod = wrapperImportScopeArrayBuilderType.GetTypeInfo().GetDeclaredMethod("ToImmutable"); + var arrayCreateImportScopeBuilderMethod = typeof(ImmutableArray).GetTypeInfo().GetDeclaredMethods("CreateBuilder").Single(IsCorrectCreateBuilderMethod).MakeGenericMethod(typeof(IImportScopeWrapper)); + + var semanticModelParameter = Expression.Parameter(typeof(SemanticModel), "semanticModel"); + var positionParameter = Expression.Parameter(typeof(int), "position"); + var cancellationTokenParameter = Expression.Parameter(typeof(CancellationToken), "cancellationToken"); + + var nativeImportScopesVariable = Expression.Variable(nativeImportScopeArrayType, "nativeImportScopes"); + var nativeImportScopeVariable = Expression.Variable(nativeImportScopeType, "nativeImportScope"); + var countVariable = Expression.Variable(typeof(int), "count"); + var indexVariable = Expression.Variable(typeof(int), "index"); + var wrapperImportScopesBuilderVariable = Expression.Variable(wrapperImportScopeArrayBuilderType, "wrapperImportScopesBuilder"); + var wrapperImportScopesVariable = Expression.Variable(wrapperImportScopeArrayType, "wrapperImoprtScopes"); + var wrapperImportScopeVariable = Expression.Variable(typeof(IImportScopeWrapper), "wrapperImportScope"); + + var breakLabel = Expression.Label("break"); + var block = Expression.Block( + new[] { nativeImportScopesVariable, countVariable, indexVariable, wrapperImportScopesBuilderVariable, wrapperImportScopesVariable }, + //// nativeImportScopes = semanticModel.GetImportScopes(position, cancellationToken); + Expression.Assign( + nativeImportScopesVariable, + Expression.Call( + semanticModelParameter, + method, + new[] { positionParameter, cancellationTokenParameter })), + //// index = 0; + Expression.Assign(indexVariable, Expression.Constant(0)), + //// count = nativeImportScopes.Length; + Expression.Assign( + countVariable, + Expression.Property(nativeImportScopesVariable, "Length")), + //// wrapperImportScopesBuilder = ImmutableArray.CreateBuilder(); + Expression.Assign( + wrapperImportScopesBuilderVariable, + Expression.Call(null, arrayCreateImportScopeBuilderMethod)), + Expression.Loop( + Expression.Block( + new[] { nativeImportScopeVariable, wrapperImportScopeVariable }, + //// if (index >= count) break; + Expression.IfThen( + Expression.GreaterThanOrEqual(indexVariable, countVariable), + Expression.Break(breakLabel)), + //// nativeImportScope = nativeImportScopes[index]; + Expression.Assign( + nativeImportScopeVariable, + Expression.Call( + nativeImportScopesVariable, + nativeImportScopeArrayGetItemMethod, + indexVariable)), + //// wrapperImportScope = IImportScopeWrapper.FromObject(nativeImportScope); + Expression.Assign( + wrapperImportScopeVariable, + Expression.Call( + null, + importScopeWrapperFromObjectMethod, + nativeImportScopeVariable)), + //// wrapperImportScopesBuilder.Add(wrapperImportScope); + Expression.Call( + wrapperImportScopesBuilderVariable, + wrapperImportScopeArrayBuilderAddMethod, + new[] { wrapperImportScopeVariable }), + //// index++; + Expression.PostIncrementAssign(indexVariable)), + breakLabel), + //// wrapperImportScopes = wrapperImportScopesBuilder.ToImmutable(); + Expression.Assign( + wrapperImportScopesVariable, + Expression.Call( + wrapperImportScopesBuilderVariable, + wrapperImportScopeArrayBuilderToImmutableMethod))); + + var lambda = Expression.Lambda>>( + block, + new[] { semanticModelParameter, positionParameter, cancellationTokenParameter }); + + accessor = lambda.Compile(); + isSupported = true; + } + catch (Exception) + { + accessor = FallbackAccessor; + isSupported = false; + } + + // NOTE: At time of implementation, there is no other overload of GetImportScopes, but check in case more are added later on + static bool IsCorrectGetImportScopesMethod(MethodInfo method) + { + var parameters = method.GetParameters(); + if (parameters.Length != 2) + { + return false; + } + + return + parameters[0].ParameterType == typeof(int) && + parameters[1].ParameterType == typeof(CancellationToken); + } + + static bool IsCorrectCreateBuilderMethod(MethodInfo method) + { + var parameters = method.GetParameters(); + return parameters.Length == 0; + } + + static ImmutableArray FallbackAccessor(SemanticModel semanticModel, int position, CancellationToken cancellationToken) + { + if (semanticModel == null) + { + // Unlike an extension method which would throw ArgumentNullException here, the light-up + // behavior needs to match the behavior of the underlying method. + throw new NullReferenceException(); + } + + return ImmutableArray.Empty; + } + } + } +} diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/Lightup/WrapperHelper.cs b/StyleCop.Analyzers/StyleCop.Analyzers/Lightup/WrapperHelper.cs index b7927c89a..7b028de53 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers/Lightup/WrapperHelper.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers/Lightup/WrapperHelper.cs @@ -22,6 +22,7 @@ static WrapperHelper() builder.Add(typeof(AnalyzerConfigOptionsProviderWrapper), codeAnalysisAssembly.GetType(AnalyzerConfigOptionsProviderWrapper.WrappedTypeName)); builder.Add(typeof(AnalyzerConfigOptionsWrapper), codeAnalysisAssembly.GetType(AnalyzerConfigOptionsWrapper.WrappedTypeName)); + builder.Add(typeof(IImportScopeWrapper), codeAnalysisAssembly.GetType(IImportScopeWrapper.WrappedTypeName)); WrappedTypes = builder.ToImmutable(); } diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/MaintainabilityRules/SA1404CodeAnalysisSuppressionMustHaveJustification.cs b/StyleCop.Analyzers/StyleCop.Analyzers/MaintainabilityRules/SA1404CodeAnalysisSuppressionMustHaveJustification.cs index 10b7e65d2..53c002768 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers/MaintainabilityRules/SA1404CodeAnalysisSuppressionMustHaveJustification.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers/MaintainabilityRules/SA1404CodeAnalysisSuppressionMustHaveJustification.cs @@ -96,7 +96,7 @@ public void HandleAttributeNode(SyntaxNodeAnalysisContext context) var attribute = (AttributeSyntax)context.Node; // Return fast if the name doesn't match and the file doesn't contain any using alias directives - if (!attribute.SyntaxTree.ContainsUsingAlias(this.usingAliasCache)) + if (!attribute.SyntaxTree.ContainsUsingAlias(this.usingAliasCache, context.SemanticModel, context.CancellationToken)) { if (!(attribute.Name is SimpleNameSyntax simpleNameSyntax)) { diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/ReadabilityRules/SA1121UseBuiltInTypeAlias.cs b/StyleCop.Analyzers/StyleCop.Analyzers/ReadabilityRules/SA1121UseBuiltInTypeAlias.cs index 8f86c7322..7a297b19f 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers/ReadabilityRules/SA1121UseBuiltInTypeAlias.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers/ReadabilityRules/SA1121UseBuiltInTypeAlias.cs @@ -211,7 +211,7 @@ public void HandleIdentifierNameSyntax(SyntaxNodeAnalysisContext context, StyleC // Most source files will not have any using alias directives. Then we don't have to use semantics // if the identifier name doesn't match the name of a special type if (settings.ReadabilityRules.AllowBuiltInTypeAliases - || !identifierNameSyntax.SyntaxTree.ContainsUsingAlias(this.usingAliasCache)) + || !identifierNameSyntax.SyntaxTree.ContainsUsingAlias(this.usingAliasCache, context.SemanticModel, context.CancellationToken)) { switch (identifierNameSyntax.Identifier.ValueText) {