Fix S2234 FN nullable argument value #3880

H4pH · 2020-12-30T10:53:38Z

Fixes #3879

pavel-mikula-sonarsource

Hi @H4pH, thank you for the contribution. I left some comments to address.

analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ParametersCorrectOrder.cs

analyzers/src/SonarAnalyzer.CSharp/Rules/ParametersCorrectOrder.cs

pavel-mikula-sonarsource

Final polishing

analyzers/src/SonarAnalyzer.VisualBasic/Rules/ParametersCorrectOrder.cs

analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ParametersCorrectOrder.cs

pavel-mikula-sonarsource · 2021-02-01T12:26:37Z

/azp run Sonar.Net.External

azure-pipelines · 2021-02-01T12:26:46Z

Azure Pipelines successfully started running 1 pipeline(s).

pavel-mikula-sonarsource · 2021-02-01T15:37:40Z

Hi @H4pH, CI run failed on .NET Integration tests and that's actually a good thing - new issue was found in akka.net project. To address this situation, please:

run analyzers\its\regression-test.ps1 -ruleId S2234 -project akka.net
run analyzers\its\update-expected.ps1 -project akka.net
commit the json file that will be added/changed

H4pH · 2021-02-02T09:11:33Z

Hi @pavel-mikula-sonarsource, I tried running your script but it fails when processing the results:

================================================
Initializing the environment
================================================
Initializing the actual issues folder with the expected result
Removing existing folder 'actual'
Initializing the output folder
Removing existing folder 'output'
Running ITs with only rule S2234 turned on.
The rule set we use is .\output\AllSonarAnalyzerRules.ruleset.
Build skipped: AnalyzeGenerated
Build skipped: AnalyzeGeneratedVb

================================================
Restoring NuGet packages for Akka.sln
================================================
Trying to find 'msbuild.exe' using 'MSBUILD_PATH' environment variable
Using Msbuild from 'C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\MSBuild\Current\Bin'.
Все пакеты, перечисленные в packages.config, уже установлены.
Using Msbuild from 'C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\MSBuild\Current\Bin'.
Все пакеты, перечисленные в packages.config, уже установлены.

================================================
Building solution Akka.sln
================================================
Trying to find 'msbuild.exe' using 'MSBUILD_PATH' environment variable
Microsoft (R) Build Engine версии 16.8.3+39993bd9d для .NET Framework
(C) Корпорация Майкрософт (Microsoft Corporation). Все права защищены.


Сборка успешно завершена.
    Предупреждений: 145
    Ошибок: 0

Прошло времени 00:00:50.33
Microsoft (R) Build Engine версии 16.8.3+39993bd9d для .NET Framework
(C) Корпорация Майкрософт (Microsoft Corporation). Все права защищены.


Сборка успешно завершена.
    Предупреждений: 145
    Ошибок: 0

Прошло времени 00:00:50.33
Build skipped: Automapper
Build skipped: Ember-MM
Build skipped: ManuallyAddedNoncompliantIssues
Build skipped: ManuallyAddedNoncompliantIssuesVB
Build skipped: Nancy
Build skipped: SkipGenerated
Build skipped: SkipGeneratedVb
Build skipped: NetCore31
Build skipped: Net5

