From 7ffc5771b82075802b2dc9a0cdd0ec76dd3216f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Hellander?= Date: Sun, 17 Dec 2023 15:50:46 +0100 Subject: [PATCH] Update SA1121 and SA1404 to detect global using aliases #3594 --- .../IImportScopeWrapperCSharp11UnitTests.cs | 54 ++++++ .../SA1404CSharp11UnitTests.cs | 49 ++++++ .../SA1121CSharp11UnitTests.cs | 34 ++++ .../IImportScopeWrapperCSharp12UnitTests.cs | 9 + .../Helpers/SyntaxTreeHelpers.cs | 20 ++- .../Lightup/IImportScopeWrapper.cs | 54 ++++++ .../Lightup/SemanticModelExtensions.cs | 162 ++++++++++++++++++ .../Lightup/WrapperHelper.cs | 1 + ...nalysisSuppressionMustHaveJustification.cs | 2 +- .../SA1121UseBuiltInTypeAlias.cs | 2 +- 10 files changed, 381 insertions(+), 6 deletions(-) create mode 100644 StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp11/Lightup/IImportScopeWrapperCSharp11UnitTests.cs create mode 100644 StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp12/Lightup/IImportScopeWrapperCSharp12UnitTests.cs 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.CSharp11/Lightup/IImportScopeWrapperCSharp11UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp11/Lightup/IImportScopeWrapperCSharp11UnitTests.cs new file mode 100644 index 000000000..741fbf8e7 --- /dev/null +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp11/Lightup/IImportScopeWrapperCSharp11UnitTests.cs @@ -0,0 +1,54 @@ +// Copyright (c) Tunnel Vision Laboratories, LLC. All Rights Reserved. +// Licensed under the MIT License. See LICENSE in the project root for license information. + +// Tests calls where nullability rules are not obeyed +#pragma warning disable CS8604 // Possible null reference argument. + +namespace StyleCop.Analyzers.Test.Lightup +{ + using System; + using System.Collections.Immutable; + using Microsoft.CodeAnalysis; + using StyleCop.Analyzers.Lightup; + using Xunit; + + public class IImportScopeWrapperCSharp11UnitTests + { + [Fact] + public void TestNull() + { + object? obj = null; + Assert.False(IImportScopeWrapper.IsInstance(obj)); + var wrapper = IImportScopeWrapper.FromObject(obj); + Assert.Throws(() => wrapper.Aliases); + } + + [Fact] + public void TestIncompatibleInstance() + { + var obj = new object(); + Assert.False(IImportScopeWrapper.IsInstance(obj)); + Assert.Throws(() => IImportScopeWrapper.FromObject(obj)); + } + + [Fact] + public void TestCompatibleInstance() + { + var obj = new TestImportScope(); + Assert.True(IImportScopeWrapper.IsInstance(obj)); + var wrapper = IImportScopeWrapper.FromObject(obj); + Assert.Empty(wrapper.Aliases); + } + + private class TestImportScope : IImportScope + { + public ImmutableArray Aliases => ImmutableArray.Empty; + + public ImmutableArray ExternAliases => throw new NotImplementedException(); + + public ImmutableArray Imports => throw new NotImplementedException(); + + public ImmutableArray XmlNamespaces => throw new NotImplementedException(); + } + } +} diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp11/MaintainabilityRules/SA1404CSharp11UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp11/MaintainabilityRules/SA1404CSharp11UnitTests.cs index 0f7019680..b1dd8d863 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp11/MaintainabilityRules/SA1404CSharp11UnitTests.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp11/MaintainabilityRules/SA1404CSharp11UnitTests.cs @@ -3,9 +3,58 @@ namespace StyleCop.Analyzers.Test.CSharp11.MaintainabilityRules { + using System.Threading; + using System.Threading.Tasks; + using StyleCop.Analyzers.MaintainabilityRules; using StyleCop.Analyzers.Test.CSharp10.MaintainabilityRules; + using Xunit; + + using static StyleCop.Analyzers.Test.Verifiers.StyleCopCodeFixVerifier< + StyleCop.Analyzers.MaintainabilityRules.SA1404CodeAnalysisSuppressionMustHaveJustification, + StyleCop.Analyzers.MaintainabilityRules.SA1404CodeFixProvider>; public partial class SA1404CSharp11UnitTests : SA1404CSharp10UnitTests { + // NOTE: This tests a fix for a c# 10 feature, but the Roslyn API used to solve it wasn't available in the version + // we use in the c# 10 test project, so the test was added here instead. + [Fact] + [WorkItem(3594, "https://github.com/DotNetAnalyzers/StyleCopAnalyzers/issues/3594")] + 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.CSharp11/ReadabilityRules/SA1121CSharp11UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp11/ReadabilityRules/SA1121CSharp11UnitTests.cs index 118e24fb8..6988b642d 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp11/ReadabilityRules/SA1121CSharp11UnitTests.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp11/ReadabilityRules/SA1121CSharp11UnitTests.cs @@ -3,9 +3,43 @@ namespace StyleCop.Analyzers.Test.CSharp11.ReadabilityRules { + using System.Threading; + using System.Threading.Tasks; using StyleCop.Analyzers.Test.CSharp10.ReadabilityRules; + using Xunit; + + using static StyleCop.Analyzers.Test.Verifiers.StyleCopCodeFixVerifier< + StyleCop.Analyzers.ReadabilityRules.SA1121UseBuiltInTypeAlias, + StyleCop.Analyzers.ReadabilityRules.SA1121CodeFixProvider>; public partial class SA1121CSharp11UnitTests : SA1121CSharp10UnitTests { + // NOTE: This tests a fix for a c# 10 feature, but the Roslyn API used to solve it wasn't available in the version + // we use in the c# 10 test project, so the test was added here instead. + [Fact] + [WorkItem(3594, "https://github.com/DotNetAnalyzers/StyleCopAnalyzers/issues/3594")] + 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.Test.CSharp12/Lightup/IImportScopeWrapperCSharp12UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp12/Lightup/IImportScopeWrapperCSharp12UnitTests.cs new file mode 100644 index 000000000..5cae05686 --- /dev/null +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp12/Lightup/IImportScopeWrapperCSharp12UnitTests.cs @@ -0,0 +1,9 @@ +// 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.Test.Lightup +{ + public partial class IImportScopeWrapperCSharp12UnitTests : IImportScopeWrapperCSharp11UnitTests + { + } +} diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/Helpers/SyntaxTreeHelpers.cs b/StyleCop.Analyzers/StyleCop.Analyzers/Helpers/SyntaxTreeHelpers.cs index c917df7a5..8bc9a6764 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,28 @@ 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) { + // Check for "local" using aliases var nodes = tree.GetRoot().DescendantNodes(node => node.IsKind(SyntaxKind.CompilationUnit) || node.IsKind(SyntaxKind.NamespaceDeclaration) || node.IsKind(SyntaxKindEx.FileScopedNamespaceDeclaration)); + if (nodes.OfType().Any(x => x.Alias != null)) + { + return true; + } + + // Check for global using aliases + if (SemanticModelExtensions.SupportsGetImportScopes) + { + var scopes = semanticModel.GetImportScopes(0, cancellationToken); + return scopes.Any(x => x.Aliases.Length > 0); + } - return nodes.OfType().Any(x => x.Alias != null); + return false; } } } diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/Lightup/IImportScopeWrapper.cs b/StyleCop.Analyzers/StyleCop.Analyzers/Lightup/IImportScopeWrapper.cs new file mode 100644 index 000000000..4ff967de7 --- /dev/null +++ b/StyleCop.Analyzers/StyleCop.Analyzers/Lightup/IImportScopeWrapper.cs @@ -0,0 +1,54 @@ +// 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 => 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..95e56f0c8 --- /dev/null +++ b/StyleCop.Analyzers/StyleCop.Analyzers/Lightup/SemanticModelExtensions.cs @@ -0,0 +1,162 @@ +// 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.Diagnostics; + 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 getImportScopesAccessor, out var supportsGetImportScopes); + GetImportScopesAccessor = getImportScopesAccessor; + SupportsGetImportScopes = supportsGetImportScopes; + } + + 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) + { + var semanticModelType = typeof(SemanticModel); + + var codeAnalysisWorkspacesAssembly = semanticModelType.GetTypeInfo().Assembly; + var nativeImportScopeType = codeAnalysisWorkspacesAssembly.GetType("Microsoft.CodeAnalysis.IImportScope"); + if (nativeImportScopeType == null) + { + accessor = FallbackAccessor; + isSupported = false; + return; + } + + var method = semanticModelType.GetTypeInfo().GetDeclaredMethods("GetImportScopes").SingleOrDefault(IsCorrectGetImportScopesMethod); + if (method == null) + { + accessor = FallbackAccessor; + isSupported = false; + return; + } + + 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 arrayCreateWrapperImportScopeBuilderMethod = 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, arrayCreateWrapperImportScopeBuilderMethod)), + 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; + + static bool IsCorrectGetImportScopesMethod(MethodInfo method) + { + var parameters = method.GetParameters(); + return parameters.Length == 2 + && 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) + { + Debug.Assert(false, $"This method should not have been called. Check {nameof(SupportsGetImportScopes)} first!"); + + 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) {