From 60dc3c587d91702e1ef99ed158c23e011b27779f Mon Sep 17 00:00:00 2001 From: Zsolt Kolbay <121798625+zsolt-kolbay-sonarsource@users.noreply.github.com> Date: Fri, 31 Mar 2023 15:01:37 +0200 Subject: [PATCH] S3900: Change parameter dereference check to top-down (#7004) --- .../expected/Nancy/Nancy--net452-S3900.json | 26 ---------- .../Nancy/Nancy--netstandard2.0-S3900.json | 26 ---------- ...cy.ViewEngines.Markdown--net452-S3900.json | 13 ----- ...ngines.Markdown--netstandard2.0-S3900.json | 13 ----- ...hared.Internals--netstandard2.0-S3900.json | 13 ----- ...icMethodArgumentsShouldBeCheckedForNull.cs | 49 +++++++++---------- ...icMethodArgumentsShouldBeCheckedForNull.cs | 23 +++++++++ 7 files changed, 47 insertions(+), 116 deletions(-) diff --git a/analyzers/its/expected/Nancy/Nancy--net452-S3900.json b/analyzers/its/expected/Nancy/Nancy--net452-S3900.json index 59e57838444..2e1914ceb51 100644 --- a/analyzers/its/expected/Nancy/Nancy--net452-S3900.json +++ b/analyzers/its/expected/Nancy/Nancy--net452-S3900.json @@ -2828,19 +2828,6 @@ "uri": "sources\Nancy\src\Nancy\Routing\Constraints\ParameterizedRouteSegmentConstraintBase.cs", "region": { "startLine": 21, -"startColumn": 89, -"endLine": 21, -"endColumn": 99 -} -} -}, -{ -"id": "S3900", -"message": "Refactor this method to add validation of parameter 'constraint' before using it.", -"location": { -"uri": "sources\Nancy\src\Nancy\Routing\Constraints\ParameterizedRouteSegmentConstraintBase.cs", -"region": { -"startLine": 21, "startColumn": 113, "endLine": 21, "endColumn": 123 @@ -2854,19 +2841,6 @@ "uri": "sources\Nancy\src\Nancy\Routing\Constraints\ParameterizedRouteSegmentConstraintBase.cs", "region": { "startLine": 35, -"startColumn": 30, -"endLine": 35, -"endColumn": 40 -} -} -}, -{ -"id": "S3900", -"message": "Refactor this method to add validation of parameter 'constraint' before using it.", -"location": { -"uri": "sources\Nancy\src\Nancy\Routing\Constraints\ParameterizedRouteSegmentConstraintBase.cs", -"region": { -"startLine": 35, "startColumn": 51, "endLine": 35, "endColumn": 61 diff --git a/analyzers/its/expected/Nancy/Nancy--netstandard2.0-S3900.json b/analyzers/its/expected/Nancy/Nancy--netstandard2.0-S3900.json index 4a0d6dd1ccd..00cff75486c 100644 --- a/analyzers/its/expected/Nancy/Nancy--netstandard2.0-S3900.json +++ b/analyzers/its/expected/Nancy/Nancy--netstandard2.0-S3900.json @@ -2828,19 +2828,6 @@ "uri": "sources\Nancy\src\Nancy\Routing\Constraints\ParameterizedRouteSegmentConstraintBase.cs", "region": { "startLine": 21, -"startColumn": 89, -"endLine": 21, -"endColumn": 99 -} -} -}, -{ -"id": "S3900", -"message": "Refactor this method to add validation of parameter 'constraint' before using it.", -"location": { -"uri": "sources\Nancy\src\Nancy\Routing\Constraints\ParameterizedRouteSegmentConstraintBase.cs", -"region": { -"startLine": 21, "startColumn": 113, "endLine": 21, "endColumn": 123 @@ -2854,19 +2841,6 @@ "uri": "sources\Nancy\src\Nancy\Routing\Constraints\ParameterizedRouteSegmentConstraintBase.cs", "region": { "startLine": 35, -"startColumn": 30, -"endLine": 35, -"endColumn": 40 -} -} -}, -{ -"id": "S3900", -"message": "Refactor this method to add validation of parameter 'constraint' before using it.", -"location": { -"uri": "sources\Nancy\src\Nancy\Routing\Constraints\ParameterizedRouteSegmentConstraintBase.cs", -"region": { -"startLine": 35, "startColumn": 51, "endLine": 35, "endColumn": 61 diff --git a/analyzers/its/expected/Nancy/Nancy.ViewEngines.Markdown--net452-S3900.json b/analyzers/its/expected/Nancy/Nancy.ViewEngines.Markdown--net452-S3900.json index 9dc1cfdc922..c949039b664 100644 --- a/analyzers/its/expected/Nancy/Nancy.ViewEngines.Markdown--net452-S3900.json +++ b/analyzers/its/expected/Nancy/Nancy.ViewEngines.Markdown--net452-S3900.json @@ -32,19 +32,6 @@ "location": { "uri": "sources\Nancy\src\Nancy.ViewEngines.Markdown\MarkdownViewengineRender.cs", "region": { -"startLine": 31, -"startColumn": 16, -"endLine": 31, -"endColumn": 31 -} -} -}, -{ -"id": "S3900", -"message": "Refactor this method to add validation of parameter 'templateContent' before using it.", -"location": { -"uri": "sources\Nancy\src\Nancy.ViewEngines.Markdown\MarkdownViewengineRender.cs", -"region": { "startLine": 32, "startColumn": 20, "endLine": 32, diff --git a/analyzers/its/expected/Nancy/Nancy.ViewEngines.Markdown--netstandard2.0-S3900.json b/analyzers/its/expected/Nancy/Nancy.ViewEngines.Markdown--netstandard2.0-S3900.json index 9dc1cfdc922..c949039b664 100644 --- a/analyzers/its/expected/Nancy/Nancy.ViewEngines.Markdown--netstandard2.0-S3900.json +++ b/analyzers/its/expected/Nancy/Nancy.ViewEngines.Markdown--netstandard2.0-S3900.json @@ -32,19 +32,6 @@ "location": { "uri": "sources\Nancy\src\Nancy.ViewEngines.Markdown\MarkdownViewengineRender.cs", "region": { -"startLine": 31, -"startColumn": 16, -"endLine": 31, -"endColumn": 31 -} -} -}, -{ -"id": "S3900", -"message": "Refactor this method to add validation of parameter 'templateContent' before using it.", -"location": { -"uri": "sources\Nancy\src\Nancy.ViewEngines.Markdown\MarkdownViewengineRender.cs", -"region": { "startLine": 32, "startColumn": 20, "endLine": 32, diff --git a/analyzers/its/expected/akka.net/Akka.Tests.Shared.Internals--netstandard2.0-S3900.json b/analyzers/its/expected/akka.net/Akka.Tests.Shared.Internals--netstandard2.0-S3900.json index bcb30efbf40..0f61037c21f 100644 --- a/analyzers/its/expected/akka.net/Akka.Tests.Shared.Internals--netstandard2.0-S3900.json +++ b/analyzers/its/expected/akka.net/Akka.Tests.Shared.Internals--netstandard2.0-S3900.json @@ -137,19 +137,6 @@ "uri": "sources\akka.net\src\core\Akka.Tests.Shared.Internals\AkkaSpecExtensions.cs", "region": { "startLine": 188, -"startColumn": 26, -"endLine": 188, -"endColumn": 27 -} -} -}, -{ -"id": "S3900", -"message": "Refactor this method to add validation of parameter 's' before using it.", -"location": { -"uri": "sources\akka.net\src\core\Akka.Tests.Shared.Internals\AkkaSpecExtensions.cs", -"region": { -"startLine": 188, "startColumn": 50, "endLine": 188, "endColumn": 51 diff --git a/analyzers/src/SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs b/analyzers/src/SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs index 1c8fa439812..906a92ab05f 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs @@ -30,17 +30,6 @@ public class PublicMethodArgumentsShouldBeCheckedForNull : SymbolicRuleCheck private const string DiagnosticId = "S3900"; private const string MessageFormat = "{0}"; - private static readonly OperationKind[] DereferenceOperations = new[] - { - OperationKindEx.Invocation, - OperationKindEx.FieldReference, - OperationKindEx.PropertyReference, - OperationKindEx.EventReference, - OperationKindEx.Await, - OperationKindEx.ArrayElementReference, - OperationKindEx.MethodReference - }; - internal static readonly DiagnosticDescriptor S3900 = DescriptorFactory.Create(DiagnosticId, MessageFormat); protected override DiagnosticDescriptor Rule => S3900; @@ -82,30 +71,40 @@ static bool MethodDereferencesArguments(BaseMethodDeclarationSyntax method) protected override ProgramState PreProcessSimple(SymbolicContext context) { - var operation = context.Operation.Instance; - if (operation.Kind == OperationKindEx.ParameterReference - && operation.ToParameterReference().Parameter is var parameter - && !parameter.Type.IsValueType - && IsParameterDereferenced(context.Operation) - && NullableStateIsNotKnownForParameter(parameter) - && !parameter.HasAttribute(KnownType.Microsoft_AspNetCore_Mvc_FromServicesAttribute)) + if (NullDereferenceCandidate(context.Operation.Instance) is { } candidate + && candidate.Kind == OperationKindEx.ParameterReference + && candidate.ToParameterReference() is var parameterReference + && !parameterReference.Parameter.Type.IsValueType + && !HasObjectConstraint(parameterReference.Parameter) + && !parameterReference.Parameter.HasAttribute(KnownType.Microsoft_AspNetCore_Mvc_FromServicesAttribute)) { var message = SemanticModel.GetDeclaredSymbol(Node).IsConstructor() ? "Refactor this constructor to avoid using members of parameter '{0}' because it could be null." : "Refactor this method to add validation of parameter '{0}' before using it."; - ReportIssue(operation, string.Format(message, operation.Syntax), context); + ReportIssue(parameterReference.WrappedOperation, string.Format(message, parameterReference.WrappedOperation.Syntax), context); } return context.State; - bool NullableStateIsNotKnownForParameter(IParameterSymbol symbol) => - context.State[symbol] is null || !context.State[symbol].HasConstraint(); + bool HasObjectConstraint(IParameterSymbol symbol) => + context.State[symbol]?.HasConstraint() is true; } - private static bool IsParameterDereferenced(IOperationWrapperSonar operation) => - operation.Parent is { } parent - && (parent.IsAnyKind(DereferenceOperations) - || (parent.Kind == OperationKindEx.Conversion && IsParameterDereferenced(parent.ToSonar()))); + private static IOperation NullDereferenceCandidate(IOperation operation) + { + var candidate = operation.Kind switch + { + OperationKindEx.Invocation => operation.ToInvocation().Instance, + OperationKindEx.FieldReference => operation.ToFieldReference().Instance, + OperationKindEx.PropertyReference => operation.ToPropertyReference().Instance, + OperationKindEx.EventReference => operation.ToEventReference().Instance, + OperationKindEx.Await => operation.ToAwait().Operation, + OperationKindEx.ArrayElementReference => operation.ToArrayElementReference().ArrayReference, + OperationKindEx.MethodReference => operation.ToMethodReference().Instance, + _ => null, + }; + return candidate?.UnwrapConversion(); + } private sealed class ArgumentDereferenceWalker : SafeCSharpSyntaxWalker { diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs index 9133fb13934..4b5b0c84311 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs @@ -722,3 +722,26 @@ private class CustomClass public event EventHandler CustomEvent; } } + +public class DereferencedMultipleTimesOnTheSameExecutionPath +{ + public void Used(string s) + { + s.ToLower(); // Noncompliant + s.ToLower(); // Compliant - s was dereferenced in the previous line, so it's not null here + } + + public void UsedOnMultipleLevels(string s) + { + s.Replace( // Compliant - SubString was already called on s when we call Replace, so s is not null + "a", + s.Substring(1)); // Noncompliant + } + + public void UsedTwiceOnSameLevel(string s) + { + _ = s.Substring( // Compliant + s.IndexOf("a"), // Noncompliant + s.IndexOf("b")); // Compliant - IndexOf("a") was called before this method call, so s is not null here + } +}