================================================
Processing analyzer results
================================================
Check differences for internal projects
Normalizing the SARIF reports
Could not load type 'System.Web.UI.WebResourceAttribute' from assembly 'System.Web, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'.
System.TypeLoadException: Could not load type 'System.Web.UI.WebResourceAttribute' from assembly 'System.Web, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'.
   at System.ModuleHandle.ResolveType(QCallModule module, Int32 typeToken, IntPtr* typeInstArgs, Int32 typeInstCount, IntPtr* methodInstArgs, Int32 methodInstCount, ObjectHandleOnStack type)
   at System.ModuleHandle.ResolveTypeHandleInternal(RuntimeModule module, Int32 typeToken, RuntimeTypeHandle[] typeInstantiationContext, RuntimeTypeHandle[] methodInstantiationContext)
   at System.Reflection.RuntimeModule.ResolveType(Int32 metadataToken, Type[] genericTypeArguments, Type[] genericMethodArguments)
   at System.Reflection.CustomAttribute.FilterCustomAttributeRecord(MetadataToken caCtorToken, MetadataImport& scope, RuntimeModule decoratedModule, MetadataToken decoratedToken, RuntimeType attributeFilterType, Boolean mustBeInheritable, ListBuilder`1& derivedAttributes, RuntimeType& attributeType, IRuntimeMethodInfo& ctorWithParameters, Boolean& isVarArg)
   at System.Reflection.CustomAttribute.AddCustomAttributes(ListBuilder`1& attributes, RuntimeModule decoratedModule, Int32 decoratedMetadataToken, RuntimeType attributeFilterType, Boolean mustBeInheritable, ListBuilder`1 derivedAttributes)
   at System.Reflection.CustomAttribute.GetCustomAttributes(RuntimeModule decoratedModule, Int32 decoratedMetadataToken, Int32 pcaCount, RuntimeType attributeFilterType)
   at System.Reflection.CustomAttribute.GetCustomAttributes(RuntimeAssembly assembly, RuntimeType caType)
   at System.Reflection.RuntimeAssembly.GetCustomAttributes(Type attributeType, Boolean inherit)
   at System.Attribute.GetCustomAttributes(Assembly element, Type attributeType, Boolean inherit)
   at System.Reflection.CustomAttributeExtensions.GetCustomAttributes(Assembly element, Type attributeType)
   at System.Management.Automation.Language.PSInvokeMemberBinder.InvokeDotNetMethod(CallInfo callInfo, String name, PSMethodInvocationConstraints psMethodInvocationConstraints, MethodInvocationType methodInvocationType, DynamicMetaObject target, DynamicMetaObject[] args, BindingRestrictions restrictions, MethodInformation[] mi, Type errorExceptionType)
   at System.Management.Automation.Language.PSInvokeMemberBinder.FallbackInvokeMember(DynamicMetaObject target, DynamicMetaObject[] args, DynamicMetaObject errorSuggestion)
   at System.Dynamic.InvokeMemberBinder.FallbackInvokeMember(DynamicMetaObject target, DynamicMetaObject[] args)
   at System.Management.Automation.PSObject.PSDynamicMetaObject.BindInvokeMember(InvokeMemberBinder binder, DynamicMetaObject[] args)
   at System.Dynamic.InvokeMemberBinder.Bind(DynamicMetaObject target, DynamicMetaObject[] args)
   at System.Dynamic.DynamicMetaObjectBinder.Bind(Object[] args, ReadOnlyCollection`1 parameters, LabelTarget returnLabel)
   at System.Runtime.CompilerServices.CallSiteBinder.BindCore[T](CallSite`1 site, Object[] args)
   at System.Dynamic.UpdateDelegates.UpdateAndExecute2[T0,T1,TRet](CallSite site, T0 arg0, T1 arg1)
   at System.Management.Automation.Interpreter.DynamicInstruction`3.Run(InterpretedFrame frame)
   at System.Management.Automation.Interpreter.EnterTryCatchFinallyInstruction.Run(InterpretedFrame frame)
at GetActualIssues, C:\Users\User\Documents\GitHub\sonar-dotnet-local\analyzers\its\create-issue-reports.ps1: line 65
at New-IssueReports, C:\Users\User\Documents\GitHub\sonar-dotnet-local\analyzers\its\create-issue-reports.ps1: line 122
at <ScriptBlock>, C:\Users\User\Documents\GitHub\sonar-dotnet-local\analyzers\its\regression-test.ps1: line 505
at <ScriptBlock>, C:\Users\User\Documents\GitHub\sonar-dotnet-local\analyzers\its\regression-test.ps1: line 505
at <ScriptBlock>, <No file>: line 1

pavel-mikula-sonarsource · 2021-02-02T10:13:13Z

@H4pH, Ah OK, please address the comments and I'll run it afterwards and provide a commit to deal with it.

pavel-mikula-sonarsource · 2021-07-26T08:55:22Z

Hi @H4pH,

will you have time to finish this PR? It was close to being merged.

H4pH · 2021-09-13T16:49:06Z

Hi @H4pH,

will you have time to finish this PR? It was close to being merged.

Hi, @pavel-mikula-sonarsource,
finally I've found a time and was able to handle all test cases, let's finish with this

pavel-mikula-sonarsource · 2021-09-24T14:58:28Z

Hi @H4pH, thanks for taking a look. I'll try to check it next week. Can you please rebase your PR meanwhile? There's a merge conflict.

H4pH · 2021-09-28T18:47:14Z

Hi @H4pH, thanks for taking a look. I'll try to check it next week. Can you please rebase your PR meanwhile? There's a merge conflict.

i've done it, but now github shows all commits from master as PR commits :(

pavel-mikula-sonarsource · 2021-11-15T13:53:34Z

Something went wrong, it looks that you've merged master into your branch instead of rebasing on top of `master. This should fix it:

Checkout S2234-fn-nullable-argument-value
In VS Git Repository tab: right clickon master branch and choose Rebase S2234-fn-nullable-argument-value onto master
Solve rebase conflicts
Push changes.

In local commit history, you should see your PR commits on top of master tag.

H4pH · 2022-01-10T05:00:36Z

Hi @pavel-mikula-sonarsource, congratulations on the first year of my pull request :)
Are there any more changes required?

