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 S1144 FP: Should not mark template-method implementations #9692

Open
Xilconic opened this issue Nov 3, 2024 · 4 comments
Open

Fix S1144 FP: Should not mark template-method implementations #9692

Xilconic opened this issue Nov 3, 2024 · 4 comments
Assignees

Comments

@Xilconic
Copy link

Xilconic commented Nov 3, 2024

Description

S1144 is being raised unexpectedly for template-method implementations.

Repro steps

public class Repro()
{
    var x = new SomeClass();
    const string theName = x.Name;
    var y = x.DeepClone();
    const string theOtherName = y.Name;
}

public abstract class SomeClassDefiningTemplateMethods
{
    public abstract string Name { get; } // Template property
    public abstract SomeClassDefiningTemplateMethods DeepClone(); // Template method
}

public class SomeClass : SomeClassDefiningTemplateMethods
{
    public override string Name => "FooBar" // Currently being flagged with ``S1144: Remove the unused private property 'Name '.`
    public override SomeClassDefiningTemplateMethods DeepClone() // Currently being flagged with `S1144: Remove the unused private method 'DeepClone'.`
    {
        return new SomeClass();
    }
}

Expected behavior

S1144 should not be raised by SonarAnalyzer.CSharp

Actual behavior

For template method, raised: S1144: Remove the unused private method 'DeepClone'.
For template property, raises: S1144: Remove the unused private property 'Distribution'.
Also the message incorrectly states these are private methods/properties, while they actually are public.

Known workarounds

Ignore S1144 with #pragma's around method/property overrides.

Related information

  • JetBrains Rider 2024.2.7
  • dotnet version 8.0.303
  • SonarAnalyzer.CSharp package v9.32.0.97167
  • Windows 10
@Xilconic
Copy link
Author

Xilconic commented Nov 3, 2024

Seems related to #4918, but I think that one was focussed on the raising items on SomeClassDefiningTemplateMethods from my example.

@Xilconic
Copy link
Author

Xilconic commented Nov 3, 2024

The false positive disappeared again at some point after building.
Maybe code caches of Rider are causing this?

Now that I think of it, the S1144 was raised in code, but not the build log.

@sebastien-marichal sebastien-marichal self-assigned this Nov 5, 2024
@sebastien-marichal
Copy link
Contributor

Hello @Xilconic,

I am not able to reproduce your issue with the given reproducer.
It seems to have some errors.

Could you please take a look and fix it so I can take a deeper look at it?

Thanks,

@Xilconic
Copy link
Author

Xilconic commented Nov 6, 2024

Sorry, I took some shortcuts with writing the reproduction example from memory, based on what I was observing in my actual project.

I rewrote the provided example:

public class Repro()
{
    public void SomeMethod()
    {
        var x = new SomeClass();
        string theName = x.Name;
        var y = x.DeepClone();
        string theOtherName = y.Name;
    }
}

public abstract class SomeClassDefiningTemplateMethods
{
    public abstract string Name { get; } // Template property
    public abstract SomeClassDefiningTemplateMethods DeepClone(); // Template method
}

public class SomeClass : SomeClassDefiningTemplateMethods
{
    public override string Name => "FooBar"; // Currently being flagged with `S1144: Remove the unused private property 'Name'.`
    public override SomeClassDefiningTemplateMethods DeepClone() // Currently being flagged with `S1144: Remove the unused private method 'DeepClone'.`
    {
        return new SomeClass();
    }
}

However it might not prove useful to you, as this particular sample does not raise the S1144 false positive I originally experienced in my original project 😞
And as I mentioned in #9692 (comment), I don't have an actual reproduction scenario anymore in the original project either 😞

Shall we close this issue? And in case I manage to reproduce it again, I'll create a new issue (or reopen this one)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants