Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update SA1121 and SA1404 to detect global using aliases [on hold] #3671

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
54 changes: 40 additions & 14 deletions StyleCop.Analyzers/StyleCop.Analyzers/Helpers/SyntaxTreeHelpers.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -23,17 +24,19 @@ internal static class SyntaxTreeHelpers
/// <para>This allows many analyzers that run on every token in the file to avoid checking
/// the same state in the document repeatedly.</para>
/// </remarks>
private static Tuple<WeakReference<Compilation>, ConcurrentDictionary<SyntaxTree, bool>> usingAliasCache
= Tuple.Create(new WeakReference<Compilation>(null), default(ConcurrentDictionary<SyntaxTree, bool>));
private static Tuple<WeakReference<Compilation>, IReadOnlyDictionary<SyntaxTree, bool>> usingAliasCache
= Tuple.Create(new WeakReference<Compilation>(null), default(IReadOnlyDictionary<SyntaxTree, bool>));

public static ConcurrentDictionary<SyntaxTree, bool> GetOrCreateUsingAliasCache(this Compilation compilation)
public static IReadOnlyDictionary<SyntaxTree, bool> GetOrCreateUsingAliasCache(this Compilation compilation)
{
var cache = usingAliasCache;

Compilation cachedCompilation;
if (!cache.Item1.TryGetTarget(out cachedCompilation) || cachedCompilation != compilation)
{
var replacementCache = Tuple.Create(new WeakReference<Compilation>(compilation), new ConcurrentDictionary<SyntaxTree, bool>());
var replacementDictionary = CreateDictionary(compilation);
var replacementCache = Tuple.Create(new WeakReference<Compilation>(compilation), replacementDictionary);

while (true)
{
var prior = Interlocked.CompareExchange(ref usingAliasCache, replacementCache, cache);
Expand Down Expand Up @@ -72,29 +75,52 @@ public static bool IsWhitespaceOnly(this SyntaxTree tree, CancellationToken canc
&& TriviaHelper.IndexOfFirstNonWhitespaceTrivia(firstToken.LeadingTrivia) == -1;
}

internal static bool ContainsUsingAlias(this SyntaxTree tree, ConcurrentDictionary<SyntaxTree, bool> cache)
internal static bool ContainsUsingAlias(this SyntaxTree tree, IReadOnlyDictionary<SyntaxTree, bool> cache)
{
if (tree == null)
{
return false;
}

bool result;
if (cache.TryGetValue(tree, out result))
if (cache == null)
{
// NOTE: This happens if any syntax tree in the compilation contains a global using alias
return true;
}

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<SyntaxTree, bool> 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<SyntaxTree, bool>();

return nodes.OfType<UsingDirectiveSyntax>().Any(x => x.Alias != null);
foreach (var tree in compilation.SyntaxTrees)
Copy link
Member

@sharwell sharwell Jun 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❗ I have concerns about the performance impact of this change. We've seen many cases where analyzer initialization overhead is the dominant contributor to a negative performance trace. I believe it would be better to use SemanticModel.GetImportScopes for this purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright. GetImportScopes seems to be supported from Roslyn 4.3 and global usings from 4.0, so I guess that there will be some versions where the bug will still exist if that approach is taken, but some false negatives might be ok since it will only happen for a few versions?

Would this method be supported automatically with newer (than c# 8) light-up support or would it require manually implementing that support?

Is this method quick? I have gotten the impression that the semantic model should be used with care, but maybe this is not a problematic method.

{
CheckUsingAliases(tree, out var containsUsingAlias, out var containsGlobalUsingAlias);
if (containsGlobalUsingAlias)
{
return null;
}

result.Add(tree, containsUsingAlias);
}

return result;
}

private static void CheckUsingAliases(SyntaxTree tree, out bool containsUsingAlias, out bool containsGlobalUsingAlias)
{
var usingNodes = tree.GetRoot().DescendantNodes(node => node.IsKind(SyntaxKind.CompilationUnit) || node.IsKind(SyntaxKind.NamespaceDeclaration) || node.IsKind(SyntaxKindEx.FileScopedNamespaceDeclaration)).OfType<UsingDirectiveSyntax>();
var usingAliasNodes = usingNodes.Where(x => x.Alias != null).ToList();
containsUsingAlias = usingAliasNodes.Any();
containsGlobalUsingAlias = usingAliasNodes.Any(x => !x.GlobalKeyword().IsKind(SyntaxKind.None));
}
}
}
Original file line number Diff line number Diff line change
@@ -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<UsingDirectiveSyntax, SyntaxToken> GlobalKeywordAccessor;
private static readonly Func<UsingDirectiveSyntax, SyntaxToken, UsingDirectiveSyntax> WithGlobalKeywordAccessor;

static UsingDirectiveSyntaxExtensions()
{
GlobalKeywordAccessor = LightupHelpers.CreateSyntaxPropertyAccessor<UsingDirectiveSyntax, SyntaxToken>(typeof(UsingDirectiveSyntax), nameof(GlobalKeyword));
WithGlobalKeywordAccessor = LightupHelpers.CreateSyntaxWithPropertyAccessor<UsingDirectiveSyntax, SyntaxToken>(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);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -78,15 +78,15 @@ private static void HandleCompilationStart(CompilationStartAnalysisContext conte
/// </summary>
private sealed class AnalyzerInstance
{
private readonly ConcurrentDictionary<SyntaxTree, bool> usingAliasCache;
private readonly IReadOnlyDictionary<SyntaxTree, bool> usingAliasCache;

/// <summary>
/// A lazily-initialized reference to <see cref="SuppressMessageAttribute"/> within the context of a
/// particular <see cref="Compilation"/>.
/// </summary>
private INamedTypeSymbol suppressMessageAttribute;

public AnalyzerInstance(ConcurrentDictionary<SyntaxTree, bool> usingAliasCache)
public AnalyzerInstance(IReadOnlyDictionary<SyntaxTree, bool> usingAliasCache)
{
this.usingAliasCache = usingAliasCache;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -159,9 +159,9 @@ private static void HandleCompilationStart(CompilationStartAnalysisContext conte

private sealed class Analyzer
{
private readonly ConcurrentDictionary<SyntaxTree, bool> usingAliasCache;
private readonly IReadOnlyDictionary<SyntaxTree, bool> usingAliasCache;

public Analyzer(ConcurrentDictionary<SyntaxTree, bool> usingAliasCache)
public Analyzer(IReadOnlyDictionary<SyntaxTree, bool> usingAliasCache)
{
this.usingAliasCache = usingAliasCache;
}
Expand Down