-
Notifications
You must be signed in to change notification settings - Fork 231
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 S1144 FP: Ignore Serializable members #8936
Fix S1144 FP: Ignore Serializable members #8936
Conversation
0f39ac8
to
072a90a
Compare
b32e66e
to
4c31f0a
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.
Left a question.
Also, Tim suggests on the issue to update the RSPEC.
Please take care of it and feel free to ask me for review.
analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedPrivateMember.cs
Outdated
Show resolved
Hide resolved
{ | ||
"Id": "S4487", | ||
"Message": "Remove this unread private field \u0027name\u0027 or refactor the code to use its value.", | ||
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/ManuallyAddedNoncompliantIssues.CS/NetFramework48/InsecureDeserialization.cs#L50", |
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.
Wasn't this a valid issue? I don't understand why it disappeared.
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.
That looks correct to me. S4487 and S1144 share the same analyzer class.
The example shows a class that has a private member that can be initialized through the constructor. Then the class can be serialized, so the private member is actually used/read. So I consider this a former FP.
@@ -19,6 +19,7 @@ | |||
*/ | |||
|
|||
using SonarAnalyzer.Common.Walkers; | |||
using SonarAnalyzer.Json.Parsing; |
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.
Is this needed?
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
4e12349
to
e4a3c0b
Compare
Quality Gate passedIssues Measures |
Quality Gate passedIssues Measures |
Fixes #8532