Fix S3264: FP when using add/remove syntax #1229
Conversation
valhristov
requested review from
duncanp-lseg,
Evangelink and
michalb-sonar
as code owners
ghost
assigned
| var removableEvents = removableDeclarationCollector.GetRemovableDeclarations( | ||
| ImmutableHashSet.Create(SyntaxKind.EventDeclaration), maxAccessibility); | ||
| var removableEventFields = removableDeclarationCollector.GetRemovableFieldLikeDeclarations( | ||
| ImmutableHashSet.Create(SyntaxKind.EventFieldDeclaration), maxAccessibility); |
There was a problem hiding this comment.
We found that ImmutableHashSet is a bit slower than HashSet before, hence I switched to HashSet.
|
|
||
| protected void OnEvent1() | ||
| { | ||
| PublicEvent += (s, o) => { }; // Need to use the event to be able to trigger issues |
There was a problem hiding this comment.
IF the event declaration is not used we don't report, hence we need this line.
There was a problem hiding this comment.
If that's not used do you know if we have another rule triggering (like unused member or something like that)? I actually think we should always report because this rule is about dead code so not subscribing + not raising = issue.
WDYT?
|
|
||
| protected void OnEvent1() | ||
| { | ||
| PublicEvent += (s, o) => { }; // Need to use the event to be able to trigger issues |
There was a problem hiding this comment.
If that's not used do you know if we have another rule triggering (like unused member or something like that)? I actually think we should always report because this rule is about dead code so not subscribing + not raising = issue.
WDYT?
|
@Evangelink, the unused member will raise issue when the event is not used. I would not increase the scope of the tickets, we could rather create a new ticket to update the rule. |
Removed the check for EventDeclarationSyntax as it cannot be invoked. One can only invoke EventFieldDeclarationSyntax but checking whether the event declaration is using the correct field is a different rule IMO. S3237 will report if the value of the accessor is not used (e.g. if you don't assign an event field).
Refactored the implementation a bit to make it more functional and somewhat easier to understand.
Fix #1219
Fix #1123