SA1204 incorrectly reported when private static method follows non-private instance method

[oatkins]

The following code should not report SA1204, but actually reports it multiple times:

public class TestClass
{
    public static void A1() { }
    public void A2() { }
    internal static void A3() { }
    internal void A4() { }
    protected static void A5() { }
    protected void A6() { }
    private static void A7() { }
    static void A7a() { }
    private void A8() { }
    void A8a() { }
}

I think that the following changes would fix the code:

• constants and static fields should be treated as the same from the standpoint of this rule (since SA1203 handles that distinction),
• in the HandleMemberList() method, each time the access level changes or each time the "syntax kind" changes, previousMemberStatic should be reset to true.

I'm close to a fix for this.

[sharwell]

I'm not sure I agree with this evaluation. What does the original StyleCop report for the code above?

[oatkins]

No ordering rules are reported for the above. (Of course, there are multiple layout rule violations, and complaints about missing modifiers.)

The methods without access modifiers are treated as private. Therefore the order of A7() and A7a() may be swapped without adding a violation; similarly for A8() and A8a().

[Noryoko]

Looking through the documentation proper checks should be in the order of element type, access level , const/static, then readonly. I've got this out of order on both 1202 and 1204.

[oatkins]

That sounds good. OK if I leave you to work out what needs to change then @Noryoko?

[Noryoko]

Sure thing. Thanks for all the testing!

[Noryoko]

@oatkins Can you confirm the order below?

public const
internal const
protected internal const
protected const
private const

// fields
public static readonly
public static
public readonly
public
internal static readonly
internal static
internal readonly
internal
protected internal static readonly
protected internal static
protected internal readonly
protected internal
protected static readonly
protected static
protected readonly
protected
private static readonly
private static
private readonly
private

// non-fields per type (property, method, etc.)
public static
public
internal static
internal
protected internal static
protected internal
protected static
protected
private static
private

[oatkins]

Not quite. It seems that constants are treated as fields, just ones that sort above static readonly. The following gives no ordering errors in "classic" StyleCop:

public class Another
{
    public const int A = 1;
    public static readonly int F1 = 1;
    public static int F2 = 2;
    public readonly int F3 = 3;
    public int F4 = 4;
    internal const int B = 2;
    internal static readonly int F5 = 5;
    internal static int F6 = 6;
    internal readonly int F7 = 7;
    internal int F8 = 8;
    protected internal const int C = 3;
    // protected internal fields in same order as public...
    protected const int D = 4;
    // protected fields in same order as public...
    private const int E = 5;
    // private internal fields in same order as public...
}

Otherwise I think your list is correct.

[sharwell]

I'm torn when it comes to properties/methods, but for fields I just completely disagree with that ordering. I always want to see all constants, then all static, then all instance. Aside from StyleCop doing that in the past, do you see any advantage to interleaving them like this?

Also it makes no sense for protected internal to appear after internal, because it is strictly more visible. But that's a completely separate issue from this.

[Noryoko]

For initial implementation I think we should follow StyleCop. Perhaps we can add configurable ordering rules in the future.

[sharwell]

Yep, this will be interesting for sure. Changing this issue to 🐛 now that we know the expected order.

Thanks for the detailed feedback @oatkins.

[oatkins]

@sharwell I'm fairly resigned to not always agreeing with the SC rules on the basis that having a standard (even one I might take issue with occasionally) is better than having none at all. And I do believe that—since there is a sizable established user base for SC—it is best to maintain the SC behaviour unless it's clearly a bug.

I did notice the same thing as you regarding protected internal. (Perhaps the SC author assumed that protected internal meant protected and internal, which is what many people think it means.) Maybe it would make sense to "fix" that to be ahead of internal since I suspect that it's a very infrequently used language feature anyway.

[oatkins]

btw, I did triple-check after reading your comments. If I move all the constants to the top of the class I definitely do introduce SC warnings.