-
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 S3459 FP: Backing field with ref property #9383
Conversation
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.
Round 1 (many more to come!)
@@ -184,7 +184,8 @@ private static ISet<ISymbol> GetAssignedMemberSymbols(IList<MemberUsage> memberU | |||
|
|||
if (PreOrPostfixOpSyntaxKinds.Contains(parentNode.Kind()) | |||
|| (parentNode is AssignmentExpressionSyntax assignment && assignment.Left == node) | |||
|| (parentNode is ArgumentSyntax argument && (!argument.RefOrOutKeyword.IsKind(SyntaxKind.None) || TupleExpressionSyntaxWrapper.IsInstance(argument.Parent)))) | |||
|| (parentNode is ArgumentSyntax argument && (!argument.RefOrOutKeyword.IsKind(SyntaxKind.None) || TupleExpressionSyntaxWrapper.IsInstance(argument.Parent))) | |||
|| RefExpressionSyntaxWrapper.IsInstance(parentNode)) |
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 is too simple and introduces an FN (run with C# 9):
private int _foo; // FN
public int Foo => _foo;
void Method()
{
_ = ref _foo;
}
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 is not a FN. If you get a reference to the field, you have write access to it and that write can happen anywhere. This is like calling SomeMethod(ref field)
where you do not know what happens inside SomeMethod
. We would need to do symbolic execution on the assigned variable to properly detect all scenarios.
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.
But you have access to the method where the ref was used. I accept that this is out of the scope of this ticket, but at least add a reproducer to document the limitation.
Quality Gate passed for 'Sonar .NET Java Plugin'Issues Measures |
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.
Nice improvements!
Quality Gate passed for 'SonarAnalyzer for .NET'Issues Measures |
Peach validation |
Fixes #9106