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

Apply 'require accessibility modifiers' setting on interface methods #60226

Closed
wants to merge 1 commit into from
Closed

Apply 'require accessibility modifiers' setting on interface methods #60226

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
99 changes: 61 additions & 38 deletions src/Analyzers/CSharp/Analyzers/AddAccessibilityModifiers/CSharpAddAccessibilityModifiers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,51 +37,74 @@ public override bool ShouldUpdateAccessibilityModifier(
// This analyzer bases all of its decisions on the accessibility
var accessibility = accessibilityFacts.GetAccessibility(member);

// Omit will flag any accessibility values that exist and are default
// The other options will remove or ignore accessibility
var isOmit = option == AccessibilityModifiersRequired.OmitIfDefault;
if (accessibility == Accessibility.NotApplicable)
{
return ShouldAddAccessibilityModifier(member, option);
}
else
{
return ShouldRemoveAccessibilityModifier(member, accessibility, option);
}
}

if (isOmit)
private static bool ShouldAddAccessibilityModifier(MemberDeclarationSyntax member,
AccessibilityModifiersRequired option)
{
if (member.Parent.IsKind(SyntaxKind.InterfaceDeclaration))
{
if (accessibility == Accessibility.NotApplicable)
return false;
return option == AccessibilityModifiersRequired.Always;
}

return option != AccessibilityModifiersRequired.OmitIfDefault;
}

var parentKind = member.GetRequiredParent().Kind();
switch (parentKind)
private static bool ShouldRemoveAccessibilityModifier(MemberDeclarationSyntax member,
Accessibility accessibility,
AccessibilityModifiersRequired option)
{
if (member.Parent.IsKind(SyntaxKind.InterfaceDeclaration))
{
if (option == AccessibilityModifiersRequired.Always || accessibility != Accessibility.Public)
{
// Check for default modifiers in namespace and outside of namespace
case SyntaxKind.CompilationUnit:
case SyntaxKind.FileScopedNamespaceDeclaration:
case SyntaxKind.NamespaceDeclaration:
{
// Default is internal
if (accessibility != Accessibility.Internal)
return false;
}

break;

case SyntaxKind.ClassDeclaration:
case SyntaxKind.RecordDeclaration:
case SyntaxKind.StructDeclaration:
case SyntaxKind.RecordStructDeclaration:
{
// Inside a type, default is private
if (accessibility != Accessibility.Private)
return false;
}

break;

default:
return false; // Unknown parent kind, don't do anything
return false;
}

return true;
}
else
else if (option != AccessibilityModifiersRequired.OmitIfDefault)
{
// Mode is always, so we have to flag missing modifiers
if (accessibility != Accessibility.NotApplicable)
return false;
return false;
}

var parentKind = member.GetRequiredParent().Kind();
switch (parentKind)
{
// Check for default modifiers in namespace and outside of namespace
case SyntaxKind.CompilationUnit:
case SyntaxKind.FileScopedNamespaceDeclaration:
case SyntaxKind.NamespaceDeclaration:
{
// Default is internal
if (accessibility != Accessibility.Internal)
return false;
}

break;

case SyntaxKind.ClassDeclaration:
case SyntaxKind.RecordDeclaration:
case SyntaxKind.StructDeclaration:
case SyntaxKind.RecordStructDeclaration:
{
// Inside a type, default is private
if (accessibility != Accessibility.Private)
return false;
}

break;

default:
return false; // Unknown parent kind, don't do anything
}

return true;
Expand Down
12 changes: 1 addition & 11 deletions .../Analyzers/AddAccessibilityModifiers/CSharpAddAccessibilityModifiersDiagnosticAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,23 +43,13 @@ private void ProcessMemberDeclaration(
// If we have a class or struct, recurse inwards.
if (member.IsKind(SyntaxKind.ClassDeclaration, out TypeDeclarationSyntax? typeDeclaration) ||
member.IsKind(SyntaxKind.StructDeclaration, out typeDeclaration) ||
member.IsKind(SyntaxKind.InterfaceDeclaration, out typeDeclaration) ||
member.IsKind(SyntaxKind.RecordDeclaration, out typeDeclaration) ||
member.IsKind(SyntaxKind.RecordStructDeclaration, out typeDeclaration))
{
ProcessMembers(context, option, typeDeclaration.Members);
}

#if false
// Add this once we have the language version for C# that supports accessibility
// modifiers on interface methods.
if (option.Value == AccessibilityModifiersRequired.Always &&
member.IsKind(SyntaxKind.InterfaceDeclaration, out typeDeclaration))
{
// Only recurse into an interface if the user wants accessibility modifiers on
ProcessTypeDeclaration(context, generator, option, typeDeclaration);
}
#endif

if (!CSharpAddAccessibilityModifiers.Instance.ShouldUpdateAccessibilityModifier(CSharpAccessibilityFacts.Instance, member, option.Value, out var name))
return;

Expand Down
101 changes: 101 additions & 0 deletions src/Analyzers/CSharp/Tests/AddAccessibilityModifiers/AddAccessibilityModifiersTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -611,5 +611,106 @@ internal class C : I<C>

await test.RunAsync();
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsAddAccessibilityModifiers)]
[WorkItem(47448, "https://github.com/dotnet/roslyn/issues/47448")]
public async Task TestModifierOnInterfaceMethods_01()
{
var source = @"public interface I
{
void M();
}";

var test = new VerifyCS.Test
{
TestCode = source,
FixedCode = source,
Options =
{
{ CodeStyleOptions2.RequireAccessibilityModifiers, AccessibilityModifiersRequired.ForNonInterfaceMembers },
}
};

await test.RunAsync();
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsAddAccessibilityModifiers)]
[WorkItem(47448, "https://github.com/dotnet/roslyn/issues/47448")]
public async Task TestModifierOnInterfaceMethods_02()
{
var source = @"public interface I
{
void [|M|]();
}";
var expected = @"public interface I
{
public void M();
}";

var test = new VerifyCS.Test
{
TestCode = source,
FixedCode = expected,
Options =
{
{ CodeStyleOptions2.RequireAccessibilityModifiers, AccessibilityModifiersRequired.Always },
}
};

await test.RunAsync();
}

