Stylecopanalyzers: Analyzer proposal: Don't use 'public' on members of internal types

[AArnott]

Or more generally: "Avoid visibility modifiers that exceed their effect"

Internal types may contain public members if those members implement interfaces.
Aside from that case, public members in an internal or private type are in fact not public at all, but internal.

During code reviews seeing an added or changed public member can set off alarm bells to reviewers who are keen to watch the evolution of a library's public API. It's an extra load on them to see public on a new member only to later realize that the container itself is internal or private and thus not worth an API review.

An analyzer should prevent liberal use of public where internal would be equivalent.

[AArnott]

I'm quite likely to write this analyzer myself as it's a big annoyance to me during code reviews.

[JohanLarsson]

Should be minutes to write one. I've also been thinking about this, not sure why I did not just write it. But agreed it is a nice fit for StyleCop.

[AArnott]

Well, feel free to grab it before I do, @JohanLarsson. Just mention me on the PR, as I'd love to review the test cases you have.

Thanks.

[JohanLarsson]

@AArnott wrote it like this: https://github.com/GuOrg/Gu.Analyzers/pull/217
We can move it here if it is decided that there will be a cop rule for it.

[AArnott]

@sharwell What do you think? Can we add this to StyleCop.Analyzers?

[JohanLarsson]

Aboute the sugestion:

• [x] Clear with effective accessibility on members.
• [ ] The benefit of the above goes when overriding and implementing interfaces. Then those methods look more accessible.
• [ ] Again when there are interface implementations and overrides forcing public the member ordering rules may force an unnatural ordering.

Maybe it should be default disabled if added?

[sharwell]

I'm concerned the interface implementation limitation would be sufficiently impactful as to undermine the intent of the rule. Other approaches, including but not limited to PublicApiAnalyzer, are superior for tracking changes to the public API surface.

For teams that implement public APIs first as internal types and then move to public types after review, the use of public methods to designate eventual public APIs of the types could be beneficial.

On the other hand, projects that have no public API by design, this has the interesting side effect of becoming a rule that simply reveals interface implementations.

Design questions:

1. How would this work for members of nested types? For example, members of protected types nested in an internal type would have a maximum accessibility of private protected, which impacts members declared public, internal, and protected.

[AArnott]

including but not limited to PublicApiAnalyzer, are superior for tracking changes to the public API surface.

That's a useful tool, but it is far from adequate. There are a lot of public API aspects that that analyzer does not catch.

I'm concerned the interface implementation limitation would be sufficiently impactful as to undermine the intent of the rule.

Do you mean the fact that some members have to be public in order to implement an interface? But those are public (or at least, as public as the interface, which may or may not be public). In my experience most members of internal types do not implement interfaces, and thus should be marked internal themselves. But so many people make them public simply out of habit. My goal for this analyzer is to break that habit in folks, to get them to think about public members more conservatively, and to make it far more likely for reviewers that the members will be declared as internal and save them some time.

For teams that implement public APIs first as internal types and then move to public types after review, the use of public methods to designate eventual public APIs of the types could be beneficial.

I see the merit there, and used to advocate for that position myself. For me, the prevailing counterargument I heard and (eventually) took to heart is that when promoting members to be public, the editor and reviewer should be very intentional about what comes along for the ride, and it absolutely merits a "diff" of each member to make it public so that the reviewer can take a fresh look at "is this really ready to be a public and stable entrypoint?"

Again when there are interface implementations and overrides forcing public the member ordering rules may force an unnatural ordering.

I don't understand the bit about unnatural ordering. I always thought StyleCop's ordering rules led to unnatural ordering because I would tend to put the private methods that support public methods near the public methods but StyleCop requires that all private methods are together at the bottom of the class. But I got used to it, and ultimately I find it useful as a reviewer to see all the changes to public methods first.
So yes, this analyzer would lead to methods previously marked public to be marked internal, and moved below methods that implement interfaces. But that's where they belong per prior StyleCop rules, isn't it?

How would this work for members of nested types? For example, members of protected types nested in an internal type would have a maximum accessibility of private protected, which impacts members declared public, internal, and protected.

If the nested type is internal or private, its own members should not be public (modulo interface implementations as always). If the nested type is protected, I believe its members need to be allowed to be public. I don't think private protected is adequate for members of a protected nested type for them to be visible to another assembly. @JohanLarsson do you have a test for this case in your analyzer?
This is why I think the analyzer should only flag members of types that are (effectively) internal or private.

[sharwell]

I don't think private protected is adequate for members of a protected nested type for them to be visible to another assembly.

The nested type in my example is not visible to other assemblies, because it is nested within an internal type:

internal class Outer {
  protected class Inner {
    // The maximum effective accessibility here is 'private protected'
  }
}

[JohanLarsson]

For teams that implement public APIs first as internal types and then move to public types after review, the use of public methods to designate eventual public APIs of the types could be beneficial.

Wonder how common this is. There is always he option to disable rules. For example the documentation rules do not make much sense in test projects.

[JohanLarsson]

I don't understand the bit about unnatural ordering. I always thought StyleCop's ordering rules led to unnatural ordering...

Agreed stylecop ordering is unnatural. For example fields and properties close to each other makes much sense to me but I obey the cop for sweet consistency. What I mean is that making things internal forces another level member will be sorted compared to just public/private. But again there is always the option to disable or make he rule default disabled.

...do you have a test for this case in your analyzer?

Nope, dumbed and missed that case. Trivial fix and should be rare, rare not meaning we should not handle it correctly and test it thoroughly.

[jnm2]

For teams that implement public APIs first as internal types and then move to public types after review, the use of public methods to designate eventual public APIs of the types could be beneficial.

This has always been my mentality. If the class itself would not be properly usable without access to a particular member, that member should semantically be public. I put thought into using internal only for members that would stay internal if the class was made public (and public for members that would semantically become public).

[AArnott]

What I mean is that making things internal forces another level member will be sorted compared to just public/private.

😲 You mean people choose to make members public instead of internal just so they can group members closer together without StyleCop complaining about ordering?

This has always been my mentality. If the class itself would not be properly usable without access to a particular member, that member should semantically be public. I put thought into using internal only for members that would stay internal if the class was made public (and public for members that would semantically become public).

Ya, my heart cries out "Yes!" to this, but I gave it up. Writing code for a future requirement often falls short of what you end up needing. I prefer to carefully review each public API as it is made public than to carefully review members of internal types in the anticipation that it will one day become public.

[JohanLarsson]

You mean people choose to make members public instead of internal just so they can group members closer together without StyleCop complaining about ordering

No, why do you think that?
Internal type is a special case where public and internal members are effectively internal but changing keyword forces ordering when using StyleCop.

[AArnott]

Well, how I read your meaning was that folks just used public and private so that there were only two "groups" of members and stylecop would let you order within that group as you please. But this analyzer would force a 3rd group (internal) into the mix. Without this analyzer, folks were free to use 2 or 3 groups, and your comment made it sound like folks might prefer 2 groups (by calling all internal things public) to give more ordering flexibility.
But regardless, it's not important.