From 0101151cb68950ae53617ff0fb31f9147f69a53a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Hellander?= Date: Sun, 18 Jun 2023 17:53:30 +0200 Subject: [PATCH 1/2] Update SA1121 and SA1404 to detect global using aliases #3594 --- .../SA1404CSharp10UnitTests.cs | 48 +++++++++++++++++++ .../SA1121CSharp10UnitTests.cs | 26 ++++++++++ .../Helpers/SyntaxTreeHelpers.cs | 24 +++++++++- .../Lightup/UsingDirectiveSyntaxExtensions.cs | 31 ++++++++++++ 4 files changed, 128 insertions(+), 1 deletion(-) create mode 100644 StyleCop.Analyzers/StyleCop.Analyzers/Lightup/UsingDirectiveSyntaxExtensions.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..1182adc31 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp10/MaintainabilityRules/SA1404CSharp10UnitTests.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp10/MaintainabilityRules/SA1404CSharp10UnitTests.cs @@ -3,9 +3,57 @@ 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() + { + + } +}"; + + var test = new CSharpTest + { + RemainingDiagnostics = + { + Diagnostic().WithLocation("/0/Test1.cs", 4, 32), + }, + NumberOfIncrementalIterations = 2, + NumberOfFixAllIterations = 2, + }; + test.TestState.Sources.Add(testCode1); + test.TestState.Sources.Add(testCode2); + test.FixedState.Sources.Add(testCode1); + test.FixedState.Sources.Add(fixedCode2); + await test.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..f48030af5 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp10/ReadabilityRules/SA1121CSharp10UnitTests.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp10/ReadabilityRules/SA1121CSharp10UnitTests.cs @@ -42,5 +42,31 @@ 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; +}"; + + var test = new CSharpTest(); + test.TestState.Sources.Add(source1); + test.TestState.Sources.Add(oldSource2); + test.FixedState.Sources.Add(source1); + test.FixedState.Sources.Add(newSource2); + await test.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..ff0b17281 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers/Helpers/SyntaxTreeHelpers.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers/Helpers/SyntaxTreeHelpers.cs @@ -33,7 +33,10 @@ public static ConcurrentDictionary GetOrCreateUsingAliasCache( Compilation cachedCompilation; if (!cache.Item1.TryGetTarget(out cachedCompilation) || cachedCompilation != compilation) { - var replacementCache = Tuple.Create(new WeakReference(compilation), new ConcurrentDictionary()); + var containsGlobalUsingAlias = ContainsGlobalUsingAliasNoCache(compilation); + var replacementDictionary = containsGlobalUsingAlias ? null : new ConcurrentDictionary(); + var replacementCache = Tuple.Create(new WeakReference(compilation), replacementDictionary); + while (true) { var prior = Interlocked.CompareExchange(ref usingAliasCache, replacementCache, cache); @@ -79,6 +82,12 @@ internal static bool ContainsUsingAlias(this SyntaxTree tree, ConcurrentDictiona return false; } + if (cache == null) + { + // NOTE: This happens if any syntax tree in the compilation contains a global using alias + return true; + } + bool result; if (cache.TryGetValue(tree, out result)) { @@ -96,5 +105,18 @@ private static bool ContainsUsingAliasNoCache(SyntaxTree tree) return nodes.OfType().Any(x => x.Alias != null); } + + private static bool ContainsGlobalUsingAliasNoCache(Compilation compilation) + { + return compilation.SyntaxTrees.Any(ContainsGlobalUsingAliasNoCache); + } + + private static bool ContainsGlobalUsingAliasNoCache(SyntaxTree tree) + { + var nodes = tree.GetRoot().DescendantNodes(node => node.IsKind(SyntaxKind.CompilationUnit) || node.IsKind(SyntaxKind.NamespaceDeclaration) || node.IsKind(SyntaxKindEx.FileScopedNamespaceDeclaration)); + + var relevantNodes = nodes.OfType().ToArray(); + return relevantNodes.Any(x => x.Alias != null && !x.GlobalKeyword().IsKind(SyntaxKind.None)); + } } } diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/Lightup/UsingDirectiveSyntaxExtensions.cs b/StyleCop.Analyzers/StyleCop.Analyzers/Lightup/UsingDirectiveSyntaxExtensions.cs new file mode 100644 index 000000000..02f27de3e --- /dev/null +++ b/StyleCop.Analyzers/StyleCop.Analyzers/Lightup/UsingDirectiveSyntaxExtensions.cs @@ -0,0 +1,31 @@ +// 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 Microsoft.CodeAnalysis; + using Microsoft.CodeAnalysis.CSharp.Syntax; + + internal static class UsingDirectiveSyntaxExtensions + { + private static readonly Func GlobalKeywordAccessor; + private static readonly Func WithGlobalKeywordAccessor; + + static UsingDirectiveSyntaxExtensions() + { + GlobalKeywordAccessor = LightupHelpers.CreateSyntaxPropertyAccessor(typeof(UsingDirectiveSyntax), nameof(GlobalKeyword)); + WithGlobalKeywordAccessor = LightupHelpers.CreateSyntaxWithPropertyAccessor(typeof(UsingDirectiveSyntax), nameof(GlobalKeyword)); + } + + public static SyntaxToken GlobalKeyword(this UsingDirectiveSyntax syntax) + { + return GlobalKeywordAccessor(syntax); + } + + public static UsingDirectiveSyntax WithGlobalKeyword(this UsingDirectiveSyntax syntax, SyntaxToken globalKeyword) + { + return WithGlobalKeywordAccessor(syntax, globalKeyword); + } + } +} From f2a69edf78ba5dc60c60ae3d1700283e07d157fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Hellander?= Date: Mon, 19 Jun 2023 23:10:31 +0200 Subject: [PATCH 2/2] Optimize using alias cache to only check using statements once per syntax tree, by filling the dictionary directly when it is created. In the previous commit, it was checked for global using aliases once and then again for normal using aliases. #3594 --- .../Helpers/SyntaxTreeHelpers.cs | 52 ++++++++++--------- ...nalysisSuppressionMustHaveJustification.cs | 6 +-- .../SA1121UseBuiltInTypeAlias.cs | 6 +-- 3 files changed, 34 insertions(+), 30 deletions(-) diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/Helpers/SyntaxTreeHelpers.cs b/StyleCop.Analyzers/StyleCop.Analyzers/Helpers/SyntaxTreeHelpers.cs index ff0b17281..9e2470720 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers/Helpers/SyntaxTreeHelpers.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers/Helpers/SyntaxTreeHelpers.cs @@ -6,7 +6,8 @@ namespace StyleCop.Analyzers.Helpers { using System; - using System.Collections.Concurrent; + using System.Collections.Generic; + using System.Diagnostics; using System.Linq; using System.Threading; using Microsoft.CodeAnalysis; @@ -23,18 +24,17 @@ internal static class SyntaxTreeHelpers /// This allows many analyzers that run on every token in the file to avoid checking /// the same state in the document repeatedly. /// - private static Tuple, ConcurrentDictionary> usingAliasCache - = Tuple.Create(new WeakReference(null), default(ConcurrentDictionary)); + private static Tuple, IReadOnlyDictionary> usingAliasCache + = Tuple.Create(new WeakReference(null), default(IReadOnlyDictionary)); - public static ConcurrentDictionary GetOrCreateUsingAliasCache(this Compilation compilation) + public static IReadOnlyDictionary GetOrCreateUsingAliasCache(this Compilation compilation) { var cache = usingAliasCache; Compilation cachedCompilation; if (!cache.Item1.TryGetTarget(out cachedCompilation) || cachedCompilation != compilation) { - var containsGlobalUsingAlias = ContainsGlobalUsingAliasNoCache(compilation); - var replacementDictionary = containsGlobalUsingAlias ? null : new ConcurrentDictionary(); + var replacementDictionary = CreateDictionary(compilation); var replacementCache = Tuple.Create(new WeakReference(compilation), replacementDictionary); while (true) @@ -75,7 +75,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, IReadOnlyDictionary cache) { if (tree == null) { @@ -88,35 +88,39 @@ internal static bool ContainsUsingAlias(this SyntaxTree tree, ConcurrentDictiona return true; } - bool result; - if (cache.TryGetValue(tree, out result)) + if (cache.TryGetValue(tree, out var result)) { return result; } - bool generated = ContainsUsingAliasNoCache(tree); - cache.TryAdd(tree, generated); - return generated; + Debug.Assert(false, "This should not happen. Syntax tree could not be found in cache!"); + return false; } - private static bool ContainsUsingAliasNoCache(SyntaxTree tree) + private static IReadOnlyDictionary CreateDictionary(Compilation compilation) { - var nodes = tree.GetRoot().DescendantNodes(node => node.IsKind(SyntaxKind.CompilationUnit) || node.IsKind(SyntaxKind.NamespaceDeclaration) || node.IsKind(SyntaxKindEx.FileScopedNamespaceDeclaration)); + var result = new Dictionary(); - return nodes.OfType().Any(x => x.Alias != null); - } + foreach (var tree in compilation.SyntaxTrees) + { + CheckUsingAliases(tree, out var containsUsingAlias, out var containsGlobalUsingAlias); + if (containsGlobalUsingAlias) + { + return null; + } - private static bool ContainsGlobalUsingAliasNoCache(Compilation compilation) - { - return compilation.SyntaxTrees.Any(ContainsGlobalUsingAliasNoCache); + result.Add(tree, containsUsingAlias); + } + + return result; } - private static bool ContainsGlobalUsingAliasNoCache(SyntaxTree tree) + private static void CheckUsingAliases(SyntaxTree tree, out bool containsUsingAlias, out bool containsGlobalUsingAlias) { - var nodes = tree.GetRoot().DescendantNodes(node => node.IsKind(SyntaxKind.CompilationUnit) || node.IsKind(SyntaxKind.NamespaceDeclaration) || node.IsKind(SyntaxKindEx.FileScopedNamespaceDeclaration)); - - var relevantNodes = nodes.OfType().ToArray(); - return relevantNodes.Any(x => x.Alias != null && !x.GlobalKeyword().IsKind(SyntaxKind.None)); + var usingNodes = tree.GetRoot().DescendantNodes(node => node.IsKind(SyntaxKind.CompilationUnit) || node.IsKind(SyntaxKind.NamespaceDeclaration) || node.IsKind(SyntaxKindEx.FileScopedNamespaceDeclaration)).OfType(); + var usingAliasNodes = usingNodes.Where(x => x.Alias != null).ToList(); + containsUsingAlias = usingAliasNodes.Any(); + containsGlobalUsingAlias = usingAliasNodes.Any(x => !x.GlobalKeyword().IsKind(SyntaxKind.None)); } } } diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/MaintainabilityRules/SA1404CodeAnalysisSuppressionMustHaveJustification.cs b/StyleCop.Analyzers/StyleCop.Analyzers/MaintainabilityRules/SA1404CodeAnalysisSuppressionMustHaveJustification.cs index 10b7e65d2..b0d1d8d5d 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers/MaintainabilityRules/SA1404CodeAnalysisSuppressionMustHaveJustification.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers/MaintainabilityRules/SA1404CodeAnalysisSuppressionMustHaveJustification.cs @@ -6,7 +6,7 @@ namespace StyleCop.Analyzers.MaintainabilityRules { using System; - using System.Collections.Concurrent; + using System.Collections.Generic; using System.Collections.Immutable; using System.Diagnostics.CodeAnalysis; using Microsoft.CodeAnalysis; @@ -78,7 +78,7 @@ private static void HandleCompilationStart(CompilationStartAnalysisContext conte /// private sealed class AnalyzerInstance { - private readonly ConcurrentDictionary usingAliasCache; + private readonly IReadOnlyDictionary usingAliasCache; /// /// A lazily-initialized reference to within the context of a @@ -86,7 +86,7 @@ private sealed class AnalyzerInstance /// private INamedTypeSymbol suppressMessageAttribute; - public AnalyzerInstance(ConcurrentDictionary usingAliasCache) + public AnalyzerInstance(IReadOnlyDictionary usingAliasCache) { this.usingAliasCache = usingAliasCache; } diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/ReadabilityRules/SA1121UseBuiltInTypeAlias.cs b/StyleCop.Analyzers/StyleCop.Analyzers/ReadabilityRules/SA1121UseBuiltInTypeAlias.cs index 8f86c7322..6c820eddd 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers/ReadabilityRules/SA1121UseBuiltInTypeAlias.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers/ReadabilityRules/SA1121UseBuiltInTypeAlias.cs @@ -6,7 +6,7 @@ namespace StyleCop.Analyzers.ReadabilityRules { using System; - using System.Collections.Concurrent; + using System.Collections.Generic; using System.Collections.Immutable; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp; @@ -159,9 +159,9 @@ private static void HandleCompilationStart(CompilationStartAnalysisContext conte private sealed class Analyzer { - private readonly ConcurrentDictionary usingAliasCache; + private readonly IReadOnlyDictionary usingAliasCache; - public Analyzer(ConcurrentDictionary usingAliasCache) + public Analyzer(IReadOnlyDictionary usingAliasCache) { this.usingAliasCache = usingAliasCache; }