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

Refactor PropertiesShouldNotBeWriteOnlyAnalyzer (CA1044) #6871

Merged
merged 1 commit into from
Aug 28, 2023
Merged

Refactor PropertiesShouldNotBeWriteOnlyAnalyzer (CA1044) #6871

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
31 changes: 16 additions & 15 deletions ...ore/Microsoft.CodeQuality.Analyzers/ApiDesignGuidelines/PropertiesShouldNotBeWriteOnly.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,40 +57,41 @@ public override void Initialize(AnalysisContext context)
/// </summary>
private static void AnalyzeSymbol(SymbolAnalysisContext context)
{
if (context.Symbol is not IPropertySymbol property)
{
return;
}
var property = (IPropertySymbol)context.Symbol;

// not raising a violation for when:
// property is overridden because the issue can only be fixed in the base type
// property is the implementation of any interface member
if (property.IsOverride || property.IsImplementationOfAnyInterfaceMember())
if (property.IsOverride || GetRule(property) is not DiagnosticDescriptor descriptor || property.IsImplementationOfAnyInterfaceMember())
Copy link
Member Author

Choose a reason for hiding this comment

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

NOTE: The checks in GetRule were previously done after calling IsImplementationOfAnyInterfaceMember.

I think that checks in GetRule are a lot cheaper and can eliminate lots of properties. So this might slightly have a performance impact. This analyzer is generally fast enough though.

{
return;
}

Debug.Assert(context.Options.MatchesConfiguredVisibility(MakeMoreAccessibleRule, property, context.Compilation) == context.Options.MatchesConfiguredVisibility(AddGetterRule, property, context.Compilation));
Debug.Assert(descriptor == MakeMoreAccessibleRule || descriptor == AddGetterRule);

// Only analyze externally visible properties by default
if (context.Options.MatchesConfiguredVisibility(descriptor, property, context.Compilation))
{
context.ReportDiagnostic(property.CreateDiagnostic(descriptor, property.Name));
}
}

private static DiagnosticDescriptor? GetRule(IPropertySymbol property)
{
// We handled the non-CA1044 cases earlier. Now, we handle CA1044 cases
// If there is no getter then it is not accessible
if (property.IsWriteOnly)
{
// Only analyze externally visible properties by default
if (context.Options.MatchesConfiguredVisibility(AddGetterRule, property, context.Compilation))
{
context.ReportDiagnostic(property.CreateDiagnostic(AddGetterRule, property.Name));
}
return AddGetterRule;
}
// Otherwise if there is a setter, check for its relative accessibility
else if (!property.IsReadOnly && (property.GetMethod!.DeclaredAccessibility < property.SetMethod!.DeclaredAccessibility))
{
// Only analyze externally visible properties by default
if (context.Options.MatchesConfiguredVisibility(MakeMoreAccessibleRule, property, context.Compilation))
{
context.ReportDiagnostic(property.CreateDiagnostic(MakeMoreAccessibleRule, property.Name));
}
return MakeMoreAccessibleRule;
}

return null;
}
}
}
6 changes: 3 additions & 3 deletions src/Utilities/Compiler/Extensions/ISymbolExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,17 +563,17 @@ public static ISymbol GetOverriddenMember(this ISymbol symbol)
/// </summary>
public static bool IsImplementationOfAnyExplicitInterfaceMember([NotNullWhen(returnValue: true)] this ISymbol? symbol)
{
if (symbol is IMethodSymbol methodSymbol && methodSymbol.ExplicitInterfaceImplementations.Any())
if (symbol is IMethodSymbol methodSymbol && !methodSymbol.ExplicitInterfaceImplementations.IsEmpty)
{
return true;
}

if (symbol is IPropertySymbol propertySymbol && propertySymbol.ExplicitInterfaceImplementations.Any())
if (symbol is IPropertySymbol propertySymbol && !propertySymbol.ExplicitInterfaceImplementations.IsEmpty)
{
return true;
}

if (symbol is IEventSymbol eventSymbol && eventSymbol.ExplicitInterfaceImplementations.Any())
if (symbol is IEventSymbol eventSymbol && !eventSymbol.ExplicitInterfaceImplementations.IsEmpty)
{
return true;
}
Expand Down