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

Rule S4015: Inherited member visibility should not be decreased #399

Merged
merged 6 commits into from
Jun 14, 2017
Merged
Show file tree
Hide file tree
Changes from 4 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
@@ -0,0 +1,17 @@
{
"issues": [
{
"id": "S4015",
"message": "This member hides 'Akka.Persistence.Eventsourced.Log'. Make it non-private or seal the class.",
"location": {
"uri": "akka.net\src\core\Akka.Persistence.Tests\AtLeastOnceDeliveryCrashSpec.cs",
"region": {
"startLine": 75,
"startColumn": 29,
"endLine": 75,
"endColumn": 32
}
}
}
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{
"issues": [
{
"id": "S4015",
"message": "This member hides 'Akka.TestKit.AkkaSpec.AtStartup()'. Make it non-private or seal the class.",
"location": {
"uri": "akka.net\src\core\Akka.Remote.Tests\RemotingSpec.cs",
"region": {
"startLine": 328,
"startColumn": 22,
"endLine": 328,
"endColumn": 31
}
}
}
]
}
36 changes: 36 additions & 0 deletions sonaranalyzer-dotnet/rspec/cs/S4015_c#.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
<p>Changing an inherited member to <code>private</code> will not prevent access to the base class implementation.</p>
<p>This rule raises an issue when a <code>private</code>, non-<code>final</code> method in an unsealed type has a signature that is identical to a
<code>public</code> method declared in a base type.</p>
<h2>Noncompliant Code Example</h2>
<pre>
using System;

namespace MyLibrary
{
public class Foo
{
public void SomeMethod(int count) { }
}
public class Bar:Foo
{
private void SomeMethod(int count) { } // Noncompliant
}
}
</pre>
<h2>Compliant Solution</h2>
<pre>
using System;

namespace MyLibrary
{
public class Foo
{
public void SomeMethod(int count) { }
}
public class Bar:Foo
{
private final void SomeMethod(int count) { }
}
}
</pre>

13 changes: 13 additions & 0 deletions sonaranalyzer-dotnet/rspec/cs/S4015_c#.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{
"title": "Inherited member visibility should not be decreased",
"type": "CODE_SMELL",
"status": "ready",
"remediation": {
"func": "Constant\/Issue",
"constantCost": "2min"
},
"tags": [
"pitfall"
],
"defaultSeverity": "Critical"
}
1 change: 1 addition & 0 deletions sonaranalyzer-dotnet/rspec/cs/Sonar_way_profile.json
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@
"S3981",
"S3984",
"S3998",
"S4015",
"S4016"
]
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

using System;
using System.Collections.Generic;
using System.Linq;

