Rule S4015: Inherited member visibility should not be decreased #399
Conversation
| public sealed class DoNotDecreaseMemberVisibility : SonarDiagnosticAnalyzer | ||
| { | ||
| internal const string DiagnosticId = "S4015"; | ||
| private const string MessageFormat = "Make this member non-private or the class sealed."; |
There was a problem hiding this comment.
The message seems weirdo (especially the end part of it).
There was a problem hiding this comment.
It looks ok to me, but happy to adjust. Do you have a suggestion on how to improve?
There was a problem hiding this comment.
Forget about this comment.
There was a problem hiding this comment.
Why do we say member but care only about methods?
| return false; | ||
| } | ||
|
|
||
| if (classType.BaseType |
There was a problem hiding this comment.
I would extract this from the if condition and make it a meaningful variable.
| private bool HasSameParameters(IMethodSymbol methodCandidate, IMethodSymbol methodSymbol) | ||
| { | ||
| var methodCandidateParams = methodCandidate.Parameters.ToList(); | ||
| var methodParams = methodSymbol.Parameters.ToList(); |
There was a problem hiding this comment.
Why do you need to copy immutable arrays?
| return false; | ||
| } | ||
|
|
||
| bool baseClassHasMatchingPublicMethod = classType.BaseType |
There was a problem hiding this comment.
I would use a name like: isHiddingBaseClassMethod
| public sealed class DoNotDecreaseMemberVisibility : SonarDiagnosticAnalyzer | ||
| { | ||
| internal const string DiagnosticId = "S4015"; | ||
| private const string MessageFormat = "Make this member non-private or the class sealed."; |
There was a problem hiding this comment.
Why do we say member but care only about methods?
| var methodSymbol = c.SemanticModel.GetDeclaredSymbol(methodDeclaration); | ||
| var classType = methodSymbol?.ContainingType; | ||
|
|
||
| if (classType != null && |
There was a problem hiding this comment.
Don't know why I missed it on first review but here again I would extract the big if into a meaningful variable (isDecreasingMethodAccessibility for example).
| c.ReportDiagnostic(Diagnostic.Create(rule, methodDeclaration.Identifier.GetLocation())); | ||
| } | ||
| }, | ||
| SyntaxKind.MethodDeclaration); |
There was a problem hiding this comment.
Not sure it would change a lot about performances but I am wondering if it's not better to subscribe to class declaration so we don't have to walk through all base methods for every sub methods (what is currently done).
| using Microsoft.CodeAnalysis.Diagnostics; | ||
| using SonarAnalyzer.Common; | ||
| using SonarAnalyzer.Helpers; | ||
| using System.Collections.Generic; |
There was a problem hiding this comment.
Could you really set up the reordering of the using. I am commenting this on every PR xD
| { | ||
| context.RegisterSyntaxNodeActionInNonGenerated(c => | ||
| { | ||
| var classDeclaration = c.Node as ClassDeclarationSyntax; |
There was a problem hiding this comment.
You are sure this is a ClassDeclaration so you should use direct cast and not as.
| allBaseClassProperties = allBaseClassMembers.OfType<IPropertySymbol>().ToList(); | ||
| } | ||
|
|
||
| private static IEnumerable<ISymbol> GetAllBaseMembers(INamedTypeSymbol classType, Func<ISymbol, bool> filter) |
There was a problem hiding this comment.
Could you have a look at SemanticModel.LookupBaseMembers to see if it does the same thing or not?
|
|
||
| if (hidingMethod != null) | ||
| { | ||
| var location = (memberDeclaration as MethodDeclarationSyntax)?.Identifier.GetLocation(); |
There was a problem hiding this comment.
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.
| var hidingProperty = allBaseClassProperties.FirstOrDefault(p => DecreasesPropertyAccess(p, propertySymbol)); | ||
| if (hidingProperty != null) | ||
| { | ||
| var location = (memberDeclaration as PropertyDeclarationSyntax).Identifier.GetLocation(); |
There was a problem hiding this comment.
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 Equals(p1.Type.OriginalDefinition, p2.Type.OriginalDefinition); | ||
| } | ||
|
|
||
| private static bool DecrasesAccess(Accessibility baseAccess, Accessibility memberAccess) |
There was a problem hiding this comment.
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.
| return false; | ||
| } | ||
|
|
||
| bool hasMatchingParameterTypes = CollectionUtils.AreEqual(baseMethod.Parameters, methodSymbol.Parameters, |
There was a problem hiding this comment.
I would return directly the result of the method call.
|
|
||
| private static bool AreParameterTypesEqual(IParameterSymbol p1, IParameterSymbol p2) | ||
| { | ||
| if (p1.Type.TypeKind == TypeKind.TypeParameter) |
There was a problem hiding this comment.
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);
|
|
||
| private static bool DecrasesAccess(Accessibility baseAccess, Accessibility memberAccess) | ||
| { | ||
| if (baseAccess == Accessibility.NotApplicable || memberAccess == Accessibility.NotApplicable) |
There was a problem hiding this comment.
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.
|
|
||
| private static bool IsMatchingSignature(IMethodSymbol baseMethod, IMethodSymbol methodSymbol) | ||
| { | ||
| if (baseMethod.Name != methodSymbol.Name || |
There was a problem hiding this comment.
I would also merge all the method as a single return with 2 ORs (negate the condition of this IF).
…he class is in a different namespace.
Fix #350