[Theory, Trait(Traits.Feature, Traits.Features.CodeActionsAddAccessibilityModifiers)]
[WorkItem(47448, "https://github.com/dotnet/roslyn/issues/47448")]
[InlineData((int)AccessibilityModifiersRequired.ForNonInterfaceMembers)]
[InlineData((int)AccessibilityModifiersRequired.OmitIfDefault)]
public async Task TestModifierOnInterfaceMethods_03(int modifier)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public async Task TestModifierOnInterfaceMethods_03(int modifier)
public async Task TestModifierOnInterfaceMethods_03(int modifiersRequired)

{
var source = @"public interface I
{
public void [|M|]();
}";
var expected = @"public interface I
{
void M();
}";
var test = new VerifyCS.Test
{
TestCode = source,
FixedCode = expected,
Options =
{
{ CodeStyleOptions2.RequireAccessibilityModifiers, (AccessibilityModifiersRequired)modifier },
}
};

await test.RunAsync();
}

[Theory, Trait(Traits.Feature, Traits.Features.CodeActionsAddAccessibilityModifiers)]
[WorkItem(47448, "https://github.com/dotnet/roslyn/issues/47448")]
[InlineData((int)AccessibilityModifiersRequired.Always)]
[InlineData((int)AccessibilityModifiersRequired.ForNonInterfaceMembers)]
[InlineData((int)AccessibilityModifiersRequired.OmitIfDefault)]
public async Task TestModifierOnInterfaceMethods_04(int modifier)
{
var source = @"public interface I
{
protected void M();
}";

var test = new VerifyCS.Test
{
TestCode = source,
FixedCode = source,
ReferenceAssemblies = Testing.ReferenceAssemblies.Net.Net60,
Options =
{
{ CodeStyleOptions2.RequireAccessibilityModifiers, (AccessibilityModifiersRequired)modifier },
}
};

await test.RunAsync();
}
}
}
3 changes: 3 additions & 0 deletions src/Analyzers/Core/CodeFixes/AddAccessibilityModifiers/AddAccessibilityModifiersHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ private static Accessibility GetPreferredAccessibility(ISymbol symbol)
// that's not legal. And these are reasonable default values for them.
if (symbol is IMethodSymbol or IPropertySymbol or IEventSymbol)
{
if (symbol.ContainingType.IsInterfaceType())
return Accessibility.Public;

if (symbol.IsAbstract)
return Accessibility.Protected;

Expand Down