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

Fix S2234 FN: primary constructors for records, classes and structs #8238

Merged
merged 29 commits into from
Oct 25, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
dd03b74
Rework S2234
martin-strecker-sonarsource Oct 20, 2023
3a0dc75
Clean-up
martin-strecker-sonarsource Oct 20, 2023
7089603
Fix NRE
martin-strecker-sonarsource Oct 20, 2023
b4c5a49
Move ArgumentList impl to extension mehtods in the language projects
martin-strecker-sonarsource Oct 23, 2023
6dccb04
member ordering
martin-strecker-sonarsource Oct 23, 2023
9daab19
Add VB tests for ArgumentList
martin-strecker-sonarsource Oct 23, 2023
36d3ca8
Add links to Roslyn
martin-strecker-sonarsource Oct 23, 2023
bd4f634
Refactor
martin-strecker-sonarsource Oct 23, 2023
5b3260c
Support primary location handling
martin-strecker-sonarsource Oct 23, 2023
c687e03
Location for PrimaryConstructorBaseTypeSyntax
martin-strecker-sonarsource Oct 23, 2023
ce9613d
Add support for VB primary location
martin-strecker-sonarsource Oct 23, 2023
0861d96
naming
martin-strecker-sonarsource Oct 23, 2023
6600fd1
Use Language comparrsion
martin-strecker-sonarsource Oct 23, 2023
7228408
Commets
martin-strecker-sonarsource Oct 23, 2023
b1e53e5
Comments
martin-strecker-sonarsource Oct 23, 2023
b11b572
Use `Rule` and add null test
martin-strecker-sonarsource Oct 23, 2023
076d3f6
Add tests for ArgumentNameColon
martin-strecker-sonarsource Oct 23, 2023
2b60c99
Simplify test
martin-strecker-sonarsource Oct 23, 2023
282169f
Coverage
martin-strecker-sonarsource Oct 23, 2023
47acd12
Fix IsRedundantPrimaryConstructorBaseTypeContext
martin-strecker-sonarsource Oct 23, 2023
b0c93d0
Fix comment
martin-strecker-sonarsource Oct 23, 2023
8089763
Primary clocation: ImplicitObjectCreationExpressionSyntaxWrapper
martin-strecker-sonarsource Oct 23, 2023
64bd12f
Return ArgumentListSyntax from ArgumentList() and use it in MethodPar…
martin-strecker-sonarsource Oct 23, 2023
5568b57
Fix AssertionArgsShouldBePassedInCorrectOrder tests
martin-strecker-sonarsource Oct 23, 2023
d29167a
Fix failing UTs
martin-strecker-sonarsource Oct 23, 2023
be99633
Simplify
martin-strecker-sonarsource Oct 24, 2023
9870e05
Code review
martin-strecker-sonarsource Oct 24, 2023
0ebf903
Use "NodeIdentifier" for PrimaryLocation
martin-strecker-sonarsource Oct 25, 2023
a9fe51c
Split condition
martin-strecker-sonarsource Oct 25, 2023
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
Expand Up @@ -146,6 +146,7 @@ public static SyntaxNode WalkUpParentheses(this SyntaxNode node)
AttributeSyntax { Name: { } name } => GetIdentifier(name),
BaseTypeDeclarationSyntax { Identifier: var identifier } => identifier,
ConstructorDeclarationSyntax { Identifier: var identifier } => identifier,
ConstructorInitializerSyntax { ThisOrBaseKeyword: var keyword } => keyword,
ConversionOperatorDeclarationSyntax { Type: { } type } => GetIdentifier(type),
DelegateDeclarationSyntax { Identifier: var identifier } => identifier,
DestructorDeclarationSyntax { Identifier: var identifier } => identifier,
Expand Down Expand Up @@ -177,8 +178,11 @@ public static SyntaxNode WalkUpParentheses(this SyntaxNode node)
PostfixUnaryExpressionSyntax { Operand: { } operand } => GetIdentifier(operand),
UsingDirectiveSyntax { Alias.Name: { } name } => GetIdentifier(name),
VariableDeclaratorSyntax { Identifier: var identifier } => identifier,
{ } implicitNew when ImplicitObjectCreationExpressionSyntaxWrapper.IsInstance(implicitNew) => ((ImplicitObjectCreationExpressionSyntaxWrapper)implicitNew).NewKeyword,
{ } fileScoped when FileScopedNamespaceDeclarationSyntaxWrapper.IsInstance(fileScoped)
&& ((FileScopedNamespaceDeclarationSyntaxWrapper)fileScoped).Name is { } name => GetIdentifier(name),
{ } primary when PrimaryConstructorBaseTypeSyntaxWrapper.IsInstance(primary)
&& ((PrimaryConstructorBaseTypeSyntaxWrapper)primary).Type is { } type => GetIdentifier(type),
{ } refType when RefTypeSyntaxWrapper.IsInstance(refType) => GetIdentifier(((RefTypeSyntaxWrapper)refType).Type),
_ => null
};
Expand Down Expand Up @@ -342,6 +346,19 @@ public static bool IsParentKind<T>(this SyntaxNode node, SyntaxKind kind, out T
return false;
}

