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.

Sign up for GitHub

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

Jump to bottom

Tiny Refactoring CA1824 #5124

Merged
merged 4 commits into from
Jun 28, 2021
Merged

Tiny Refactoring CA1824 #5124

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f951d6a
Tiny Refactoring CA1824
Youssef1313 May 23, 2021
c7a923d
Move TryCheckNeturalResourcesLanguageAttribute to compilation start
Youssef1313 May 23, 2021
4e56074
Fix formatting
Youssef1313 May 23, 2021
a35b306
Address feedback
Youssef1313 Jun 27, 2021
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
12 changes: 6 additions & 6 deletions ...Microsoft.NetCore.Analyzers/Resources/CSharpMarkAssembliesWithNeutralResourcesLanguage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,17 @@ namespace Microsoft.NetCore.CSharp.Analyzers.Resources
[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class CSharpMarkAssembliesWithNeutralResourcesLanguageAnalyzer : MarkAssembliesWithNeutralResourcesLanguageAnalyzer
{
protected override void RegisterAttributeAnalyzer(CompilationStartAnalysisContext context, Action onResourceFound)
protected override void RegisterAttributeAnalyzer(CompilationStartAnalysisContext context, Action onResourceFound, INamedTypeSymbol generatedCode)
{
context.RegisterSyntaxNodeAction(nc =>
context.RegisterSyntaxNodeAction(context =>
{
if (!CheckAttribute(nc.Node))
var attributeSyntax = (AttributeSyntax)context.Node;
Copy link
Member

Choose a reason for hiding this comment

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

Previous code was having the possibility that the Node isn't a AttributeSyntax, it also doesn't look possible but I am wondering if that was done for a specific reason or just by "node is generic so let's work in the generic case".

Copy link
Member Author

Choose a reason for hiding this comment

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

@Evangelink It should always be AttributeSyntax since we do RegisterSyntaxNodeAction(...., SyntaxKind.Attribute)

if (!CheckAttribute(attributeSyntax))
{
return;
}
if (!CheckResxGeneratedFile(nc.SemanticModel, nc.Node, ((AttributeSyntax)nc.Node).ArgumentList.Arguments[0].Expression, nc.CancellationToken))
if (!CheckResxGeneratedFile(context.SemanticModel, attributeSyntax, attributeSyntax.ArgumentList.Arguments[0].Expression, generatedCode, context.CancellationToken))
{
return;
}
Expand All @@ -33,9 +34,8 @@ protected override void RegisterAttributeAnalyzer(CompilationStartAnalysisContex
}, SyntaxKind.Attribute);
}

private static bool CheckAttribute(SyntaxNode node)
private static bool CheckAttribute(AttributeSyntax attribute)
{
var attribute = node as AttributeSyntax;
return attribute?.Name?.GetLastToken().Text?.Equals(GeneratedCodeAttribute, StringComparison.Ordinal) == true &&
attribute.ArgumentList.Arguments.Count > 0;
}
Expand Down
41 changes: 24 additions & 17 deletions .../Core/Microsoft.NetCore.Analyzers/Resources/MarkAssembliesWithNeutralResourcesLanguage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ public abstract class MarkAssembliesWithNeutralResourcesLanguageAnalyzer : Diagn
isDataflowRule: false,
isReportedAtCompilationEnd: true);

protected abstract void RegisterAttributeAnalyzer(CompilationStartAnalysisContext context, Action onResourceFound);
protected abstract void RegisterAttributeAnalyzer(CompilationStartAnalysisContext context, Action onResourceFound, INamedTypeSymbol generatedCode);

public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Rule);

Expand All @@ -53,9 +53,26 @@ public override void Initialize(AnalysisContext context)

context.RegisterCompilationStartAction(cc =>
Copy link
Member

Choose a reason for hiding this comment

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

As you have done some extra refactorings, it might be worth also renaming cc into context.

{
var hasResource = false;
INamedTypeSymbol? generatedCode = cc.Compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemCodeDomCompilerGeneratedCodeAttribute);
Copy link
Member

Choose a reason for hiding this comment

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

[cosmetic][minor] You could use the TryGetXXX instead of doing get then null check.

if (generatedCode is null)
{
return;
}

INamedTypeSymbol? attribute = cc.Compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemResourcesNeutralResourcesLanguageAttribute);
if (attribute is null)
{
return;
}

RegisterAttributeAnalyzer(cc, () => hasResource = true);
if (TryCheckNeutralResourcesLanguageAttribute(cc.Compilation, attribute, out var data))
{
// attribute already exist
return;
}

var hasResource = false;
RegisterAttributeAnalyzer(cc, () => hasResource = true, generatedCode);

cc.RegisterCompilationEndAction(ce =>
{
Expand All @@ -65,12 +82,6 @@ public override void Initialize(AnalysisContext context)
return;
}

if (TryCheckNeutralResourcesLanguageAttribute(ce, out AttributeData data))
{
// attribute already exist
return;
}

if (data != null)
{
// we have the attribute but its doing it wrong.
Expand All @@ -89,14 +100,13 @@ protected static bool CheckDesignerFile(SyntaxTree tree)
return tree.FilePath?.IndexOf(Designer, StringComparison.OrdinalIgnoreCase) > 0;
}

protected static bool CheckResxGeneratedFile(SemanticModel model, SyntaxNode attribute, SyntaxNode argument, CancellationToken cancellationToken)
protected static bool CheckResxGeneratedFile(SemanticModel model, SyntaxNode attribute, SyntaxNode argument, INamedTypeSymbol generatedCode, CancellationToken cancellationToken)
{
if (!CheckDesignerFile(model.SyntaxTree))
{
return false;
}

INamedTypeSymbol? generatedCode = model.Compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemCodeDomCompilerGeneratedCodeAttribute);
if (model.GetSymbolInfo(attribute, cancellationToken).Symbol?.ContainingType?.Equals(generatedCode) != true)
{
return false;
Expand All @@ -121,15 +131,12 @@ protected static bool CheckResxGeneratedFile(SemanticModel model, SyntaxNode att
return true;
}

private static bool TryCheckNeutralResourcesLanguageAttribute(CompilationAnalysisContext context, out AttributeData attributeData)
private static bool TryCheckNeutralResourcesLanguageAttribute(Compilation compilation, INamedTypeSymbol attribute, out AttributeData? attributeData)
{
INamedTypeSymbol? attribute = context.Compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemResourcesNeutralResourcesLanguageAttribute);
INamedTypeSymbol? @string = context.Compilation.GetSpecialType(SpecialType.System_String);

IEnumerable<AttributeData> attributes = context.Compilation.Assembly.GetAttributes().Where(d => d.AttributeClass?.Equals(attribute) == true);
IEnumerable<AttributeData> attributes = compilation.Assembly.GetAttributes().Where(d => SymbolEqualityComparer.Default.Equals(attribute, d.AttributeClass));
foreach (AttributeData data in attributes)
{
if (data.ConstructorArguments.Any(c => c.Type?.Equals(@string) == true && !string.IsNullOrWhiteSpace((string)c.Value)))
if (data.ConstructorArguments.Any(c => c.Value is string constantValue && !string.IsNullOrWhiteSpace(constantValue)))
{
// found one that already does right thing.
attributeData = data;
Expand Down
10 changes: 5 additions & 5 deletions .../Microsoft.NetCore.Analyzers/Resources/BasicMarkAssembliesWithNeutralResourcesLanguage.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,23 +13,23 @@ Namespace Microsoft.NetCore.VisualBasic.Analyzers.Resources
<DiagnosticAnalyzer(LanguageNames.VisualBasic)>
Public NotInheritable Class BasicMarkAssembliesWithNeutralResourcesLanguageAnalyzer
Inherits MarkAssembliesWithNeutralResourcesLanguageAnalyzer
Protected Overrides Sub RegisterAttributeAnalyzer(context As CompilationStartAnalysisContext, onResourceFound As Action)
Protected Overrides Sub RegisterAttributeAnalyzer(context As CompilationStartAnalysisContext, onResourceFound As Action, generatedCode As INamedTypeSymbol)
context.RegisterSyntaxNodeAction(
Sub(nc)
If Not CheckBasicAttribute(nc.Node) Then
Dim attributeSyntax = DirectCast(nc.Node, AttributeSyntax)
If Not CheckBasicAttribute(attributeSyntax) Then
Return
End If

If Not CheckResxGeneratedFile(nc.SemanticModel, nc.Node, DirectCast(nc.Node, AttributeSyntax).ArgumentList.Arguments(0).GetExpression(), nc.CancellationToken) Then
If Not CheckResxGeneratedFile(nc.SemanticModel, attributeSyntax, attributeSyntax.ArgumentList.Arguments(0).GetExpression(), generatedCode, nc.CancellationToken) Then
Return
End If

onResourceFound()
End Sub, SyntaxKind.Attribute)
End Sub

Private Shared Function CheckBasicAttribute(node As SyntaxNode) As Boolean
Dim attribute = TryCast(node, AttributeSyntax)
Private Shared Function CheckBasicAttribute(attribute As AttributeSyntax) As Boolean
Return (attribute?.Name?.GetLastToken().Text.Equals(GeneratedCodeAttribute, StringComparison.Ordinal) = True AndAlso
attribute.ArgumentList.Arguments.Count > 0).GetValueOrDefault()
End Function
Expand Down