-
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
New rule S6507: Blocks should not be synchronized on local variables #6854
New rule S6507: Blocks should not be synchronized on local variables #6854
Conversation
053f7a3
to
dfae8e0
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.
LGTM
} | ||
} | ||
|
||
void ReportIssue(string message) => |
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.
Nitpick: I'd keep this method and add the Rule as a parameter because it's still shorter.
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 agree. Otherwise LocalVariableRule
doesn't need to be reported via format string and the full message can be in the rule (using string.Format directly there)
Not required for the release.
441f077
to
032c6b2
Compare
Kudos, SonarCloud Quality Gate passed! |
Kudos, SonarCloud Quality Gate passed! |
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
<ul> | ||
<li> <a href="https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/statements/lock">Lock Statement</a> - lock statement - ensure | ||
exclusive access to a shared resource </li> | ||
<li> <a href="https://cwe.mitre.org/data/definitions/412">MITRE, CWE-412</a> - Unrestricted Externally Accessible Lock </li> |
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.
Not relevant for this clone of the rule - can be fixed in RSPEC later, not in this release
<li> <a href="https://cwe.mitre.org/data/definitions/412">MITRE, CWE-412</a> - Unrestricted Externally Accessible Lock </li> |
} | ||
} | ||
|
||
void ReportIssue(string message) => |
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 agree. Otherwise LocalVariableRule
doesn't need to be reported via format string and the full message can be in the rule (using string.Format directly there)
Not required for the release.
Implements new rule with "lock on local variable" check moved from S2445
To fix false positives introduced by #6701
Draft, to be merged together with RSPEC: SonarSource/rspec#1608