Skip to content

BRO1409: Methods of internal types should be internal, not public

An ordinary method declared public in a type that can't be seen outside the assembly (the type, or a type it is nested in, is internal, private, private protected or file) is internal in effect; internal says so. Not a StyleCop rule: proposed in StyleCop issue #2981, never implemented. Off by default, also in the preset. Deliberately narrow: only methods, and only where nothing outside the compiler's own rules could depend on public (see Why and Not reported).

Example

root = true

[*.cs]
dotnet_diagnostic.BRO1409.severity = warning

Before

internal sealed class PriceCalculator
{
    public decimal Total(decimal net, decimal rate) => net * (1 + rate);
}

After

internal sealed class PriceCalculator
{
    internal decimal Total(decimal net, decimal rate) => net * (1 + rate);
}

Why

public on a member of an internal type promises more than it gives: the member is reachable only inside the assembly (and its InternalsVisibleTo friends) either way. internal tells the reader that, and making the type public later doesn't silently publish the member.

But the compiler is not the only one that reads public. Reflection's defaults look at public members only, and so do most libraries built on it. On a member of an internal type, public vs internal changes what they see:

internal class Order
{
    public int Id { get; set; }        // internal: System.Text.Json and Newtonsoft.Json skip it, "Id" vanishes from the JSON
    public void Ship() { }             // internal: typeof(Order).GetMethods() no longer lists it
}

public class OrderTests
{
    [Fact]
    public void Ships() { }            // internal: xUnit, NUnit and MSTest don't discover the test
}

The same goes for XmlSerializer and data contracts (properties and fields), WPF/WinForms data binding and ASP.NET model binding (properties), AutoMapper and Dapper (properties), Activator.CreateInstance and Microsoft.Extensions.DependencyInjection (public constructors), MVC actions, SignalR hubs and Blazor's [JSInvokable] (methods). So the rule leaves out every kind of member and every method such code finds, and keeps the ordinary methods your own code calls.

How the fix works

It replaces public with internal where public was (static public becomes static internal; an explicit modifier stays, because BRO1404 wants one). The method's name is then looked up in every string literal and every nameof(...) of the solution (GetMethod("Ship"), GetMethod(nameof(Order.Ship))): when it appears, the warning stays and the fix leaves the method alone, like the naming rules' reflection guard. The same when another project (an InternalsVisibleTo friend) derives from the type: its class could implement an interface with the inherited method.

BRO1001 sorts public members before internal ones, so a method that becomes internal may now be out of order: when BRO1001 is on, the fix sorts the type too, so one dotnet format run is enough.

Not reported

  • Anything but ordinary methods: properties, fields, events, indexers (serializers, binding, mappers), constructors (dependency injection, Activator), operators and conversion operators (must be public), finalizers, nested types.
  • Methods others implement or override with: override, virtual, abstract, implicit and explicit interface implementations (also an inherited method that a derived class uses to implement an interface), methods of interfaces.
  • Methods with any attribute (tests, benchmarks, [JSInvokable], MVC, [OnDeserialized] callbacks, ...), and methods of a type that has an attribute or is nested in one that has.
  • Methods of a type deriving from a type of another assembly other than object/ValueType (ASP.NET Controller and Hub, test base classes, Unity's MonoBehaviour messages, Exception, List<T>): frameworks call public methods of such types by convention.
  • Names found by convention: Main, Dispose, DisposeAsync, GetEnumerator, GetAsyncEnumerator, MoveNext, MoveNextAsync, GetAwaiter, GetResult, Add (collection initializers), Deconstruct, GetPinnableReference, Configure, ConfigureServices, ConfigureContainer (startup classes), Invoke, InvokeAsync (middleware), and the names records synthesize (PrintMembers, Equals, GetHashCode, ToString).
  • extern and partial methods.
  • #if in the method or in the header of an enclosing type (the base list can differ per target framework).

Extension methods are reported: internal works for them everywhere the internal type does.

Reported but not fixed

If dotnet format leaves a finding, StyleBro kept it on purpose. The fix looks at the whole solution, other projects included, which the analyzer can't see, and leaves a change that would break something there: the name in a string or a nameof (reflection, serialization), a member that would hide it, a use in generated code. It offers no code action for such a finding, and stylebro-migrate format lists each one with its reason (Findings kept on purpose); its --verify-no-changes doesn't fail on them. Change it by hand only after checking those uses, or suppress it or record it in the baseline.

Compared with StyleCop

StyleCop has no such rule. stylebro-migrate leaves it off.