dotnet/roslyn-analyzers

CA1515: false positive with delegates

pcf0 opened this issue · 3 comments

pcf0 commented

Analyzer

Diagnostic ID: CA1515: Consider making public types internal

Analyzer source

SDK: Built-in CA analyzers in .NET 9 SDK or later

Version: SDK 9.0.100-rc.2.24474.11

OR

NuGet Package: Microsoft.CodeAnalysis.NetAnalyzers

Version: 9.0.0-preview.24454.1

Describe the bug

A public delegate is defined within an internal class. CA1859 is generated for the delegate.

Steps To Reproduce

  1. Create an executable project, e.g. a console project.
  2. Create a internal class with a public delegate
internal class Class1
{
    public delegate void SomeDelegate();
}

Expected behavior

No errors reported

Actual behavior

Error reported:
Class1.cs(3,26): error CA1515: Because an application's API isn't typically referenced from outside the assembly, types can be made internal (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1515)

Additional context

This false positive seems to happen for any public nested type of an otherwise internal type. The analyzer SHOULD be checking if the target type is "effectively public" rather than marked public.

I did create a merge request that checks if the type/delegate/enum is externally accessible instead of public.

However, from reading through the docs it isn't quite clear whether this is intended but some of the tests for this functionality imply that it is intended...

My view is that CA1515 is intended to enable analysis (like pruning) that would help to either eliminate or optimize abstractions that fall within a finite closure. A public type does not because any external assembly can take a dependency on such a type. Non-public types, however, are subject to such analysis because all of their consumers can be known. However, nested public types of an internal type are not impacted by this argument. They still allow to the same analysis as their containing types because their effective visibility is still finite.

Whether nested types of internal types should be marked internal vs. public is somewhat of a stylistic consideration. My personally feeling is that marking a nested type public (regardless of its containing type's visibility) is meant to express an intent to the consumers of the containing abstraction. It is meant to indicate that the nested type makes up part of the public surface area of the containing abstraction. Consumers (whether within the same assembly or outside it) should constrain themselves to the public surface area of an abstraction to improve maintainability and reserve the implementation's freedom to alter its private design choices. Members of an abstraction marked internal, whether they be methods, properties, or nested types, should generally be reserved for use cases where an implementation of a single abstraction spans multiple classes (for example, where a collection class and its specialized Enumerator type work together to implement a single enumerable abstraction). Consumers of an abstraction, even those in the same assembly should never take advantage of unintended internals access beyond an explicitly expressed purpose because that leads to leaky abstractions. This draws a distinction between the use of internals on a top-level type and its use on members within a type. Use on a top-level type expresses an explicit intent about the audience of an abstraction. The use on members expresses an explicit intent on which members make up the surface area of an abstraction. Both of these intents are important, but they aren't the same thing.

Promoting the overuse of internal on members for other purposes defeats these principles and will lead to weaker code development overall. As such, it seems illogical to me that CA1515's intended purpose would be to enforce a policy that violates these principles. If it does, then I personally would disable it completely (even though that means losing out on its possible benefits for top-level types - I'd just have to remember to do the right thing for those types without it).