Skip to content

Commit

Permalink
Fix S3604 FP: Primary constructors (#8390)
Browse files Browse the repository at this point in the history
  • Loading branch information
martin-strecker-sonarsource authored Nov 22, 2023
1 parent d338d0d commit 34d8bc8
Show file tree
Hide file tree
Showing 3 changed files with 59 additions and 70 deletions.
30 changes: 0 additions & 30 deletions analyzers/its/expected/Net8/Net8--net8.0-S3604.json

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -47,19 +47,16 @@ protected override void Initialize(SonarAnalysisContext context) =>
c =>
{
var declaration = (TypeDeclarationSyntax)c.Node;
var members = c.SemanticModel.GetDeclaredSymbol(declaration)?.GetMembers();
if (members == null || members.Value.Length == 0)
if (c.SemanticModel.GetDeclaredSymbol(declaration)?.GetMembers() is { Length: > 0 } members)
{
return;
}

// structs cannot initialize fields/properties at declaration time
// interfaces cannot have instance fields and instance properties cannot have initializers
if (declaration is ClassDeclarationSyntax)
{
CheckInstanceMembers(c, declaration, members);
// structs cannot initialize fields/properties at declaration time
// interfaces cannot have instance fields and instance properties cannot have initializers
if (declaration is ClassDeclarationSyntax)
{
CheckInstanceMembers(c, declaration, members);
}
CheckStaticMembers(c, declaration, members);
}
CheckStaticMembers(c, declaration, members);
},
// For record support, see details in https://github.com/SonarSource/sonar-dotnet/pull/4756
// it is difficult to work with instance record constructors w/o raising FPs
Expand All @@ -69,9 +66,11 @@ protected override void Initialize(SonarAnalysisContext context) =>

private void CheckInstanceMembers(SonarSyntaxNodeReportingContext c, TypeDeclarationSyntax declaration, IEnumerable<ISymbol> typeMembers)
{
var typeInitializers = typeMembers.OfType<IMethodSymbol>().Where(x => x is { MethodKind: MethodKind.Constructor, IsStatic: false, IsImplicitlyDeclared: false }).ToList();
if (typeInitializers.Count == 0)
var constructors = typeMembers.OfType<IMethodSymbol>().Where(x => x is { MethodKind: MethodKind.Constructor, IsStatic: false }).ToList();
if (constructors.Exists(x => x.IsImplicitlyDeclared || x.IsPrimaryConstructor()))
{
// Implicit parameterless constructors and primary constructors can be considered as having an
// empty body and they do not initialize any members. If any of these is present, the rule does not apply.
return;
}

Expand All @@ -82,24 +81,17 @@ private void CheckInstanceMembers(SonarSyntaxNodeReportingContext c, TypeDeclara
return;
}

var initializerDeclarations = GetInitializerDeclarations<ConstructorDeclarationSyntax>(c, typeInitializers);
foreach (var memberSymbol in initializedMembers.Keys)
var constructorDeclarations = GetConstructorDeclarations<ConstructorDeclarationSyntax>(c, constructors);
foreach (var kvp in initializedMembers)
{
// the instance member should be initialized in ALL instance constructors
// otherwise, initializing it inline makes sense and the rule should not report
if (initializerDeclarations.All(constructor =>
{
if (constructor.Node.Initializer != null
&& constructor.Node.Initializer.ThisOrBaseKeyword.IsKind(SyntaxKind.ThisKeyword))
{
// Calls another ctor, which is also checked.
return true;
}

return IsSymbolFirstSetInCfg(memberSymbol, constructor.Node, constructor.Model, c.Cancel);
}))
if (constructorDeclarations.TrueForAll(x =>
// Calls another ctor, which is also checked:
x is { Node.Initializer.ThisOrBaseKeyword.RawKind: (int)SyntaxKind.ThisKeyword }
|| IsSymbolFirstSetInCfg(kvp.Key, x.Node, x.Model, c.Cancel)))
{
c.ReportIssue(Diagnostic.Create(Rule, initializedMembers[memberSymbol].GetLocation(), InstanceMemberMessage));
c.ReportIssue(Diagnostic.Create(Rule, kvp.Value.GetLocation(), InstanceMemberMessage));
}
}
}
Expand All @@ -119,7 +111,7 @@ private void CheckStaticMembers(SonarSyntaxNodeReportingContext c, TypeDeclarati
return;
}

var initializerDeclarations = GetInitializerDeclarations<BaseMethodDeclarationSyntax>(c, typeInitializers);
var initializerDeclarations = GetConstructorDeclarations<BaseMethodDeclarationSyntax>(c, typeInitializers);
foreach (var memberSymbol in initializedMembers.Keys)
{
// there can be only one static constructor
Expand Down Expand Up @@ -176,14 +168,13 @@ private static Dictionary<ISymbol, EqualsValueClauseSyntax> GetInitializedMember
return allMembers;
}

private static List<NodeSymbolAndModel<TSyntax, IMethodSymbol>> GetInitializerDeclarations<TSyntax>(SonarSyntaxNodeReportingContext context, List<IMethodSymbol> constructorSymbols)
private static List<NodeSymbolAndModel<TSyntax, IMethodSymbol>> GetConstructorDeclarations<TSyntax>(SonarSyntaxNodeReportingContext context, List<IMethodSymbol> constructorSymbols)
where TSyntax : SyntaxNode =>
constructorSymbols
.Select(x => new NodeSymbolAndModel<TSyntax, IMethodSymbol>(null, x.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax() as TSyntax, x))
.Where(x => x.Node != null)
.Select(x => new NodeSymbolAndModel<TSyntax, IMethodSymbol>(x.Node.EnsureCorrectSemanticModelOrDefault(context.SemanticModel), x.Node, x.Symbol))
.Where(x => x.Model != null)
.ToList();
(from constructorSymbol in constructorSymbols
from declarationNode in constructorSymbol.DeclaringSyntaxReferences.Select(x => x.GetSyntax()).OfType<TSyntax>()
let model = declarationNode.EnsureCorrectSemanticModelOrDefault(context.SemanticModel)
where model != null
select new NodeSymbolAndModel<TSyntax, IMethodSymbol>(model, declarationNode, constructorSymbol)).ToList();

private static IEnumerable<DeclarationTuple<IPropertySymbol>> GetInitializedPropertyDeclarations(TypeDeclarationSyntax declaration,
Func<SyntaxTokenList, bool> filterModifiers,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,36 @@
using System;
using System.Runtime.CompilerServices;
/// Reproducer for https://github.com/SonarSource/sonar-dotnet/issues/7624
class WithPrimaryConstructor(object options)
{
private readonly object _options = options; // Compliant
public object Options { get; } = options; // Compliant
public object[] AllOptions { get; } = [options]; // Compliant
}

class WithReferencedPrimaryConstructor(object options)
{
public WithReferencedPrimaryConstructor() : this(null)
{

}
private readonly object _options = options; // Compliant
}

/// Reproducer for https://github.com/SonarSource/sonar-dotnet/issues/7624
class SampleClass(object options)
class WithPrimaryConstructorAndAssignment(object options)
{
private readonly object _options = options; // Noncompliant FP
public WithPrimaryConstructorAndAssignment() : this(null)
{
_options = null;
}
private readonly object _options = options; // Compliant
}

class CollectionExpression1
{
private readonly int[] numbers = [1, 2, 3]; // Noncompliant
// ^^^^^^^^^^^

CollectionExpression1()
{
numbers = [4, 5, 6];
}
}

0 comments on commit 34d8bc8

Please sign in to comment.