diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/PrivateFieldUsedAsLocalVariable.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/PrivateFieldUsedAsLocalVariable.cs index 06ee00f2ef1..d48884c94e5 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/PrivateFieldUsedAsLocalVariable.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/PrivateFieldUsedAsLocalVariable.cs @@ -20,249 +20,254 @@ using SonarAnalyzer.Common.Walkers; -namespace SonarAnalyzer.Rules.CSharp +namespace SonarAnalyzer.Rules.CSharp; + +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class PrivateFieldUsedAsLocalVariable : SonarDiagnosticAnalyzer { - [DiagnosticAnalyzer(LanguageNames.CSharp)] - public sealed class PrivateFieldUsedAsLocalVariable : SonarDiagnosticAnalyzer + private const string DiagnosticId = "S1450"; + private const string MessageFormat = "Remove the field '{0}' and declare it as a local variable in the relevant methods."; + + private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, MessageFormat); + + private static readonly ISet NonPrivateModifiers = new HashSet { - private const string DiagnosticId = "S1450"; - private const string MessageFormat = "Remove the field '{0}' and declare it as a local variable in the relevant methods."; + SyntaxKind.PublicKeyword, + SyntaxKind.ProtectedKeyword, + SyntaxKind.InternalKeyword, + }; - private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, MessageFormat); + public override ImmutableArray SupportedDiagnostics { get; } = ImmutableArray.Create(Rule); - public override ImmutableArray SupportedDiagnostics { get; } = ImmutableArray.Create(Rule); + protected override void Initialize(SonarAnalysisContext context) => + context.RegisterNodeAction(c => + { + var typeDeclaration = (TypeDeclarationSyntax)c.Node; + if (c.IsRedundantPositionalRecordContext() || typeDeclaration.Modifiers.Any(SyntaxKind.PartialKeyword)) + { + return; + } - private static readonly ISet NonPrivateModifiers = new HashSet - { - SyntaxKind.PublicKeyword, - SyntaxKind.ProtectedKeyword, - SyntaxKind.InternalKeyword, - }; + var methodNames = typeDeclaration.Members.OfType().Select(x => x.Identifier.ValueText).ToHashSet(); + var privateFields = GetPrivateFields(c.SemanticModel, typeDeclaration); + var collector = new FieldAccessCollector(c.SemanticModel, privateFields, methodNames); + if (!collector.SafeVisit(typeDeclaration)) + { + // We couldn't finish the exploration so we cannot take any decision + return; + } - protected override void Initialize(SonarAnalysisContext context) => - context.RegisterNodeAction(c => + foreach (var fieldSymbol in privateFields.Keys.Where(collector.IsRemovableField)) { - var typeDeclaration = (TypeDeclarationSyntax)c.Node; - if (c.IsRedundantPositionalRecordContext() || typeDeclaration.Modifiers.Any(SyntaxKind.PartialKeyword)) - { - return; - } + c.ReportIssue(Rule, privateFields[fieldSymbol], fieldSymbol.Name); + } + }, + SyntaxKind.ClassDeclaration, + SyntaxKind.StructDeclaration, + SyntaxKindEx.RecordClassDeclaration, + SyntaxKindEx.RecordStructDeclaration); - var methodNames = typeDeclaration.Members.OfType().Select(x => x.Identifier.ValueText).ToHashSet(); - var privateFields = GetPrivateFields(c.SemanticModel, typeDeclaration); - var collector = new FieldAccessCollector(c.SemanticModel, privateFields, methodNames); - if (!collector.SafeVisit(typeDeclaration)) - { - // We couldn't finish the exploration so we cannot take any decision - return; - } + private static IDictionary GetPrivateFields(SemanticModel model, TypeDeclarationSyntax typeDeclaration) + { + return typeDeclaration.Members + .OfType() + .Where(IsPrivate) + .Where(HasNoAttributes) + .SelectMany(x => x.Declaration.Variables) + .ToDictionary( + x => (IFieldSymbol)model.GetDeclaredSymbol(x), + x => x); + + bool IsPrivate(FieldDeclarationSyntax fieldDeclaration) => + !fieldDeclaration.Modifiers.Select(x => x.Kind()).Any(NonPrivateModifiers.Contains); + + bool HasNoAttributes(FieldDeclarationSyntax fieldDeclaration) => + fieldDeclaration.AttributeLists.Count == 0; + } - foreach (var fieldSymbol in privateFields.Keys.Where(collector.IsRemovableField)) - { - c.ReportIssue(Rule, privateFields[fieldSymbol], fieldSymbol.Name); - } - }, - SyntaxKind.ClassDeclaration, - SyntaxKind.StructDeclaration, - SyntaxKindEx.RecordClassDeclaration, - SyntaxKindEx.RecordStructDeclaration); + private sealed class FieldAccessCollector : SafeCSharpSyntaxWalker + { + // Contains statements that READ field values. First grouped by field symbol (that is read), + // then by method/property/ctor symbol (that contains the statements) + private readonly Dictionary> readsByField = new(); - private static IDictionary GetPrivateFields(SemanticModel semanticModel, TypeDeclarationSyntax typeDeclaration) - { - return typeDeclaration.Members - .OfType() - .Where(IsPrivate) - .Where(HasNoAttributes) - .SelectMany(f => f.Declaration.Variables) - .ToDictionary( - variable => (IFieldSymbol)semanticModel.GetDeclaredSymbol(variable), - variable => variable); - - bool IsPrivate(FieldDeclarationSyntax fieldDeclaration) => - !fieldDeclaration.Modifiers.Select(m => m.Kind()).Any(NonPrivateModifiers.Contains); - - bool HasNoAttributes(FieldDeclarationSyntax fieldDeclaration) => - fieldDeclaration.AttributeLists.Count == 0; - } + // Contains statements that WRITE field values. First grouped by field symbol (that is written), + // then by method/property/ctor symbol (that contains the statements) + private readonly Dictionary> writesByField = new(); - private sealed class FieldAccessCollector : SafeCSharpSyntaxWalker - { - // Contains statements that READ field values. First grouped by field symbol (that is read), - // then by method/property/ctor symbol (that contains the statements) - private readonly Dictionary> readsByField = new(); + // Contains all method/property invocations grouped by the statement that contains them. + private readonly Lookup invocations = new(); - // Contains statements that WRITE field values. First grouped by field symbol (that is written), - // then by method/property/ctor symbol (that contains the statements) - private readonly Dictionary> writesByField = new(); + private readonly SemanticModel model; + private readonly IDictionary privateFields; - // Contains all method/property invocations grouped by the statement that contains them. - private readonly Lookup invocations = new(); + private readonly HashSet methodNames; - private readonly SemanticModel semanticModel; - private readonly IDictionary privateFields; + public FieldAccessCollector(SemanticModel model, IDictionary privateFields, HashSet methodNames) + { + this.model = model; + this.privateFields = privateFields; + this.methodNames = methodNames; + } - private readonly HashSet methodNames; + public bool IsRemovableField(IFieldSymbol fieldSymbol) + { + var writesByEnclosingSymbol = writesByField.GetValueOrDefault(fieldSymbol); + var readsByEnclosingSymbol = readsByField.GetValueOrDefault(fieldSymbol); - public FieldAccessCollector(SemanticModel semanticModel, IDictionary privateFields, HashSet methodNames) + // No methods overwrite the field value + if (writesByEnclosingSymbol is null) { - this.semanticModel = semanticModel; - this.privateFields = privateFields; - this.methodNames = methodNames; + return false; } - public bool IsRemovableField(IFieldSymbol fieldSymbol) - { - var writesByEnclosingSymbol = writesByField.GetValueOrDefault(fieldSymbol); - var readsByEnclosingSymbol = readsByField.GetValueOrDefault(fieldSymbol); - - // No methods overwrite the field value - if (writesByEnclosingSymbol == null) - { - return false; - } - - // A field is removable when no method reads it, or all methods that read it, overwrite it before reading - // However, as S4487 reports on fields that are written but not read, we only raise on the latter case - return readsByEnclosingSymbol?.Keys.All(ValueOverwrittenBeforeReading) ?? false; + // A field is removable when no method reads it, or all methods that read it, overwrite it before reading + // However, as S4487 reports on fields that are written but not read, we only raise on the latter case + return readsByEnclosingSymbol?.Keys.All(ValueOverwrittenBeforeReading) ?? false; - bool ValueOverwrittenBeforeReading(ISymbol enclosingSymbol) + bool ValueOverwrittenBeforeReading(ISymbol enclosingSymbol) + { + var writeStatements = writesByEnclosingSymbol.GetValueOrDefault(enclosingSymbol); + var readStatements = readsByEnclosingSymbol.GetValueOrDefault(enclosingSymbol); + + // Note that Enumerable.All() will return true if readStatements is empty. The collection + // will be empty if the field is read only in property/field initializers or returned from + // expression-bodied methods. + return writeStatements is not null && (readStatements?.All(IsPrecededWithWrite) ?? false); + + // Returns true when readStatement is preceded with a statement that overwrites fieldSymbol, + // or false when readStatement is preceded with an invocation of a method or property that + // overwrites fieldSymbol. + bool IsPrecededWithWrite(SyntaxNode readStatementOrArrowExpression) { - var writeStatements = writesByEnclosingSymbol.GetValueOrDefault(enclosingSymbol); - var readStatements = readsByEnclosingSymbol.GetValueOrDefault(enclosingSymbol); - - // Note that Enumerable.All() will return true if readStatements is empty. The collection - // will be empty if the field is read only in property/field initializers or returned from - // expression-bodied methods. - return writeStatements != null && (readStatements?.All(IsPrecededWithWrite) ?? false); - - // Returns true when readStatement is preceded with a statement that overwrites fieldSymbol, - // or false when readStatement is preceded with an invocation of a method or property that - // overwrites fieldSymbol. - bool IsPrecededWithWrite(SyntaxNode readStatementOrArrowExpression) + if (readStatementOrArrowExpression is StatementSyntax readStatement) { - if (readStatementOrArrowExpression is StatementSyntax readStatement) + foreach (var statement in readStatement.GetPreviousStatements()) { - foreach (var statement in readStatement.GetPreviousStatements()) + // When the readStatement is preceded with a write statement (that is also not a read statement) + // we want to report this field. + if (IsOverwritingValue(statement)) { - // When the readStatement is preceded with a write statement (that is also not a read statement) - // we want to report this field. - if (IsOverwritingValue(statement)) - { - return true; - } - - // When the readStatement is preceded with an invocation that has side effects, e.g. writes the field - // we don't want to report this field because it could be difficult or impossible to change the code. - if (IsInvocationWithSideEffects(statement)) - { - return false; - } + return true; + } + + // When the readStatement is preceded with an invocation that has side effects, e.g. writes the field + // we don't want to report this field because it could be difficult or impossible to change the code. + if (IsInvocationWithSideEffects(statement)) + { + return false; } } - // ArrowExpressionClauseSyntax cannot be preceded by anything... - return false; } + // ArrowExpressionClauseSyntax cannot be preceded by anything... + return false; + } - bool IsOverwritingValue(StatementSyntax statement) => - writeStatements.Contains(statement) - && !readStatements.Contains(statement); + bool IsOverwritingValue(StatementSyntax statement) => + writeStatements.Contains(statement) + && !readStatements.Contains(statement); - bool IsInvocationWithSideEffects(StatementSyntax statement) => - invocations.TryGetValue(statement, out var invocationsInStatement) - && invocationsInStatement.Any(writesByEnclosingSymbol.ContainsKey); - } + bool IsInvocationWithSideEffects(StatementSyntax statement) => + invocations.TryGetValue(statement, out var invocationsInStatement) + && invocationsInStatement.Any(writesByEnclosingSymbol.ContainsKey); } + } - public override void VisitIdentifierName(IdentifierNameSyntax node) + public override void VisitIdentifierName(IdentifierNameSyntax node) + { + if (privateFields.Keys.Any(x => x.Name == node.Identifier.ValueText) + || (node.Parent is not InvocationExpressionSyntax + && methodNames.Contains(node.Identifier.ValueText))) { - if (privateFields.Keys.Any(x => x.Name == node.Identifier.ValueText)) + var memberReference = GetTopmostSyntaxWithTheSameSymbol(node); + if (memberReference.Symbol is IFieldSymbol fieldSymbol && privateFields.ContainsKey(fieldSymbol)) { - var memberReference = GetTopmostSyntaxWithTheSameSymbol(node); - if (memberReference.Symbol is IFieldSymbol fieldSymbol - && privateFields.ContainsKey(fieldSymbol)) - { - ClassifyFieldReference(semanticModel.GetEnclosingSymbol(memberReference.Node.SpanStart), memberReference); - } + ClassifyFieldReference(model.GetEnclosingSymbol(memberReference.Node.SpanStart), memberReference); } - base.VisitIdentifierName(node); - } - - public override void VisitInvocationExpression(InvocationExpressionSyntax node) - { - if (node.GetMethodCallIdentifier() is { } methodCallIdentifier - && methodNames.Contains(methodCallIdentifier.ValueText) - && GetTopmostSyntaxWithTheSameSymbol(node) is var memberReference - && memberReference.Symbol is IMethodSymbol - && GetParentPseudoStatement(memberReference) is { } pseudoStatement) + else if (memberReference.Symbol is IMethodSymbol && GetParentPseudoStatement(memberReference) is { } pseudoStatement) { - invocations.GetOrAdd(pseudoStatement, _ => new HashSet()) - .Add(memberReference.Symbol); + // Adding method group to the invocation list + invocations.GetOrAdd(pseudoStatement, _ => new HashSet()).Add(memberReference.Symbol); } - base.VisitInvocationExpression(node); } - // A PseudoStatement is a Statement or an ArrowExpressionClauseSyntax (which denotes an expression-bodied member). - private static SyntaxNode GetParentPseudoStatement(NodeAndSymbol memberReference) => - memberReference.Node.Ancestors().FirstOrDefault(a => a is StatementSyntax or ArrowExpressionClauseSyntax); + base.VisitIdentifierName(node); + } - /// - /// Stores the statement that contains the provided field reference in one of the "reads" or "writes" collections, - /// first grouped by field symbol, then by containing method. - /// - private void ClassifyFieldReference(ISymbol enclosingSymbol, NodeAndSymbol fieldReference) + public override void VisitInvocationExpression(InvocationExpressionSyntax node) + { + if (node.GetMethodCallIdentifier() is { } methodCallIdentifier + && methodNames.Contains(methodCallIdentifier.ValueText) + && GetTopmostSyntaxWithTheSameSymbol(node) is var memberReference + && memberReference.Symbol is IMethodSymbol + && GetParentPseudoStatement(memberReference) is { } pseudoStatement) { - // It is important to create the field access HashSet regardless of the statement (see the local var below) - // being null or not, because the rule will not be able to detect field reads from inline property - // or field initializers. - var fieldAccessInMethod = (IsWrite(fieldReference) ? writesByField : readsByField) - .GetOrAdd((IFieldSymbol)fieldReference.Symbol, x => new Lookup()) - .GetOrAdd(enclosingSymbol, x => new HashSet()); - - var pseudoStatement = GetParentPseudoStatement(fieldReference); - if (pseudoStatement != null) - { - fieldAccessInMethod.Add(pseudoStatement); - } + invocations.GetOrAdd(pseudoStatement, _ => new HashSet()) + .Add(memberReference.Symbol); } + base.VisitInvocationExpression(node); + } - private static bool IsWrite(NodeAndSymbol fieldReference) + // A PseudoStatement is a Statement or an ArrowExpressionClauseSyntax (which denotes an expression-bodied member). + private static SyntaxNode GetParentPseudoStatement(NodeAndSymbol memberReference) => + memberReference.Node.Ancestors().FirstOrDefault(x => x is StatementSyntax or ArrowExpressionClauseSyntax); + + /// + /// Stores the statement that contains the provided field reference in one of the "reads" or "writes" collections, + /// first grouped by field symbol, then by containing method. + /// + private void ClassifyFieldReference(ISymbol enclosingSymbol, NodeAndSymbol fieldReference) + { + // It is important to create the field access HashSet regardless of the statement (see the local var below) + // being null or not, because the rule will not be able to detect field reads from inline property + // or field initializers. + var fieldAccessInMethod = (IsWrite(fieldReference) ? writesByField : readsByField) + .GetOrAdd((IFieldSymbol)fieldReference.Symbol, _ => new Lookup()) + .GetOrAdd(enclosingSymbol, _ => new HashSet()); + + var pseudoStatement = GetParentPseudoStatement(fieldReference); + if (pseudoStatement is not null) { - // If the field is not static and is not from the current instance we - // consider the reference as read. - if (!fieldReference.Symbol.IsStatic - && !(fieldReference.Node as ExpressionSyntax).RemoveParentheses().IsOnThis()) - { - return false; - } + fieldAccessInMethod.Add(pseudoStatement); + } + } - return IsLeftSideOfAssignment(fieldReference.Node) - || IsOutArgument(fieldReference.Node); + private static bool IsWrite(NodeAndSymbol fieldReference) + { + // If the field is not static and is not from the current instance we + // consider the reference as read. + if (!fieldReference.Symbol.IsStatic && !(fieldReference.Node as ExpressionSyntax).RemoveParentheses().IsOnThis()) + { + return false; + } - bool IsOutArgument(SyntaxNode syntaxNode) => - syntaxNode.Parent is ArgumentSyntax argument - && argument.RefOrOutKeyword.IsKind(SyntaxKind.OutKeyword); + return IsLeftSideOfAssignment(fieldReference.Node) + || IsOutArgument(fieldReference.Node); - bool IsLeftSideOfAssignment(SyntaxNode syntaxNode) => - syntaxNode.Parent.IsKind(SyntaxKind.SimpleAssignmentExpression) - && syntaxNode.Parent is AssignmentExpressionSyntax assignmentExpression - && assignmentExpression.Left == syntaxNode; - } + bool IsOutArgument(SyntaxNode node) => + node.Parent is ArgumentSyntax argument + && argument.RefOrOutKeyword.IsKind(SyntaxKind.OutKeyword); - private NodeAndSymbol GetTopmostSyntaxWithTheSameSymbol(SyntaxNode identifier) => - // All of the cases below could be parts of invocation or other expressions - identifier.Parent switch - { - // this.identifier or a.identifier or ((a)).identifier, but not identifier.other - MemberAccessExpressionSyntax memberAccess when memberAccess.Name == identifier => - new NodeAndSymbol(memberAccess.GetSelfOrTopParenthesizedExpression(), semanticModel.GetSymbolInfo(memberAccess).Symbol), - // this?.identifier or a?.identifier or ((a))?.identifier, but not identifier?.other - MemberBindingExpressionSyntax memberBinding when memberBinding.Name == identifier => - new NodeAndSymbol(memberBinding.Parent.GetSelfOrTopParenthesizedExpression(), semanticModel.GetSymbolInfo(memberBinding).Symbol), - // identifier or ((identifier)) - _ => new NodeAndSymbol(identifier.GetSelfOrTopParenthesizedExpression(), semanticModel.GetSymbolInfo(identifier).Symbol) - }; + bool IsLeftSideOfAssignment(SyntaxNode node) => + node.Parent.IsKind(SyntaxKind.SimpleAssignmentExpression) + && node.Parent is AssignmentExpressionSyntax assignmentExpression + && assignmentExpression.Left == node; } - private sealed class Lookup : Dictionary> { } + private NodeAndSymbol GetTopmostSyntaxWithTheSameSymbol(SyntaxNode node) => + // All of the cases below could be parts of invocation or other expressions + node.Parent switch + { + // this.identifier or a.identifier or ((a)).identifier, but not identifier.other + MemberAccessExpressionSyntax memberAccess when memberAccess.Name == node => + new NodeAndSymbol(memberAccess.GetSelfOrTopParenthesizedExpression(), model.GetSymbolInfo(memberAccess).Symbol), + // this?.identifier or a?.identifier or ((a))?.identifier, but not identifier?.other + MemberBindingExpressionSyntax memberBinding when memberBinding.Name == node => + new NodeAndSymbol(memberBinding.Parent.GetSelfOrTopParenthesizedExpression(), model.GetSymbolInfo(memberBinding).Symbol), + // identifier or ((identifier)) + _ => new NodeAndSymbol(node.GetSelfOrTopParenthesizedExpression(), model.GetSymbolInfo(node).Symbol) + }; } + + private sealed class Lookup : Dictionary> { } } diff --git a/analyzers/tests/SonarAnalyzer.Test/Rules/PrivateFieldUsedAsLocalVariableTest.cs b/analyzers/tests/SonarAnalyzer.Test/Rules/PrivateFieldUsedAsLocalVariableTest.cs index 795e4a8cdf2..0df059baa1f 100644 --- a/analyzers/tests/SonarAnalyzer.Test/Rules/PrivateFieldUsedAsLocalVariableTest.cs +++ b/analyzers/tests/SonarAnalyzer.Test/Rules/PrivateFieldUsedAsLocalVariableTest.cs @@ -20,33 +20,32 @@ using SonarAnalyzer.Rules.CSharp; -namespace SonarAnalyzer.Test.Rules +namespace SonarAnalyzer.Test.Rules; + +[TestClass] +public class PrivateFieldUsedAsLocalVariableTest { - [TestClass] - public class PrivateFieldUsedAsLocalVariableTest - { - private readonly VerifierBuilder builder = new VerifierBuilder(); + private readonly VerifierBuilder builder = new VerifierBuilder(); - [TestMethod] - public void PrivateFieldUsedAsLocalVariable() => - builder.AddPaths("PrivateFieldUsedAsLocalVariable.cs") - .Verify(); + [TestMethod] + public void PrivateFieldUsedAsLocalVariable() => + builder.AddPaths("PrivateFieldUsedAsLocalVariable.cs") + .Verify(); #if NET - [TestMethod] - public void PrivateFieldUsedAsLocalVariable_CSharp9() => - builder.AddPaths("PrivateFieldUsedAsLocalVariable.CSharp9.cs") - .WithOptions(ParseOptionsHelper.FromCSharp9) - .Verify(); + [TestMethod] + public void PrivateFieldUsedAsLocalVariable_CSharp9() => + builder.AddPaths("PrivateFieldUsedAsLocalVariable.CSharp9.cs") + .WithOptions(ParseOptionsHelper.FromCSharp9) + .Verify(); - [TestMethod] - public void PrivateFieldUsedAsLocalVariable_CSharp10() => - builder.AddPaths("PrivateFieldUsedAsLocalVariable.CSharp10.cs") - .WithOptions(ParseOptionsHelper.FromCSharp10) - .Verify(); + [TestMethod] + public void PrivateFieldUsedAsLocalVariable_CSharp10() => + builder.AddPaths("PrivateFieldUsedAsLocalVariable.CSharp10.cs") + .WithOptions(ParseOptionsHelper.FromCSharp10) + .Verify(); #endif - } } diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/PrivateFieldUsedAsLocalVariable.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/PrivateFieldUsedAsLocalVariable.cs index 8d3d210de10..e1f67028779 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/PrivateFieldUsedAsLocalVariable.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/PrivateFieldUsedAsLocalVariable.cs @@ -25,6 +25,7 @@ void M1() private int F5 = 0; // Noncompliant private int F6; // Noncompliant + void M2() { F5 = 42; @@ -239,6 +240,36 @@ void M17() SetValueInProperty = 42; Use(PropertyWithSideEffect); } + + private int F38 = 21; // Compliant - unread + + void M18() + { + F38 = 42; + } + + struct SomeStruct + { + public int Field; + } + + private SomeStruct F39; // Noncompliant + private SomeStruct? F40 = null; // Noncompliant + + void M19() + { + F39 = new SomeStruct() { Field = 42 }; + F39.Field = 42; + } + + void M20() + { + F40 = new SomeStruct() { Field = 42 }; + if (F40 != null) + { + Console.WriteLine(F40?.Field); + } + } } public partial class SomePartialClass @@ -360,18 +391,21 @@ public event EventHandler E1 public void M15(int i) => F36 = i + 1; } + public class Broker + { + public event EventHandler Receive; + public void Process() { Receive?.Invoke(this, EventArgs.Empty); } + } + // https://github.com/SonarSource/sonar-dotnet/issues/8239 - public class Repo_8239 + public class Repro_8239 { - private bool _received; // Noncompliant FP + private bool _received; // Noncompliant - public void Program() + public void Program(Broker broker) { - var broker = new Broker(); broker.Receive += Broker_Receive; // Broker_Receive should be treated as "public" as it is passed as a delegate - - _received = false; - + _received = false; // This is not compliant because this line is after registering the event broker.Process(); if (_received) @@ -384,17 +418,33 @@ private void Broker_Receive(object sender, EventArgs e) { _received = true; } + } + + public class Repro_8239_Compliant + { + private bool _received; - public class Broker + public void Program(Broker broker) { - public event EventHandler Receive; - public void Process() { Receive?.Invoke(this, EventArgs.Empty); } + _received = false; + broker.Receive += Broker_Receive; + broker.Process(); + + if (_received) + { + Console.WriteLine("OK"); + } + } + + private void Broker_Receive(object sender, EventArgs e) + { + _received = true; } } public class Repo_8239_Variation { - private bool _wasCalled; // Noncompliant FP + private bool _wasCalled; public void Program() { @@ -402,7 +452,6 @@ public void Program() _wasCalled = false; list.ForEach(Increment); // Increment is passed as a delegate `new Action(Increment)` and should be treated as "public" - // list.ForEach(x => Increment(x)); // This is detected correctly if (_wasCalled) { @@ -412,6 +461,25 @@ public void Program() private void Increment(int dummy) => _wasCalled = true; + } + + public class Repo_8239_Variation_Noncompliant + { + private bool _wasCalled; // Noncompliant + + public void Program() + { + var list = new List(); + list.ForEach(x => Increment(x)); + _wasCalled = false; + + if (_wasCalled) + { + Console.WriteLine("OK"); + } + } + private void Increment(int dummy) => + _wasCalled = true; } }