Fix S3254 FN: primary constructors #8222

martin-strecker-sonarsource · 2023-10-18T12:49:57Z

Fixes #8096
Also reverts #4620
~~Blocked by #8238~~

analyzers/src/SonarAnalyzer.CSharp/Rules/RedundantArgument.cs

martin-strecker-sonarsource · 2023-10-19T08:53:38Z

analyzers/its/expected/Nancy/Nancy--net452-S3254.json

@@ -2,6 +2,58 @@
 "issues":  [
 {
 "id":  "S3254",
+"message":  "Remove this default value assigned to parameter 'lifetime'.",
+"location":  {
+"uri":  "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/Nancy/src/Nancy/Bootstrapper/NancyBootstrapperWithRequestContainerBase.cs#L125",


TP: Constructor:

sonar-dotnet/analyzers/its/sources/Nancy/src/Nancy/Bootstrapper/NancyBootstrapperWithRequestContainerBase.cs

Line 125 in 21bb286

.Select(tr => new TypeRegistration(tr.RegistrationType, tr.ImplementationType, Lifetime.Singleton)))

martin-strecker-sonarsource · 2023-10-19T08:54:33Z

analyzers/its/expected/Nancy/Nancy--net452-S3254.json

+"id":  "S3254",
+"message":  "Remove this default value assigned to parameter 'lifetime'.",
+"location":  {
+"uri":  "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/Nancy/src/Nancy/Bootstrapper/NancyBootstrapperWithRequestContainerBase.cs#L134",


TP: Constructor

sonar-dotnet/analyzers/its/sources/Nancy/src/Nancy/Bootstrapper/NancyBootstrapperWithRequestContainerBase.cs

Line 134 in 21bb286

.Select(tr => new CollectionTypeRegistration(tr.RegistrationType, tr.ImplementationTypes, Lifetime.Singleton)))

martin-strecker-sonarsource · 2023-10-19T08:55:08Z

analyzers/its/expected/Nancy/Nancy--net452-S3254.json

+"id":  "S3254",
+"message":  "Remove this default value assigned to parameter 'disableMethodNotAllowedResponses'.",
+"location":  {
+"uri":  "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/Nancy/src/Nancy/RouteConfiguration.cs#L12",


TP: Constructor:

sonar-dotnet/analyzers/its/sources/Nancy/src/Nancy/RouteConfiguration.cs

Lines 11 to 12 in 21bb286

public static readonly RouteConfiguration Default = new RouteConfiguration(

disableMethodNotAllowedResponses: false,

martin-strecker-sonarsource · 2023-10-19T08:55:26Z

analyzers/its/expected/Nancy/Nancy--net452-S3254.json

+"id":  "S3254",
+"message":  "Remove this default value assigned to parameter 'explicitHeadRouting'.",
+"location":  {
+"uri":  "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/Nancy/src/Nancy/RouteConfiguration.cs#L13",


TP: Constructor

sonar-dotnet/analyzers/its/sources/Nancy/src/Nancy/RouteConfiguration.cs

Lines 11 to 13 in 21bb286

public static readonly RouteConfiguration Default = new RouteConfiguration(

disableMethodNotAllowedResponses: false,

explicitHeadRouting: false);

martin-strecker-sonarsource · 2023-10-19T08:58:06Z

analyzers/its/expected/Nancy/Nancy--net452-S3254.json

+"id":  "S3254",
+"message":  "Remove this default value assigned to parameter 'runtimeViewDiscovery'.",
+"location":  {
+"uri":  "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/Nancy/src/Nancy/ViewConfiguration.cs#L12",


TP: Constructor

sonar-dotnet/analyzers/its/sources/Nancy/src/Nancy/ViewConfiguration.cs

Lines 11 to 13 in 21bb286

public static readonly ViewConfiguration Default = new ViewConfiguration(

runtimeViewDiscovery: false,

runtimeViewUpdates: false);

martin-strecker-sonarsource · 2023-10-19T09:09:28Z

analyzers/its/expected/akka.net/Akka.Persistence--netstandard2.0-S3254.json

+"id":  "S3254",
+"message":  "Remove this default value assigned to parameter 'redeliveryBurstLimit'.",
+"location":  {
+"uri":  "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/akka.net/src/core/Akka.Persistence/Persistence.cs#L464",


Same as above:

sonar-dotnet/analyzers/its/sources/akka.net/src/core/Akka.Persistence/Persistence.cs

Line 464 in 21bb286

return Copy(null, null, null, maxUnconfirmedMessages);

martin-strecker-sonarsource · 2023-10-19T09:09:48Z

analyzers/its/expected/akka.net/Akka.Persistence--netstandard2.0-S3254.json

+"id":  "S3254",
+"message":  "Remove this default value assigned to parameter 'unconfirmedAttemptsToWarn'.",
+"location":  {
+"uri":  "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/akka.net/src/core/Akka.Persistence/Persistence.cs#L464",


Same as above

sonar-dotnet/analyzers/its/sources/akka.net/src/core/Akka.Persistence/Persistence.cs

Line 464 in 21bb286

return Copy(null, null, null, maxUnconfirmedMessages);

martin-strecker-sonarsource · 2023-10-19T09:10:27Z

analyzers/its/expected/akka.net/Akka.Persistence--netstandard2.0-S3254.json

+"id":  "S3254",
+"message":  "Remove this default value assigned to parameter 'redeliverInterval'.",
+"location":  {
+"uri":  "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/akka.net/src/core/Akka.Persistence/Persistence.cs#L474",


Same as above

sonar-dotnet/analyzers/its/sources/akka.net/src/core/Akka.Persistence/Persistence.cs

Line 474 in 21bb286

return Copy(null, redeliveryBurstLimit);

martin-strecker-sonarsource · 2023-10-19T09:10:56Z

analyzers/its/expected/akka.net/Akka.Persistence--netstandard2.0-S3254.json

+"id":  "S3254",
+"message":  "Remove this default value assigned to parameter 'redeliverInterval'.",
+"location":  {
+"uri":  "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/akka.net/src/core/Akka.Persistence/Persistence.cs#L484",


Two TPs:

sonar-dotnet/analyzers/its/sources/akka.net/src/core/Akka.Persistence/Persistence.cs

Line 484 in 21bb286

return Copy(null, null, unconfirmedAttemptsToWarn);

martin-strecker-sonarsource · 2023-10-19T09:11:34Z

analyzers/its/expected/akka.net/Akka.Persistence.Sql.Common--netstandard2.0-S3254.json

@@ -54,6 +54,32 @@
 },
 {
 "id":  "S3254",
+"message":  "Remove this default value assigned to parameter 'writerGuid'.",
+"location":  {
+"uri":  "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/akka.net/src/contrib/persistence/Akka.Persistence.Sql.Common/Journal/BatchingSqlJournal.cs#L1361",


I stopped here:
TP

sonar-dotnet/analyzers/its/sources/akka.net/src/contrib/persistence/Akka.Persistence.Sql.Common/Journal/BatchingSqlJournal.cs

Line 1361 in 21bb286

return new Persistent(deserialized, sequenceNr, persistenceId, manifest, isDeleted, ActorRefs.NoSender, null, timestamp);

cristian-ambrosini-sonarsource · 2023-10-23T12:23:17Z

analyzers/src/SonarAnalyzer.CSharp/Rules/RedundantArgument.cs

                        {
-                            var hasDefaultValue = ArgumentHasDefaultValue(argumentMapping, c.SemanticModel);


I'm still not 100% sure about reverting a PR that we are not sure about why it was introduced in the first place. I suggest asking @pavel-mikula-sonarsource and if he's ok we can move on. With the current information, I think you are right, and even with methods with lengthy parameters, we should raise and suggest to use named arguments (see my other comment).

We got no feedback from @pavel-mikula-sonarsource until now. We either merge today before peach deployment or the PR will not be part of the release. There are only a few new issues in the ITs related to this change and the new issues are TPs (e.g. https://github.com/SonarSource/sonar-dotnet/pull/8222/files#r1365174847).

This line was introduced as a fix to #4620, because it was fixing annoying FP in the rule.

analyzers/src/SonarAnalyzer.CSharp/Rules/RedundantArgument.cs

sonarcloud · 2023-10-25T17:08:24Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

sonarcloud · 2023-10-25T17:12:41Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

cristian-ambrosini-sonarsource

LGTM!

martin-strecker-sonarsource · 2023-10-27T08:52:26Z

Peach validation:

New issues: 1215 all TPs

The distribution of new issues is about:

~70% due to support of new (...)
~15% due to support of this (...) and base (...)
~15% due to the revert of Fix S3254 FP: Don't raise if the parameter isn't last. #4620

martin-strecker-sonarsource changed the title ~~Add object creation expression support~~ Fix S3254 FN: primary constructors Oct 18, 2023

martin-strecker-sonarsource commented Oct 19, 2023

View reviewed changes

analyzers/src/SonarAnalyzer.CSharp/Rules/RedundantArgument.cs Show resolved Hide resolved

martin-strecker-sonarsource commented Oct 19, 2023

View reviewed changes

martin-strecker-sonarsource requested a review from antonioaversa October 19, 2023 09:12

martin-strecker-sonarsource marked this pull request as ready for review October 19, 2023 09:16

martin-strecker-sonarsource requested a review from cristian-ambrosini-sonarsource October 20, 2023 07:55

cristian-ambrosini-sonarsource suggested changes Oct 23, 2023

View reviewed changes

martin-strecker-sonarsource added 11 commits October 25, 2023 18:02

Add object creation expression support

1605731

Formatting

e24f785

Fix NRE if ArgumentList is null.

cb6959d

Reduce method complexity

3b1fa58

Fix bug in early exit of loop

8725bd5

Update code fixes

702ec77

Remove unreachable branch and inline loop

2e953f4

Simplifications

c93c67a

Update ITs: akka

bdf65c6

Update ITs: Nancy

957544e

Add support for ConstructorInitializer and PrimaryConstructorBaseType

d01a843

martin-strecker-sonarsource force-pushed the Martin/CS12_S3254 branch from de5f27a to d01a843 Compare October 25, 2023 16:02

martin-strecker-sonarsource added 2 commits October 25, 2023 18:08

Fix fixmes

4d30125

Update ITs Akka

7393c0a

martin-strecker-sonarsource requested a review from cristian-ambrosini-sonarsource October 25, 2023 19:54

cristian-ambrosini-sonarsource approved these changes Oct 26, 2023

View reviewed changes

cristian-ambrosini-sonarsource merged commit 9ff001d into master Oct 26, 2023
28 checks passed

cristian-ambrosini-sonarsource deleted the Martin/CS12_S3254 branch October 26, 2023 12:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix S3254 FN: primary constructors #8222

Fix S3254 FN: primary constructors #8222

martin-strecker-sonarsource commented Oct 18, 2023 •

edited

Loading

martin-strecker-sonarsource Oct 19, 2023

martin-strecker-sonarsource Oct 19, 2023

martin-strecker-sonarsource Oct 19, 2023

martin-strecker-sonarsource Oct 19, 2023

martin-strecker-sonarsource Oct 19, 2023

martin-strecker-sonarsource Oct 19, 2023

martin-strecker-sonarsource Oct 19, 2023

martin-strecker-sonarsource Oct 19, 2023

martin-strecker-sonarsource Oct 19, 2023

martin-strecker-sonarsource Oct 19, 2023

cristian-ambrosini-sonarsource Oct 23, 2023

martin-strecker-sonarsource Oct 26, 2023

pavel-mikula-sonarsource Oct 26, 2023

sonarcloud bot commented Oct 25, 2023

sonarcloud bot commented Oct 25, 2023

cristian-ambrosini-sonarsource left a comment

martin-strecker-sonarsource commented Oct 27, 2023

	public static readonly RouteConfiguration Default = new RouteConfiguration(
	disableMethodNotAllowedResponses: false,

	public static readonly RouteConfiguration Default = new RouteConfiguration(
	disableMethodNotAllowedResponses: false,
	explicitHeadRouting: false);

	public static readonly ViewConfiguration Default = new ViewConfiguration(
	runtimeViewDiscovery: false,
	runtimeViewUpdates: false);

		{
		var hasDefaultValue = ArgumentHasDefaultValue(argumentMapping, c.SemanticModel);

Fix S3254 FN: primary constructors #8222

Fix S3254 FN: primary constructors #8222

Conversation

martin-strecker-sonarsource commented Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Oct 25, 2023

sonarcloud bot commented Oct 25, 2023

cristian-ambrosini-sonarsource left a comment

Choose a reason for hiding this comment

martin-strecker-sonarsource commented Oct 27, 2023

martin-strecker-sonarsource commented Oct 18, 2023 •

edited

Loading