Fix S2094 FP: Primary constructors for classes should be handled equal to records #9019
Conversation
Corniel
force-pushed
the
corniel/fp-s2094
branch
from
fb87e2c
to
b1ce1e2
Compare
sebastien-marichal
added
Type: False Positive
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Corniel
force-pushed
the
corniel/fp-s2094
branch
from
b1ce1e2
to
d57c9b6
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| || IsParameterlessRecord(node); | ||
|
|
||
| private static bool IsParameterlessClass(SyntaxNode node) => | ||
| ClassDeclarationSyntax(node) is { } declaration |
There was a problem hiding this comment.
Why not just use the ClassDeclarationSyntax type directly? It's available without a wrapper.
The only change that it will need is to replace the declaration.ParameterList property with the declaration.ParameterList() method call.
There was a problem hiding this comment.
@zsolt-kolbay-sonarsource I thought I tried that, and that it did not work. Anyhow, now it does.
| && LacksParameters(declaration.ParameterList, declaration.BaseList); | ||
|
|
||
| private static bool LacksParameters(ParameterListSyntax parameterList, BaseListSyntax baseList) => | ||
| parameterList?.Parameters is not { Count: >= 1 } |
There was a problem hiding this comment.
[Optional] Count: > 0 is slightly less of a cognitive load for me.
There was a problem hiding this comment.
It was { Count : >= 1 } at other places in this class, so I kept it the same. Now I updated all here to your suggestion.
| @@ -18,6 +18,8 @@ | |||
| * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. | |||
| */ | |||
|
|
|||
| using System.Xml.Linq; | |||
There was a problem hiding this comment.
Not needed.
There was a problem hiding this comment.
I hate it when Visual Studio adds usings without telling you. It has been removed now.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
|
Consider the following piece of code:
Than
ChildClassshould not be considered an empty class, and hence should not raise S2094,I'm in doubt if S2094 should raise on
BaseClass, but you could argue that there should be a different rule, that checks if the parameters of a primary constructor (on classes only) is actually used.