pavel-mikula-sonarsource

Hi @H4pH, I'll try to prioritize this PR now.

pavel-mikula-sonarsource · 2022-01-21T11:09:39Z

analyzers/src/SonarAnalyzer.Common/Rules/ParametersCorrectOrderBase.cs

+            bool IdentifierWithSameNameAndTypeExistsLater(ArgumentIdentifier argumentIdentifier, int index) =>
+                argumentIdentifiers.Skip(index + 1)
+                                   .Any(ia => string.Equals(ia.IdentifierName, argumentIdentifier.IdentifierName, StringComparison.OrdinalIgnoreCase)
+                                                              && ArgumentTypesAreSame(ia.ArgumentSyntax, argumentIdentifier.ArgumentSyntax));


What was the motivation for having this method? AFAICT it prevents the issue from being raised twice when arguments are swapped. For sake of simplicity, I'd say it's fine to raise twice and we can remove this check.

Without this method there is different result for same test. Example is passing one var to both params, see TestCases\ParametersCorrectOrder.cs line 97 and 101. (pass "a" and "b" to both params of m(int a, int b))
I think, that if argument was passed at correct place once, we shouldn't rise

Ok, that makes sense. And at the same time, if b was passed to both, we would have an equal scenario and we would raise and we shouldn't, right?
So we need to know if the argument was passed on the correct location at least once (no matter if before or after). This sounds complicated to do in a clean way to me. To keep this PR focused, I would be fine with creating a new issue for that case, fix it in another PR and here just document those two cases m(a, a) and m(b, b) as ' Noncompliant FP with a link to that new issue.

pavel-mikula-sonarsource · 2022-01-21T11:24:47Z

analyzers/src/SonarAnalyzer.CSharp/Rules/ParametersCorrectOrder.cs

+        private static SyntaxToken? GetValueAccessIdentifier(MemberAccessExpressionSyntax expression, SyntaxNodeAnalysisContext syntaxNodeAnalysisContext) =>
+            expression.Name.ToString() == "Value" && IsNullableValueAccess(expression, syntaxNodeAnalysisContext)
+                ? GetExpressionSyntaxIdentifier(expression.Expression, syntaxNodeAnalysisContext)
+                : expression.Name.Identifier;
+
+        private static bool IsNullableValueAccess(MemberAccessExpressionSyntax expression, SyntaxNodeAnalysisContext syntaxNodeAnalysisContext)
+        {
+            var typeInfo = syntaxNodeAnalysisContext.SemanticModel.GetTypeInfo(expression.Expression);
+            var type = typeInfo.ConvertedType;
+            var nullableT = syntaxNodeAnalysisContext.Compilation.GetTypeByMetadataName(typeof(Nullable<>).FullName);
+            return type.OriginalDefinition.DerivesOrImplements(nullableT);
+        }


This can be simplified a lot using the KnownType.

You actually don't need SyntaxNodeAnalysisContext and using SemanticModel model everywhere is enough.

Suggested change

private static SyntaxToken? GetValueAccessIdentifier(MemberAccessExpressionSyntax expression, SyntaxNodeAnalysisContext syntaxNodeAnalysisContext) =>

expression.Name.ToString() == "Value" && IsNullableValueAccess(expression, syntaxNodeAnalysisContext)

? GetExpressionSyntaxIdentifier(expression.Expression, syntaxNodeAnalysisContext)

: expression.Name.Identifier;

private static bool IsNullableValueAccess(MemberAccessExpressionSyntax expression, SyntaxNodeAnalysisContext syntaxNodeAnalysisContext)

{

var typeInfo = syntaxNodeAnalysisContext.SemanticModel.GetTypeInfo(expression.Expression);

var type = typeInfo.ConvertedType;

var nullableT = syntaxNodeAnalysisContext.Compilation.GetTypeByMetadataName(typeof(Nullable<>).FullName);

return type.OriginalDefinition.DerivesOrImplements(nullableT);

}

private static SyntaxToken? GetValueAccessIdentifier(MemberAccessExpressionSyntax expression, SemanticModel model) =>

