Fix S2259 FP: Recognize NotNull results of String methods #7838

mary-georgiou-sonarsource · 2023-08-21T12:12:09Z

mary-georgiou-sonarsource · 2023-08-22T09:51:58Z

analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/Invocation.cs

@@ -67,6 +67,7 @@ protected override ProgramState[] Process(SymbolicContext context, IInvocationOp
            _ when invocation.TargetMethod.Name == nameof(Equals) => ProcessEquals(context, invocation),
            _ when invocation.TargetMethod.IsAny(KnownType.System_String, nameof(string.IsNullOrEmpty), nameof(string.IsNullOrWhiteSpace)) =>
                ProcessIsNotNullWhen(state, invocation.WrappedOperation, invocation.Arguments[0].ToArgument(), false, true),
+            _ when invocation.TargetMethod.ContainingType.Is(KnownType.System_String) => ProcessSystemStringInvocation(state, invocation),


@Tim-Pohlmann in line 66 for ProcessLinqEnumerableAndQueryable see that the current state is being passed (context.State) - ignoring any constraints that might have been reset/added from previous lines.

I refactored the method there, in my machine locally, to accept the state as parameter and the UTs did not fail :/

If I'm not wrong we should pass also there the state and not the context.State - and figure out a UT that breaks in case of change.

Do I miss something here?

You miss that this is a switch expression. There is no fall-through. Only one line will get executed.

@Tim-Pohlmann I was not clear enough sorry - when I said previous lines I meant the ones before the switch -33- 58.

So you agree in passing the state and not the context.State?

It seems the correct thing to do. But we have to confirm it with a suitable UT.

I added a task for this to my calendar and I'll open an issue if it makes sense.
For now I will leave this PR as is.

Tim-Pohlmann

Round 1

...s/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/Invocation.String.cs

analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/Invocation.cs

Tim-Pohlmann · 2023-08-22T10:46:46Z

analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/Invocation.cs

@@ -67,6 +67,7 @@ protected override ProgramState[] Process(SymbolicContext context, IInvocationOp
            _ when invocation.TargetMethod.Name == nameof(Equals) => ProcessEquals(context, invocation),
            _ when invocation.TargetMethod.IsAny(KnownType.System_String, nameof(string.IsNullOrEmpty), nameof(string.IsNullOrWhiteSpace)) =>
                ProcessIsNotNullWhen(state, invocation.WrappedOperation, invocation.Arguments[0].ToArgument(), false, true),
+            _ when invocation.TargetMethod.ContainingType.Is(KnownType.System_String) => ProcessSystemStringInvocation(state, invocation),


You miss that this is a switch expression. There is no fall-through. Only one line will get executed.

...ts/expected/Ember-MM/generic.EmberCore.NMT-{84B2143A-D04F-4262-923D-21AEDF86E2B7}-S2259.json

mary-georgiou-sonarsource · 2023-08-22T12:21:55Z

...ts/expected/Ember-MM/generic.EmberCore.NMT-{84B2143A-D04F-4262-923D-21AEDF86E2B7}-S2259.json

@@ -158,6 +145,19 @@
 },
 {
 "id":  "S2259",
+"message":  "'fiAV' is Nothing on at least one execution path.",
+"location":  {
+"uri":  "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/Ember-MM/Addons/generic.EmberCore.NMT/dlgNMTMovies.vb#L620",


Not sure how this PR helped but this seems to be a TP due to line 601 (downcasting).

601 is no downcast, this is just VB's way to declare variables. The reason for 2259 here is probably the IsNothing in line 607.
Then again, I don't think this PR should raise any new issues or am I missing something?

well we learn now that certain methods do not return null - so from my side some issues would not be surprising.

This is a TP.
It's raising normally in the UTs before and after this change (thus is should not be related to this PR at all).

It should be related to the way the engine visits and cuts branches (this is my assumption).
I would not spend more time on it for now.

analyzers/its/expected/Ember-MM/EmberAPI-{208AA35E-C6AE-4D2D-A9DD-B6EFD19A4279}-S2589.json

Tim-Pohlmann

We need more dedicated UTs. And something is fishy with the ITs. We definitely need more investigation.

...ts/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.Invocation.cs

analyzers/its/expected/Ember-MM/EmberAPI-{208AA35E-C6AE-4D2D-A9DD-B6EFD19A4279}-S2589.json

...ts/expected/Ember-MM/generic.EmberCore.NMT-{84B2143A-D04F-4262-923D-21AEDF86E2B7}-S2259.json

Tim-Pohlmann · 2023-08-23T06:08:53Z

...ts/expected/Ember-MM/generic.EmberCore.NMT-{84B2143A-D04F-4262-923D-21AEDF86E2B7}-S2259.json

@@ -158,6 +145,19 @@
 },
 {
 "id":  "S2259",
+"message":  "'fiAV' is Nothing on at least one execution path.",
+"location":  {
+"uri":  "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/Ember-MM/Addons/generic.EmberCore.NMT/dlgNMTMovies.vb#L620",


601 is no downcast, this is just VB's way to declare variables. The reason for 2259 here is probably the IsNothing in line 607.
Then again, I don't think this PR should raise any new issues or am I missing something?

pavel-mikula-sonarsource · 2023-08-24T09:23:25Z

...s/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/Invocation.String.cs

+        {
+            return ProcessIsNotNullWhen(state, invocation.WrappedOperation, invocation.Arguments[0].ToArgument(), false, true);
+        }
+        else if (StringMethodReturningNotNull.Contains(invocation.TargetMethod.Name))


This should either use methodName, or methodName should be removed.
Following "2 is a group, 3 is a crowd", I'd remove methodName variable.

sonarcloud · 2023-08-24T10:50:49Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

sonarcloud · 2023-08-24T10:55:15Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

Tim-Pohlmann

LGTM!

mary-georgiou-sonarsource force-pushed the mary/2259-fp branch 3 times, most recently from 839c81a to 9d0930f Compare August 22, 2023 09:45

mary-georgiou-sonarsource commented Aug 22, 2023

View reviewed changes

mary-georgiou-sonarsource requested a review from Tim-Pohlmann August 22, 2023 09:52

Tim-Pohlmann requested changes Aug 22, 2023

View reviewed changes

mary-georgiou-sonarsource added 6 commits August 22, 2023 13:50

minimal implementation

7b687e1

Add tests

bb66fbe

remove string methods that are not needed and fix duplications

fb2cf24

simplify

251553a

update UT

d2da5a2

update ITs

43f040e

mary-georgiou-sonarsource force-pushed the mary/2259-fp branch from dd0d937 to 43f040e Compare August 22, 2023 12:03

mary-georgiou-sonarsource commented Aug 22, 2023

View reviewed changes

...ts/expected/Ember-MM/generic.EmberCore.NMT-{84B2143A-D04F-4262-923D-21AEDF86E2B7}-S2259.json Show resolved Hide resolved

mary-georgiou-sonarsource commented Aug 22, 2023

View reviewed changes

analyzers/its/expected/Ember-MM/EmberAPI-{208AA35E-C6AE-4D2D-A9DD-B6EFD19A4279}-S2589.json Show resolved Hide resolved

fix code smells

4f8aa37

mary-georgiou-sonarsource requested a review from Tim-Pohlmann August 22, 2023 12:24

Tim-Pohlmann requested changes Aug 23, 2023

View reviewed changes

re-add deleted test

656c970

pavel-mikula-sonarsource reviewed Aug 24, 2023

View reviewed changes

remove variable

d635495

mary-georgiou-sonarsource requested a review from Tim-Pohlmann August 24, 2023 12:09

Tim-Pohlmann approved these changes Aug 24, 2023

View reviewed changes

mary-georgiou-sonarsource merged commit 49afee5 into master Aug 24, 2023
29 checks passed

mary-georgiou-sonarsource deleted the mary/2259-fp branch August 24, 2023 12:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix S2259 FP: Recognize NotNull results of String methods #7838

Fix S2259 FP: Recognize NotNull results of String methods #7838

mary-georgiou-sonarsource commented Aug 21, 2023

mary-georgiou-sonarsource Aug 22, 2023

Tim-Pohlmann Aug 22, 2023

mary-georgiou-sonarsource Aug 22, 2023

Tim-Pohlmann Aug 22, 2023

mary-georgiou-sonarsource Aug 22, 2023

Tim-Pohlmann Aug 22, 2023

mary-georgiou-sonarsource Aug 24, 2023

Tim-Pohlmann left a comment

Tim-Pohlmann Aug 22, 2023

mary-georgiou-sonarsource Aug 22, 2023

Tim-Pohlmann Aug 23, 2023

mary-georgiou-sonarsource Aug 23, 2023

mary-georgiou-sonarsource Aug 24, 2023

Tim-Pohlmann left a comment

Tim-Pohlmann Aug 23, 2023

pavel-mikula-sonarsource Aug 24, 2023

sonarcloud bot commented Aug 24, 2023

sonarcloud bot commented Aug 24, 2023

Tim-Pohlmann left a comment

Fix S2259 FP: Recognize NotNull results of String methods #7838

Fix S2259 FP: Recognize NotNull results of String methods #7838

Conversation

mary-georgiou-sonarsource commented Aug 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tim-Pohlmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tim-Pohlmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Aug 24, 2023

sonarcloud bot commented Aug 24, 2023

Tim-Pohlmann left a comment

Choose a reason for hiding this comment