namespace SonarAnalyzer.Helpers
{
Expand Down Expand Up @@ -76,5 +77,19 @@ public static bool AreEqual<T, V>(IEnumerable<T> collection1, IEnumerable<V> col

return enum1HasNext == enum2HasNext;
}

public static void ForEach<T>(this IEnumerable<T> enumerable, Action<T> action)
{
foreach (var element in enumerable)
{
action(element);
}
}

public static IEnumerable<T> WhereNotNull<T>(this IEnumerable<T> enumerable)
where T : class
{
return enumerable.Where(e => e != null);
}
}
}
27 changes: 27 additions & 0 deletions sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/RspecStrings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -6579,6 +6579,33 @@
<data name="S4005_Type" xml:space="preserve">
<value>CODE_SMELL</value>
</data>
<data name="S4015_Category" xml:space="preserve">
<value>Sonar Code Smell</value>
</data>
<data name="S4015_Description" xml:space="preserve">
<value>Changing an inherited member to private will not prevent access to the base class implementation.</value>
</data>
<data name="S4015_IsActivatedByDefault" xml:space="preserve">
<value>True</value>
</data>
<data name="S4015_Remediation" xml:space="preserve">
<value>Constant/Issue</value>
</data>
<data name="S4015_RemediationCost" xml:space="preserve">
<value>2min</value>
</data>
<data name="S4015_Severity" xml:space="preserve">
<value>Critical</value>
</data>
<data name="S4015_Tags" xml:space="preserve">
<value>pitfall</value>
</data>
<data name="S4015_Title" xml:space="preserve">
<value>Inherited member visibility should not be decreased</value>
</data>
<data name="S4015_Type" xml:space="preserve">
<value>CODE_SMELL</value>
</data>
<data name="S4016_Category" xml:space="preserve">
<value>Sonar Code Smell</value>
</data>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,198 @@
/*
* SonarAnalyzer for .NET
* Copyright (C) 2015-2017 SonarSource SA
* mailto: contact AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

using System.Collections.Immutable;
using System.Linq;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Diagnostics;
using SonarAnalyzer.Common;
using SonarAnalyzer.Helpers;
using System.Collections.Generic;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you really set up the reordering of the using. I am commenting this on every PR xD

using System;

namespace SonarAnalyzer.Rules.CSharp
{
[DiagnosticAnalyzer(LanguageNames.CSharp)]
[Rule(DiagnosticId)]
public sealed class DoNotDecreaseMemberVisibility : SonarDiagnosticAnalyzer
{
internal const string DiagnosticId = "S4015";
private const string MessageFormat = "This member hides '{0}'. Make it non-private or seal the class.";

private static readonly DiagnosticDescriptor rule =
DiagnosticDescriptorBuilder.GetDescriptor(DiagnosticId, MessageFormat, RspecStrings.ResourceManager);
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(rule);

protected override void Initialize(SonarAnalysisContext context)
{
context.RegisterSyntaxNodeActionInNonGenerated(c =>
{
var classDeclaration = c.Node as ClassDeclarationSyntax;
Copy link
Contributor

Choose a reason for hiding this comment

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

You are sure this is a ClassDeclaration so you should use direct cast and not as.

var classSymbol = c.SemanticModel.GetDeclaredSymbol(classDeclaration);

if (classSymbol == null ||
classDeclaration.Identifier.IsMissing ||
classSymbol.IsSealed)
{
return;
}

var issueFinder = new IssueFinder(classSymbol, c.SemanticModel);

classDeclaration
.Members
.Select(issueFinder.FindIssue)
.WhereNotNull()
.ForEach(c.ReportDiagnostic);
},
SyntaxKind.ClassDeclaration);
}

private class IssueFinder
{
private readonly IList<IMethodSymbol> allBaseClassMethods;
private readonly IList<IPropertySymbol> allBaseClassProperties;
private readonly SemanticModel semanticModel;

public IssueFinder(INamedTypeSymbol classSymbol, SemanticModel semanticModel)
{
this.semanticModel = semanticModel;
var allBaseClassMembers = GetAllBaseMembers(classSymbol, m => m.DeclaredAccessibility != Accessibility.Private);
allBaseClassMethods = allBaseClassMembers.OfType<IMethodSymbol>().ToList();
allBaseClassProperties = allBaseClassMembers.OfType<IPropertySymbol>().ToList();
}

private static IEnumerable<ISymbol> GetAllBaseMembers(INamedTypeSymbol classType, Func<ISymbol, bool> filter)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you have a look at SemanticModel.LookupBaseMembers to see if it does the same thing or not?

{
while (classType?.BaseType != null)
{
foreach (var member in classType.BaseType.GetMembers().Where(filter))
{
yield return member;
}

classType = classType.BaseType;
}
}

public Diagnostic FindIssue(MemberDeclarationSyntax memberDeclaration)
{
var memberSymbol = semanticModel.GetDeclaredSymbol(memberDeclaration);

var methodSymbol = memberSymbol as IMethodSymbol;
if (methodSymbol != null)
{
var hidingMethod = allBaseClassMethods.FirstOrDefault(
m => DecrasesAccess(m.DeclaredAccessibility, methodSymbol.DeclaredAccessibility) &&
IsMatchingSignature(m, methodSymbol));

if (hidingMethod != null)
{
var location = (memberDeclaration as MethodDeclarationSyntax)?.Identifier.GetLocation();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a preference so you are free to ignore it. I would extract the as cast into another variable. This makes things easier to read.

if (location != null)
{
return Diagnostic.Create(rule, location, hidingMethod);
}
}

return null;
}

var propertySymbol = memberSymbol as IPropertySymbol;
if (propertySymbol != null)
{
var hidingProperty = allBaseClassProperties.FirstOrDefault(p => DecreasesPropertyAccess(p, propertySymbol));
if (hidingProperty != null)
{
var location = (memberDeclaration as PropertyDeclarationSyntax).Identifier.GetLocation();
Copy link
Contributor

Choose a reason for hiding this comment

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

I would also extract this cast into a separate variable. Besides you use as cast but you are not checking for null here. Either it can't be null so you should use direct cast or it can and you should check for it.

return Diagnostic.Create(rule, location, hidingProperty);
}
}
return null;
}

private static bool DecreasesPropertyAccess(IPropertySymbol baseProperty, IPropertySymbol propertySymbol)
Copy link
Contributor

Choose a reason for hiding this comment

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

I tend to try prefixing boolean members with 'Has', 'Is', 'Can' which make it easy to understand this returns a boolean. So I would rename this method to IsDecreasingPropertyAccess.

{
if (baseProperty.Name != propertySymbol.Name ||
!Equals(baseProperty.Type, propertySymbol.Type))
{
return false;
}

var baseGetAccess = GetEffectiveDeclaredAccess(baseProperty.GetMethod, baseProperty.DeclaredAccessibility);
var baseSetAccess = GetEffectiveDeclaredAccess(baseProperty.SetMethod, baseProperty.DeclaredAccessibility);

var propertyGetAccess = GetEffectiveDeclaredAccess(propertySymbol.GetMethod, baseProperty.DeclaredAccessibility);
var propertySetAccess = GetEffectiveDeclaredAccess(propertySymbol.SetMethod, baseProperty.DeclaredAccessibility);

return DecrasesAccess(baseGetAccess, propertyGetAccess) ||
DecrasesAccess(baseSetAccess, propertySetAccess);
}

private static Accessibility GetEffectiveDeclaredAccess(IMethodSymbol method, Accessibility propertyDefaultAccess)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this is not doing what you want but the class SymbolHelper class already has a GetEffectiveAccessiblity method.

{
if (method == null)
{
return Accessibility.NotApplicable;
}

return method.DeclaredAccessibility != Accessibility.NotApplicable ? method.DeclaredAccessibility : propertyDefaultAccess;
}

private static bool IsMatchingSignature(IMethodSymbol baseMethod, IMethodSymbol methodSymbol)
{
if (baseMethod.Name != methodSymbol.Name ||
Copy link
Contributor

Choose a reason for hiding this comment

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

I would also merge all the method as a single return with 2 ORs (negate the condition of this IF).

baseMethod.TypeParameters.Length != methodSymbol.TypeParameters.Length)
{
return false;
}

bool hasMatchingParameterTypes = CollectionUtils.AreEqual(baseMethod.Parameters, methodSymbol.Parameters,
Copy link
Contributor

Choose a reason for hiding this comment

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

I would return directly the result of the method call.

AreParameterTypesEqual);

return hasMatchingParameterTypes;
}

private static bool AreParameterTypesEqual(IParameterSymbol p1, IParameterSymbol p2)
{
if (p1.Type.TypeKind == TypeKind.TypeParameter)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rewrite this method using the ? : operator.
return p1.Type.TypeKind == TypeKind.TypeParameter ? p2.Type.TypeKind == TypeKind.TypeParameter : Equals(p1.Type.OriginalDefinition, p2.Type.OriginalDefinition);

{
return p2.Type.TypeKind == TypeKind.TypeParameter;
}

return Equals(p1.Type.OriginalDefinition, p2.Type.OriginalDefinition);
}

private static bool DecrasesAccess(Accessibility baseAccess, Accessibility memberAccess)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same remark for the name of the method. Besides you made a typo in the name (Decrases instead of Decreases). There is a spell checker extension for VS you might want to use.

{
if (baseAccess == Accessibility.NotApplicable || memberAccess == Accessibility.NotApplicable)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can merge this if and this return. Besides you can simplify it furthermore because memberAccess == Accessibility.NotApplicable returns false is already covered by memberAccess == Accessibility.Private.

{
return false;
}

return memberAccess == Accessibility.Private ||
(baseAccess == Accessibility.Public && memberAccess != Accessibility.Public);
}

}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
<p>Changing an inherited member to <code>private</code> will not prevent access to the base class implementation.</p>
<p>This rule raises an issue when a <code>private</code>, non-<code>final</code> method in an unsealed type has a signature that is identical to a
<code>public</code> method declared in a base type.</p>
<h2>Noncompliant Code Example</h2>
<pre>
using System;

namespace MyLibrary
{
public class Foo
{
public void SomeMethod(int count) { }
}
public class Bar:Foo
{
private void SomeMethod(int count) { } // Noncompliant
}
}
</pre>
<h2>Compliant Solution</h2>
<pre>
using System;

namespace MyLibrary
{
public class Foo
{
public void SomeMethod(int count) { }
}
public class Bar:Foo
{
private final void SomeMethod(int count) { }
}
}
</pre>

Loading