expression.Name.ToString() == "Value" && model.GetTypeInfo(expression.Expression).ConvertedType.DerivesOrImplements(KnownType.System_Nullable_T)

? GetExpressionSyntaxIdentifier(expression.Expression, model)

: expression.Name.Identifier;

Same for VB

pavel-mikula-sonarsource · 2022-01-21T11:29:09Z

/azp run Sonar.Net

azure-pipelines · 2022-01-21T11:29:25Z

Azure Pipelines successfully started running 1 pipeline(s).

sonarcloud · 2022-01-21T11:46:59Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

sonarcloud · 2022-01-21T11:50:21Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
7 Code Smells

94.8% Coverage
13.3% Duplication

pavel-mikula-sonarsource · 2022-01-21T12:07:58Z

IT results have changed, please rerun the PowerShell scripts again to update the JSON as described above after your changes.

pavel-mikula-sonarsource · 2022-01-21T12:09:29Z

analyzers/src/SonarAnalyzer.CSharp/Rules/ParametersCorrectOrder.cs

@@ -87,10 +88,32 @@ protected override void Initialize(SonarAnalysisContext context)
        protected override Location GetMethodDeclarationIdentifierLocation(SyntaxNode syntaxNode) =>
            (syntaxNode as BaseMethodDeclarationSyntax)?.FindIdentifierLocation();

-        protected override SyntaxToken? GetArgumentIdentifier(ArgumentSyntax argument) =>
-            (argument.Expression as IdentifierNameSyntax)?.Identifier;
+        protected override SyntaxToken? GetArgumentIdentifier(ArgumentSyntax argument, SyntaxNodeAnalysisContext syntaxNodeAnalysisContext) => GetExpressionSyntaxIdentifier(argument?.Expression, syntaxNodeAnalysisContext);


The body should be on the next line, it will also fix the issue about too long line

Suggested change

protected override SyntaxToken? GetArgumentIdentifier(ArgumentSyntax argument, SyntaxNodeAnalysisContext syntaxNodeAnalysisContext) => GetExpressionSyntaxIdentifier(argument?.Expression, syntaxNodeAnalysisContext);

protected override SyntaxToken? GetArgumentIdentifier(ArgumentSyntax argument, SyntaxNodeAnalysisContext syntaxNodeAnalysisContext) =>

GetExpressionSyntaxIdentifier(argument?.Expression, syntaxNodeAnalysisContext);

- VB.NET implementation - more cases and tests

- added better nullable recognising - added better same arguments check

cristian-ambrosini-sonarsource · 2023-10-25T10:29:33Z

This Pull Request, along with the subsequent one #6748, has been decided to be closed as we are preparing to merge PR #8238.
The original implementation has been entirely rewritten, as detailed in the PR description, rendering the modifications in this PR incompatible with the new structure. #3879 will remain open and will be fixed as soon as possible. Nullable value types are just one of the many conversions that the rule does not support, the proposed solution will be to write a helper to cover them all.

pavel-mikula-sonarsource requested changes Jan 4, 2021

View reviewed changes

H4pH requested a review from pavel-mikula-sonarsource January 13, 2021 06:27

pavel-mikula-sonarsource requested changes Feb 1, 2021

View reviewed changes

analyzers/src/SonarAnalyzer.VisualBasic/Rules/ParametersCorrectOrder.cs Outdated Show resolved Hide resolved

analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ParametersCorrectOrder.cs Outdated Show resolved Hide resolved

csaba-sagi-sonarsource assigned pavel-mikula-sonarsource Aug 30, 2021

H4pH requested a review from pavel-mikula-sonarsource September 24, 2021 10:33

H4pH force-pushed the S2234-fn-nullable-argument-value branch from ed21c18 to 4bd6a1f Compare November 16, 2021 17:34

pavel-mikula-sonarsource requested changes Jan 21, 2022

View reviewed changes

pavel-mikula-sonarsource reviewed Jan 21, 2022

View reviewed changes

Nikolay Musikhin added 5 commits July 20, 2022 14:30

Add repro for S2234 FN SonarSource#3879

3f37883

Fix S2234 FN SonarSource#3879

140b6fa

Review update SonarSource#3879

e551ac9

- VB.NET implementation - more cases and tests

Review update

b3e8eed

- added better nullable recognising - added better same arguments check

jsons by akka.net

7a9eb69

pavel-mikula-sonarsource force-pushed the S2234-fn-nullable-argument-value branch from 4bd6a1f to 7a9eb69 Compare July 20, 2022 12:31

