Skip to content
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 S3604 FP: Primary constructors #7624

Closed
hankovich opened this issue Jul 19, 2023 · 14 comments · Fixed by #8390
Closed

Fix S3604 FP: Primary constructors #7624

hankovich opened this issue Jul 19, 2023 · 14 comments · Fixed by #8390
Assignees
Labels
Area: C# C# rules related issues. Area: C#12 C#12 related issues Type: False Positive Rule IS triggered when it shouldn't be.
Projects
Milestone

Comments

@hankovich
Copy link

hankovich commented Jul 19, 2023

Description

S3604 is reported for classes with primary constructors.

Repro steps

class SampleClass(object options)
{
    private readonly object _options = options; // Noncompliant - FP: S3604 shouldn't be reported here, this is not a redundant initializer
}

Expected behavior

S3064 should not be raised.

Actual behavior

S3064 raises a warning.

Known workarounds

N/A

Related information

  • C#/VB.NET Plugins version: 9.5.0.73987
  • Visual Studio version: latest preview
  • MSBuild / dotnet version: .NET 8 preview 6
@zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource added Type: False Positive Rule IS triggered when it shouldn't be. Area: C# C# rules related issues. Area: C#12 C#12 related issues labels Jul 24, 2023
@zsolt-kolbay-sonarsource
Copy link
Contributor

Thank you for reporting this issue @hankovich. Confirmed as False Positive.

@zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource removed their assignment Jul 24, 2023
@zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource changed the title S3604 is reported for classes with primary constructors Fix S3604 FP: Primary constructors Jul 24, 2023
@smokedlinq
Copy link

With .NET 8.0 right around the corner, this would be nice to get fixed as the primary constructor refactor causes this to appear

@OpenEnergyGroup
Copy link

This is an issue for properties too: this code will raises an incorrect S3604 warning:

public class Foo(string foo)
{
    public List<string> MyList { get; } = [foo];
}

Thanks.

@Guiorgy
Copy link

Guiorgy commented Nov 15, 2023

.NET 8 is out, and indeed I get S3604:

public sealed class MyClass(string name, string lastName) : IEquatable<MyClass>
{
    public string Name { get; } = name; // S3604
    public string LastName{ get; } = lastName; // S3604
    // ...
}

As a workaround, you could rewrite the properties to return the generated private field dirrectly instead of the backing fields:

public sealed class MyClass(string name, string lastName) : IEquatable<MyClass>
{
    public string Name { get => name; }
    public string LastName{ get; } = lastName; // S3604
    // ...
}

Name no longer triggers S3604. Here's the Generated C#:

[System.Runtime.CompilerServices.NullableContext(1)]
[System.Runtime.CompilerServices.Nullable(0)]
public sealed class MyClass
{
    [CompilerGenerated]
    [DebuggerBrowsable(DebuggerBrowsableState.Never)]
    private string <name>P;

    [CompilerGenerated]
    [DebuggerBrowsable(DebuggerBrowsableState.Never)]
    private readonly string <LastName>k__BackingField;

    public string Name
    {
        get
        {
            return <name>P;
        }
    }

    public string LastName
    {
        [CompilerGenerated]
        get
        {
            return <LastName>k__BackingField;
        }
    }

    public MyClass(string name, string lastName)
    {
        <name>P = name;
        <LastName>k__BackingField = lastName;
        base..ctor();
    }
}

So pretty much the same.

@Corniel
Copy link
Contributor

Corniel commented Nov 15, 2023

I can confirm this issue.

@Davidsv
Copy link

Davidsv commented Nov 15, 2023

Still got this issue, NET 8.0.100, latest visal studio 17.8, latest sonarlint 7.4.0.80741

@FelixDamrau
Copy link

.NET 8 is out, and indeed I get S3604:
...snip...
As a workaround, you could rewrite the properties to return the generated private field dirrectly instead of the backing fields:
...snip...
So pretty much the same.

The workaround suppresses the S3604 message, but the compiler-generated backing field is not readonly anymore.

Thank you for your effort in providing the workaround. However, I still need the readonly stuff, so this issue remains a concern for me.

Anyhow, I mainly want to add a comment, so this issue may get prioritized.

@Guiorgy
Copy link

Guiorgy commented Nov 17, 2023

You're right, missed that. readonly should allow the runtime some low level optimizations too

@shkarface
Copy link

The issue has affected our codebases as we're upgrading to .NET 8 and new C# features as well.

+1.

@ms-lemos
Copy link

ms-lemos commented Nov 21, 2023

+1.
False positives with .net 8. Commenting in hope for this to get priority, it's quite annoying.

@andrei-epure-sonarsource
Copy link
Contributor

Thanks, we will prioritize it and apologies for missing this in our previous release.

@Corniel
Copy link
Contributor

Corniel commented Nov 21, 2023

Thanks, we will prioritize it and apologies for missing this in our previous release.

Those things can happen. No worries. Are you considering to ship it earlier than you normally do?

@github-actions github-actions bot moved this from Review in progress to In progress in Best Kanban Nov 22, 2023
@github-actions github-actions bot moved this from In progress to Review in progress in Best Kanban Nov 22, 2023
@github-actions github-actions bot moved this from Review in progress to Review approved in Best Kanban Nov 22, 2023
Best Kanban automation moved this from Review approved to Validate Peach Nov 22, 2023
@antonioaversa antonioaversa added this to the 9.14 milestone Nov 23, 2023
@martin-strecker-sonarsource martin-strecker-sonarsource moved this from Validate Peach to Done in Best Kanban Nov 23, 2023
@andrei-epure-sonarsource
Copy link
Contributor

andrei-epure-sonarsource commented Nov 23, 2023

@Corniel, it went into the latest release. Soon, we'll deploy to nuget.org, and next week, we'll deploy to SonarCloud.
This will be included in SQ 10.4 and the next release of SonarLint.

Thanks to all who reported this issue on this thread, and again, my apologies for slipping this through the cracks.

@Corniel
Copy link
Contributor

Corniel commented Nov 24, 2023

We're all developers, we all miss things sometimes. No reason to apologize. Keep up the good work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: C# C# rules related issues. Area: C#12 C#12 related issues Type: False Positive Rule IS triggered when it shouldn't be.
Projects
Best Kanban
  
Done
Development

Successfully merging a pull request may close this issue.