Skip to content

Avoid reporting CA1859 for properties with setter more accessible than getter#50488

Closed
Alex-Sob wants to merge 3 commits intodotnet:mainfrom
Alex-Sob:dev/alex-sob/CA1859-fix
Closed

Avoid reporting CA1859 for properties with setter more accessible than getter#50488
Alex-Sob wants to merge 3 commits intodotnet:mainfrom
Alex-Sob:dev/alex-sob/CA1859-fix

Conversation

@Alex-Sob
Copy link
Copy Markdown

This PR fixes #50957

Issue

'CA1859: Use concrete types when possible for improved performance' rule is reported for properties with setter more accessible than getter, for example:

public class C
{
    // CA1859: Change type of property 'Property' from 'ITest' to 'Test' for improved performance
    public ITest Property { private get; set; } = new Test();

    void M()
    {
        Property.M();
    }
}

public interface ITest
{
    void M();
}

public class Test : ITest
{
    public void M() { }
}

While properties with setter that is more accessible than getter should probably be avoided, they are legal in C# and the analyzer should avoid reporting CA1859 in such cases.

Changes

  • Fixed UseConcreteTypeAnalyzer to avoid report CA1859 if a property has setter that is more accessible than getter.

  • Removed bool setter parameter from UseConcreteTypeAnalyzer.Collector.CanUpgrade method since it's only used with setter: false.

  • Added [StringSyntax("C#-test")] attribute to helper methods in UseConcreteTypeTests class that run a test. This enables syntax highlighting to all tests in that class.

    In [Proposal] Analyzer test readability/performance improvements #50342 I also suggested to add a helper method to Test class itself so that [StringSyntax] attribute can be applied in only in one place. It could be used instead of creating local helpers in test classes.

  • Added a unit test.

@Alex-Sob Alex-Sob requested a review from a team as a code owner August 26, 2025 15:04
@github-actions
Copy link
Copy Markdown
Contributor

This PR is targeting main, which is now for .NET 11-facing work. If you intended to target .NET 10, either retarget this PR to release/10.0.1xx or make sure you backport the change to release/10.0.1xx after merging. See #50394 for more details.

@Alex-Sob
Copy link
Copy Markdown
Author

@dotnet/dotnet-analyzers

@github-actions
Copy link
Copy Markdown
Contributor

Due to lack of recent activity, this PR has been labeled as 'Stale'. It will be closed if no further activity occurs within 7 more days. Any new comment will remove the label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CA1859 incorrectly being raised on a public property

2 participants