-
Notifications
You must be signed in to change notification settings - Fork 228
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 S1450 FP when field is assigned value in event handler #9510
Conversation
81930cd
to
50ab407
Compare
50ab407
to
303f71c
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.
Details and one question for extra tests.
if (privateFields.Keys.Any(x => x.Name == node.Identifier.ValueText)) | ||
var memberReference = GetTopmostSyntaxWithTheSameSymbol(node); | ||
if (memberReference.Symbol is IFieldSymbol fieldSymbol | ||
&& privateFields.ContainsKey(fieldSymbol)) |
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 can go to the previous line
&& memberReference.Symbol is IMethodSymbol | ||
&& GetParentPseudoStatement(memberReference) is { } pseudoStatement) | ||
else if (memberReference.Symbol is IMethodSymbol | ||
&& GetParentPseudoStatement(memberReference) is { } pseudoStatement) |
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.
same here
{ | ||
// Adding method group to the invocation list | ||
invocations.GetOrAdd(pseudoStatement, _ => new HashSet<ISymbol>()) | ||
.Add(memberReference.Symbol); |
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.
same
{ | ||
// If the field is not static and is not from the current instance we | ||
// consider the reference as read. | ||
if (!fieldReference.Symbol.IsStatic |
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.
same - one line
private void Increment(int dummy) | ||
=> _wasCalled = true; |
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.
private void Increment(int dummy) | |
=> _wasCalled = true; | |
private void Increment(int dummy) => | |
_wasCalled = true; |
} | ||
} | ||
|
||
private void Increment(int dummy) |
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 it possible ever to have such method in another class/assembly?
I know that fields should be accessible in the class it self only - but just to be sure/.
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 don't think it is possible.
If the field is updated outside the class, it is out of scope of this rule, IMO. And very peculiar, I am thinking of using unsafe
code or reflection.
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.
thanks!
Quality Gate passed for 'Sonar .NET Java Plugin'Issues Measures |
Quality Gate passed for 'SonarAnalyzer for .NET'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.
LGTM
Fixes #8239
These changes treat method groups as invocation.