-
Notifications
You must be signed in to change notification settings - Fork 229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix S3900 FP: assigned parameters #7044
Fix S3900 FP: assigned parameters #7044
Conversation
1911683
to
0c1a3d3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More tests are needed.
...st/TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.CSharp9.cs
Outdated
Show resolved
Hide resolved
...Analyzer.UnitTest/Rules/SymbolicExecution/PublicMethodArgumentsShouldBeCheckedForNullTest.cs
Outdated
Show resolved
Hide resolved
...narAnalyzer.Common/SymbolicExecution/Constraints/AssignmentOrReferenceParameterConstraint.cs
Outdated
Show resolved
Hide resolved
...narAnalyzer.Common/SymbolicExecution/Constraints/AssignmentOrReferenceParameterConstraint.cs
Outdated
Show resolved
Hide resolved
...narAnalyzer.Common/SymbolicExecution/Constraints/AssignmentOrReferenceParameterConstraint.cs
Outdated
Show resolved
Hide resolved
...SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs
Outdated
Show resolved
Hide resolved
...SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good improvement for S3900, solves quite a few FP!
I have just some minor remarks, mostly cosmetics.
@martin-strecker-sonarsource already identified pretty much all the interesting scenarios which would be good to have here.
...SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs
Outdated
Show resolved
Hide resolved
...SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs
Outdated
Show resolved
Hide resolved
...SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs
Outdated
Show resolved
Hide resolved
...narAnalyzer.Common/SymbolicExecution/Constraints/AssignmentOrReferenceParameterConstraint.cs
Outdated
Show resolved
Hide resolved
...Analyzer.UnitTest/Rules/SymbolicExecution/PublicMethodArgumentsShouldBeCheckedForNullTest.cs
Outdated
Show resolved
Hide resolved
...Analyzer.UnitTest/Rules/SymbolicExecution/PublicMethodArgumentsShouldBeCheckedForNullTest.cs
Outdated
Show resolved
Hide resolved
...r.UnitTest/TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs
Show resolved
Hide resolved
...SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Constraints/ConstraintKind.cs
Outdated
Show resolved
Hide resolved
...rs/tests/SonarAnalyzer.UnitTest/Rules/SymbolicExecution/ParameterReassignedConstraintTest.cs
Outdated
Show resolved
Hide resolved
...rs/tests/SonarAnalyzer.UnitTest/Rules/SymbolicExecution/ParameterReassignedConstraintTest.cs
Outdated
Show resolved
Hide resolved
...rs/tests/SonarAnalyzer.UnitTest/Rules/SymbolicExecution/ParameterReassignedConstraintTest.cs
Outdated
Show resolved
Hide resolved
...zers/src/SonarAnalyzer.Common/SymbolicExecution/Constraints/ParameterReassignedConstraint.cs
Outdated
Show resolved
Hide resolved
...SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Constraints/ConstraintKind.cs
Outdated
Show resolved
Hide resolved
...icExecution/PublicMethodArgumentsShouldBeCheckedForNull.ParameterReassignedConstraintTest.cs
Outdated
Show resolved
Hide resolved
...icExecution/PublicMethodArgumentsShouldBeCheckedForNull.ParameterReassignedConstraintTest.cs
Outdated
Show resolved
Hide resolved
...icExecution/PublicMethodArgumentsShouldBeCheckedForNull.ParameterReassignedConstraintTest.cs
Outdated
Show resolved
Hide resolved
...icExecution/PublicMethodArgumentsShouldBeCheckedForNull.ParameterReassignedConstraintTest.cs
Outdated
Show resolved
Hide resolved
...icExecution/PublicMethodArgumentsShouldBeCheckedForNull.ParameterReassignedConstraintTest.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestFramework/SymbolicExecution/SETestContext.cs
Show resolved
Hide resolved
...st/TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.CSharp9.cs
Outdated
Show resolved
Hide resolved
...icExecution/PublicMethodArgumentsShouldBeCheckedForNull.ParameterReassignedConstraintTest.cs
Outdated
Show resolved
Hide resolved
06c67d1
to
4e22d67
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You accidentally delete some code after re-basing which caused a lot of changes in the ITs. You need to revert the line and re-create the ITs.
Test coverage looks good now.
analyzers/its/expected/Automapper/AutoMapper--net461-S3900.json
Outdated
Show resolved
Hide resolved
analyzers/its/expected/Automapper/AutoMapper--net461-S3900.json
Outdated
Show resolved
Hide resolved
...ommon/SymbolicExecution/Roslyn/RuleChecks/PublicMethodArgumentsShouldBeCheckedForNullBase.cs
Outdated
Show resolved
Hide resolved
...ommon/SymbolicExecution/Roslyn/RuleChecks/PublicMethodArgumentsShouldBeCheckedForNullBase.cs
Show resolved
Hide resolved
...ommon/SymbolicExecution/Roslyn/RuleChecks/PublicMethodArgumentsShouldBeCheckedForNullBase.cs
Show resolved
Hide resolved
...ommon/SymbolicExecution/Roslyn/RuleChecks/PublicMethodArgumentsShouldBeCheckedForNullBase.cs
Outdated
Show resolved
Hide resolved
...itTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.ParameterReassignedConstraint.cs
Outdated
Show resolved
Hide resolved
...r.UnitTest/TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs
Outdated
Show resolved
Hide resolved
...r.UnitTest/TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.vb
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM: all my remarks have been addressed.
Martin's ones to be addressed.
d380b83
to
269a896
Compare
269a896
to
d741b1f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more corrections.
"location": { | ||
"uri": "sources\Ember-MM\Ember%20Media%20Manager\clsTheming.vb", | ||
"region": { | ||
"startLine": 169, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good:
https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/Ember-MM/Ember%20Media%20Manager/clsTheming.vb#L164-L169
The issue, raised in line 164 is still there as it should.
"location": { | ||
"uri": "sources\Ember-MM\Ember.Plugins\Utility.cs", | ||
"region": { | ||
"startLine": 46, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both are TN:
sonar-dotnet/analyzers/its/sources/Ember-MM/Ember.Plugins/Utility.cs
Lines 34 to 51 in e641f52
public static string SanitiseIMDbId(string imdbId) | |
{ | |
if (string.IsNullOrEmpty(imdbId) || imdbId.Trim().Length == 0) | |
return string.Empty; | |
imdbId = imdbId.Trim(); | |
if ("tt".Equals(imdbId)) | |
imdbId = string.Empty; | |
else | |
{ | |
if (!imdbId.StartsWith("tt")) | |
imdbId = string.Concat("tt", imdbId); | |
if (imdbId.Length != 9) | |
imdbId = string.Empty; | |
} | |
return imdbId; | |
} |
"location": { | ||
"uri": "sources\Ember-MM\EmberAPI\clsAPIFileUtils.vb", | ||
"region": { | ||
"startLine": 113, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All issues are gone except the first one on line 112
sonar-dotnet/analyzers/its/sources/Ember-MM/EmberAPI/clsAPIFileUtils.vb
Lines 111 to 122 in e641f52
Public Shared Function MakeValidFilename(ByVal filename As String) As String | |
filename = filename.Replace(":", " -") | |
filename = filename.Replace("/", String.Empty) | |
'pattern = pattern.Replace("\", String.Empty) | |
filename = filename.Replace("|", String.Empty) | |
filename = filename.Replace("<", String.Empty) | |
filename = filename.Replace(">", String.Empty) | |
filename = filename.Replace("?", String.Empty) | |
filename = filename.Replace("*", String.Empty) | |
filename = filename.Replace(" ", " ") | |
Return filename | |
End Function |
"location": { | ||
"uri": "sources\Ember-MM\Addons\scraper.EmberCore.XML\XMLScraper\Utilities\Util.vb", | ||
"region": { | ||
"startLine": 188, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here too:
Lines 186 to 197 in e641f52
Public Shared Function FixParse(ByVal value As String) As String | |
value = value.Replace("&", "&") | |
value = value.Replace("&amp;", "&") | |
value = value.Replace("&amp;amp;", "&&") | |
value = value.Replace("&#", "&#") | |
value = value.Replace("&quot;", """) | |
value = value.Replace("&lt;", "<") | |
value = value.Replace("&gt;", ">") | |
value = value.Replace("&apos;", "'") | |
Return value | |
End Function |
"location": { | ||
"uri": "sources\Nancy\src\Nancy\Helpers\HttpUtility.cs", | ||
"region": { | ||
"startLine": 192, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TN (reassign in line 148)
return e.GetString(buf); |
"location": { | ||
"uri": "sources\akka.net\src\core\Akka.Remote.TestKit\MultiNodeSpec.cs", | ||
"region": { | ||
"startLine": 549, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TN:
sonar-dotnet/analyzers/its/sources/akka.net/src/core/Akka.Remote.TestKit/MultiNodeSpec.cs
Lines 546 to 549 in e641f52
public void MuteDeadLetters(ActorSystem system = null, params Type[] messageClasses) | |
{ | |
if (system == null) system = Sys; | |
if (!system.Log.IsDebugEnabled) |
...ommon/SymbolicExecution/Roslyn/RuleChecks/PublicMethodArgumentsShouldBeCheckedForNullBase.cs
Outdated
Show resolved
Hide resolved
...ommon/SymbolicExecution/Roslyn/RuleChecks/PublicMethodArgumentsShouldBeCheckedForNullBase.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Constraints/ConstraintKind.cs
Outdated
Show resolved
Hide resolved
...zers/src/SonarAnalyzer.Common/SymbolicExecution/Constraints/ParameterReassignedConstraint.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Please fix the comment if appropriate and create a follow-up issue for the remaining operations or reuse an existing issue for the tracking.
@@ -67,4 +83,10 @@ private static IOperation NullDereferenceCandidate(IOperation operation) | |||
}; | |||
return candidate?.UnwrapConversion(); | |||
} | |||
|
|||
private static IOperation AssignmentTarget(IOperation operation) => | |||
// Missing operation types: DeconstructionAssignment, CoalesceAssignment, CompoundAssignment, ThrowExpression |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "assignment with flow capture" is missing here, right?
// Missing operation types: DeconstructionAssignment, CoalesceAssignment, CompoundAssignment, ThrowExpression | |
// Missing operation types: DeconstructionAssignment, CoalesceAssignment, CompoundAssignment, ThrowExpression, assignment with flow capture |
@@ -67,4 +83,10 @@ private static IOperation NullDereferenceCandidate(IOperation operation) | |||
}; | |||
return candidate?.UnwrapConversion(); | |||
} | |||
|
|||
private static IOperation AssignmentTarget(IOperation operation) => | |||
// Missing operation types: DeconstructionAssignment, CoalesceAssignment, CompoundAssignment, ThrowExpression |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have an issue that tracks all the missing operations? If not, create one and link any other issues that mentions parts of it. Add a checklist with all the missing operations to that issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#7091
I did not add for compound assignment - as it doesn't make much sense here -, nor for ThrowExpression
. This seems to work fine:
public void ThrowExpression(object o)
{
_ = o ?? throw new NullReferenceException(nameof(o));
o.ToString();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please link to #7039 in the issue as well. Please add all cases we discussed so far. Even if they are working already, we need to make sure we have all cases under test. "This seems to work fine." is not enough.
Kudos, SonarCloud Quality Gate passed! |
Kudos, SonarCloud Quality Gate passed! |
Fixes #6972
Fixes #3400