// based on Type="ArgumentListSyntax" in https://github.com/dotnet/roslyn/blob/main/src/Compilers/CSharp/Portable/Syntax/Syntax.xml
public static ArgumentListSyntax ArgumentList(this SyntaxNode node) =>
cristian-ambrosini-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
node switch
{
ObjectCreationExpressionSyntax creation => creation.ArgumentList,
InvocationExpressionSyntax invocation => invocation.ArgumentList,
ConstructorInitializerSyntax constructorInitializer => constructorInitializer.ArgumentList,
null => null,
_ when PrimaryConstructorBaseTypeSyntaxWrapper.IsInstance(node) => ((PrimaryConstructorBaseTypeSyntaxWrapper)node).ArgumentList,
_ when ImplicitObjectCreationExpressionSyntaxWrapper.IsInstance(node) => ((ImplicitObjectCreationExpressionSyntaxWrapper)node).ArgumentList,
_ => throw new InvalidOperationException($"The {nameof(node)} of kind {node.Kind()} does not have an {nameof(ArgumentList)}."),
};

public static ParameterListSyntax ParameterList(this SyntaxNode node) =>
node switch
{
Expand Down
18 changes: 4 additions & 14 deletions analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpFacade.cs
Original file line number Diff line number Diff line change
Expand Up @@ -56,23 +56,13 @@ public IMethodParameterLookup MethodParameterLookup(SyntaxNode invocation, IMeth
{
null => null,
AttributeSyntax x => new CSharpAttributeParameterLookup(x, methodSymbol),
_ => new CSharpMethodParameterLookup(GetArgumentList(invocation), methodSymbol),
_ => new CSharpMethodParameterLookup(invocation.ArgumentList(), methodSymbol),
};

public IMethodParameterLookup MethodParameterLookup(SyntaxNode invocation, SemanticModel semanticModel) =>
invocation != null ? new CSharpMethodParameterLookup(GetArgumentList(invocation), semanticModel) : null;

private static ArgumentListSyntax GetArgumentList(SyntaxNode invocation) =>
invocation switch
{
ArgumentListSyntax x => x,
ObjectCreationExpressionSyntax x => x.ArgumentList,
InvocationExpressionSyntax x => x.ArgumentList,
_ when ImplicitObjectCreationExpressionSyntaxWrapper.IsInstance(invocation) =>
((ImplicitObjectCreationExpressionSyntaxWrapper)invocation).ArgumentList,
ConstructorInitializerSyntax x => x.ArgumentList,
_ => throw new ArgumentException($"{invocation.GetType()} does not contain an ArgumentList.", nameof(invocation)),
};
invocation?.ArgumentList() is { } argumentList
? new CSharpMethodParameterLookup(argumentList, semanticModel)
: null;

public string GetName(SyntaxNode expression) =>
expression.GetName();
Expand Down
16 changes: 8 additions & 8 deletions analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpSyntaxFacade.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,18 +26,17 @@ public override bool AreEquivalent(SyntaxNode firstNode, SyntaxNode secondNode)
SyntaxFactory.AreEquivalent(firstNode, secondNode);

public override IEnumerable<SyntaxNode> ArgumentExpressions(SyntaxNode node) =>
node switch
{
ObjectCreationExpressionSyntax creation => creation.ArgumentList?.Arguments.Select(x => x.Expression) ?? Enumerable.Empty<SyntaxNode>(),
null => Enumerable.Empty<SyntaxNode>(),
var _ when ImplicitObjectCreationExpressionSyntaxWrapper.IsInstance(node)
=> ((ImplicitObjectCreationExpressionSyntaxWrapper)node).ArgumentList?.Arguments.Select(x => x.Expression) ?? Enumerable.Empty<SyntaxNode>(),
_ => throw InvalidOperation(node, nameof(ArgumentExpressions))
};
ArgumentList(node)?.OfType<ArgumentSyntax>().Select(x => x.Expression) ?? Enumerable.Empty<SyntaxNode>();

public override IReadOnlyList<SyntaxNode> ArgumentList(SyntaxNode node) =>
node.ArgumentList()?.Arguments;

public override int? ArgumentIndex(SyntaxNode argument) =>
Cast<ArgumentSyntax>(argument).GetArgumentIndex();

public override SyntaxToken? ArgumentNameColon(SyntaxNode argument) =>
(argument as ArgumentSyntax)?.NameColon?.Name.Identifier;

public override SyntaxNode AssignmentLeft(SyntaxNode assignment) =>
Cast<AssignmentExpressionSyntax>(assignment).Left;

Expand Down Expand Up @@ -126,6 +125,7 @@ public override SyntaxNode NodeExpression(SyntaxNode node) =>
node switch
{
ArrowExpressionClauseSyntax x => x.Expression,
ArgumentSyntax x => x.Expression,
AttributeArgumentSyntax x => x.Expression,
InterpolationSyntax x => x.Expression,
InvocationExpressionSyntax x => x.Expression,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,12 @@ public sealed class AssertionArgsShouldBePassedInCorrectOrder : SonarDiagnosticA
protected override void Initialize(SonarAnalysisContext context) =>
context.RegisterNodeAction(c =>
{
if (c.Node is InvocationExpressionSyntax { ArgumentList: { Arguments.Count: >= 2 } argumentList } invocation
if (c.Node is InvocationExpressionSyntax { ArgumentList.Arguments.Count: >= 2 } invocation
&& GetParameters(invocation.GetName()) is { } knownAssertParameters
&& c.SemanticModel.GetSymbolInfo(invocation).AllSymbols()
.SelectMany(symbol =>
symbol is IMethodSymbol { IsStatic: true, ContainingSymbol: INamedTypeSymbol container } methodSymbol
? knownAssertParameters.Select(knownParameters => FindWrongArguments(c.SemanticModel, container, methodSymbol, argumentList, knownParameters))
? knownAssertParameters.Select(knownParameters => FindWrongArguments(c.SemanticModel, container, methodSymbol, invocation, knownParameters))
: Enumerable.Empty<WrongArguments?>())
.FirstOrDefault(x => x is not null) is (Expected: var expected, Actual: var actual))
{
Expand Down Expand Up @@ -79,10 +79,10 @@ private static KnownAssertParameters[] GetParameters(string name) =>
private static WrongArguments? FindWrongArguments(SemanticModel semanticModel,
INamedTypeSymbol container,
IMethodSymbol symbol,
ArgumentListSyntax argumentList,
InvocationExpressionSyntax invocation,
KnownAssertParameters knownParameters) =>
container.Is(knownParameters.AssertClass)
&& CSharpFacade.Instance.MethodParameterLookup(argumentList, symbol) is var parameterLookup
&& CSharpFacade.Instance.MethodParameterLookup(invocation, symbol) is var parameterLookup
&& parameterLookup.TryGetSyntax(knownParameters.ExpectedParameterName, out var expectedArguments)
&& expectedArguments.FirstOrDefault() is { } expected
&& semanticModel.GetConstantValue(expected).HasValue is false
Expand Down
70 changes: 11 additions & 59 deletions analyzers/src/SonarAnalyzer.CSharp/Rules/ParametersCorrectOrder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,66 +21,18 @@
namespace SonarAnalyzer.Rules.CSharp
{
[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class ParametersCorrectOrder : ParametersCorrectOrderBase<ArgumentSyntax>
public sealed class ParametersCorrectOrder : ParametersCorrectOrderBase<SyntaxKind>
{
private static readonly DiagnosticDescriptor rule =
DescriptorFactory.Create(DiagnosticId, MessageFormat);
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(rule);

protected override void Initialize(SonarAnalysisContext context)
{
context.RegisterNodeAction(
c =>
{
var methodCall = (InvocationExpressionSyntax)c.Node;

var memberAccess = methodCall.Expression as MemberAccessExpressionSyntax;
Location getLocation() =>
memberAccess == null
? methodCall.Expression.GetLocation()
: memberAccess.Name.GetLocation();

AnalyzeArguments(c, methodCall.ArgumentList, getLocation);
}, SyntaxKind.InvocationExpression);

context.RegisterNodeAction(
c =>
{
var objectCreationCall = (ObjectCreationExpressionSyntax)c.Node;

var qualifiedAccess = objectCreationCall.Type as QualifiedNameSyntax;
Location getLocation() =>
qualifiedAccess == null
? objectCreationCall.Type.GetLocation()
: qualifiedAccess.Right.GetLocation();

AnalyzeArguments(c, objectCreationCall.ArgumentList, getLocation);
}, SyntaxKind.ObjectCreationExpression);
}

private void AnalyzeArguments(SonarSyntaxNodeReportingContext analysisContext, ArgumentListSyntax argumentList,
Func<Location> getLocation)
{
if (argumentList == null)
protected override SyntaxKind[] InvocationKinds => new[]
{
return;
}

var methodParameterLookup = new CSharpMethodParameterLookup(argumentList, analysisContext.SemanticModel);

ReportIncorrectlyOrderedParameters(analysisContext, methodParameterLookup, argumentList.Arguments, getLocation);
}

protected override TypeInfo GetArgumentTypeSymbolInfo(ArgumentSyntax argument, SemanticModel semanticModel) =>
semanticModel.GetTypeInfo(argument.Expression);

protected override Location GetMethodDeclarationIdentifierLocation(SyntaxNode syntaxNode) =>
(syntaxNode as BaseMethodDeclarationSyntax)?.FindIdentifierLocation();

protected override SyntaxToken? GetArgumentIdentifier(ArgumentSyntax argument) =>
(argument.Expression as IdentifierNameSyntax)?.Identifier;

protected override SyntaxToken? GetNameColonArgumentIdentifier(ArgumentSyntax argument) =>
argument.NameColon?.Name.Identifier;
SyntaxKind.InvocationExpression,
SyntaxKind.ObjectCreationExpression,
SyntaxKind.ThisConstructorInitializer,
SyntaxKind.BaseConstructorInitializer,
SyntaxKindEx.PrimaryConstructorBaseType,
SyntaxKindEx.ImplicitObjectCreationExpression,
};

protected override ILanguageFacade<SyntaxKind> Language => CSharpFacade.Instance;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,22 @@ internal SonarSyntaxNodeReportingContext(SonarAnalysisContext analysisContext, S
public bool IsRedundantPositionalRecordContext() =>
Context.ContainingSymbol.Kind == SymbolKind.Method;

/// <summary>
/// Roslyn invokes the analyzer twice for PrimaryConstructorBaseType. The ContainingSymbol is first the type and second the constructor. This check filters can be used to filter
/// the first invocation. See also <seealso href="https://github.com/dotnet/roslyn/issues/70488">#Roslyn/70488</seealso>.
/// </summary>
/// <returns>
/// Returns <see langword="true"/> for the invocation with PrimaryConstructorBaseType and ContainingSymbol being <see cref="SymbolKind.NamedType"/> and
/// <see langword="false"/> for everything else.
/// </returns>
public bool IsRedundantPrimaryConstructorBaseTypeContext() =>
Context is
{
Node.RawKind: (int)SyntaxKindEx.PrimaryConstructorBaseType,
Compilation.Language: LanguageNames.CSharp,
ContainingSymbol.Kind: SymbolKind.NamedType,
};

public bool IsAzureFunction() =>
AzureFunctionMethod() is not null;

Expand Down
2 changes: 2 additions & 0 deletions analyzers/src/SonarAnalyzer.Common/Facade/SyntaxFacade.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ public abstract class SyntaxFacade<TSyntaxKind>
public abstract bool AreEquivalent(SyntaxNode firstNode, SyntaxNode secondNode);
public abstract IEnumerable<SyntaxNode> ArgumentExpressions(SyntaxNode node);
public abstract int? ArgumentIndex(SyntaxNode argument);
public abstract IReadOnlyList<SyntaxNode> ArgumentList(SyntaxNode node);
public abstract SyntaxToken? ArgumentNameColon(SyntaxNode argument);
public abstract SyntaxNode AssignmentLeft(SyntaxNode assignment);
public abstract SyntaxNode AssignmentRight(SyntaxNode assignment);
public abstract ImmutableArray<SyntaxNode> AssignmentTargets(SyntaxNode assignment);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ public interface IMethodParameterLookup
bool TryGetSyntax(IParameterSymbol parameter, out ImmutableArray<SyntaxNode> expressions);
bool TryGetSyntax(string parameterName, out ImmutableArray<SyntaxNode> expressions);
bool TryGetNonParamsSyntax(IParameterSymbol parameter, out SyntaxNode expression);
IMethodSymbol MethodSymbol { get; }
}

// This should come from the Roslyn API (https://github.com/dotnet/roslyn/issues/9)
Expand Down
Loading
Loading