cristian-ambrosini-sonarsource mentioned this pull request Feb 13, 2023

Fix S2234 FN nullable argument value (H4pH) #6748

Closed

cristian-ambrosini-sonarsource closed this Oct 25, 2023

martin-strecker-sonarsource mentioned this pull request Oct 25, 2023

FNs/FPs in S2234: Improvements of the rule #8253

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix S2234 FN nullable argument value #3880

Fix S2234 FN nullable argument value #3880

H4pH commented Dec 30, 2020

pavel-mikula-sonarsource left a comment

pavel-mikula-sonarsource left a comment

pavel-mikula-sonarsource commented Feb 1, 2021

azure-pipelines bot commented Feb 1, 2021

pavel-mikula-sonarsource commented Feb 1, 2021

H4pH commented Feb 2, 2021

pavel-mikula-sonarsource commented Feb 2, 2021

pavel-mikula-sonarsource commented Jul 26, 2021

H4pH commented Sep 13, 2021

pavel-mikula-sonarsource commented Sep 24, 2021

H4pH commented Sep 28, 2021

pavel-mikula-sonarsource commented Nov 15, 2021

H4pH commented Jan 10, 2022

pavel-mikula-sonarsource left a comment

pavel-mikula-sonarsource Jan 21, 2022

H4pH Jan 28, 2022

pavel-mikula-sonarsource Jan 28, 2022

pavel-mikula-sonarsource Jan 21, 2022

pavel-mikula-sonarsource Jan 21, 2022

pavel-mikula-sonarsource commented Jan 21, 2022

azure-pipelines bot commented Jan 21, 2022

sonarcloud bot commented Jan 21, 2022

sonarcloud bot commented Jan 21, 2022

pavel-mikula-sonarsource commented Jan 21, 2022 •

edited

Loading

pavel-mikula-sonarsource Jan 21, 2022

cristian-ambrosini-sonarsource commented Oct 25, 2023

	protected override SyntaxToken? GetArgumentIdentifier(ArgumentSyntax argument, SyntaxNodeAnalysisContext syntaxNodeAnalysisContext) => GetExpressionSyntaxIdentifier(argument?.Expression, syntaxNodeAnalysisContext);
	protected override SyntaxToken? GetArgumentIdentifier(ArgumentSyntax argument, SyntaxNodeAnalysisContext syntaxNodeAnalysisContext) =>
	GetExpressionSyntaxIdentifier(argument?.Expression, syntaxNodeAnalysisContext);

Fix S2234 FN nullable argument value #3880

Fix S2234 FN nullable argument value #3880

Conversation

H4pH commented Dec 30, 2020

pavel-mikula-sonarsource left a comment

Choose a reason for hiding this comment

pavel-mikula-sonarsource left a comment

Choose a reason for hiding this comment

pavel-mikula-sonarsource commented Feb 1, 2021

azure-pipelines bot commented Feb 1, 2021

pavel-mikula-sonarsource commented Feb 1, 2021

H4pH commented Feb 2, 2021

pavel-mikula-sonarsource commented Feb 2, 2021

pavel-mikula-sonarsource commented Jul 26, 2021

H4pH commented Sep 13, 2021

pavel-mikula-sonarsource commented Sep 24, 2021

H4pH commented Sep 28, 2021

pavel-mikula-sonarsource commented Nov 15, 2021

H4pH commented Jan 10, 2022

pavel-mikula-sonarsource left a comment

Choose a reason for hiding this comment

pavel-mikula-sonarsource Jan 21, 2022

Choose a reason for hiding this comment

H4pH Jan 28, 2022

Choose a reason for hiding this comment

pavel-mikula-sonarsource Jan 28, 2022

Choose a reason for hiding this comment

pavel-mikula-sonarsource Jan 21, 2022

Choose a reason for hiding this comment

pavel-mikula-sonarsource Jan 21, 2022

Choose a reason for hiding this comment

pavel-mikula-sonarsource commented Jan 21, 2022

azure-pipelines bot commented Jan 21, 2022

sonarcloud bot commented Jan 21, 2022

sonarcloud bot commented Jan 21, 2022

pavel-mikula-sonarsource commented Jan 21, 2022 • edited Loading

pavel-mikula-sonarsource Jan 21, 2022

Choose a reason for hiding this comment

cristian-ambrosini-sonarsource commented Oct 25, 2023

pavel-mikula-sonarsource commented Jan 21, 2022 •

edited

Loading