SA1311 - new rule for Static Readonly fields. (was SA1306 now more strict in 4.7.22. Why?)

May 16, 2012 at 6:23 AM

With 4.7.22 as compared to 4.7.18 we now get tons of new SA1306 warnings.

Example:

private static readonly Dictionary<Type, Set<string>> ServiceNodeMethods = ...

is not flagged with Warning    4    SA1306: Variable names must start with a lower-case letter: ServiceNodeMethods.

It seems that the rule definition has been changed from "Constants must always start with an uppercase letter, whilst readonly fields may start with either an uppercase or a lowercase letter" (http://stylecop.soyuz5.com/SA1306.html) to "Constants and non-private readonly fields must always start with an uppercase letter, whilst private readonly fields must start with a lowercase letter" (StyleCop.chm in 4.7.22)

I wonder if there could be some better process of making existing rules stricter and not just drop them on us without any kind of community discussion (or maybe I missed it).

May 16, 2012 at 7:02 AM
Edited May 16, 2012 at 7:07 AM

We also got hundreds of these SA1306 warnings for private static readonly fields since 4.7.22.

The "and" part of the rules also makes it impossible to keep the first part of the check...

If this kind of "and" conditions is part of a changed rule I think it should be split in two rules, in this case a "PrivateReadOnlyFieldsMustBeginWithLowerCaseLetter" rule should have been needed to avoid breaking existing codebases and make it configurable.

 

/Per

Coordinator
May 16, 2012 at 9:41 AM
Edited May 16, 2012 at 9:42 AM

Its been a bug for a long time and I just got round to fixing it. There is only 1 person fixing StyleCop (in his spare time) for 200,000 users.

May 16, 2012 at 9:46 AM
shouldn't "static readonly" be treated the same as a constant when it
comes to naming?

On Wed, May 16, 2012 at 5:41 AM, [email removed] wrote:
> From: andyr
>
> Its been a bug for a long time and I just got round to fixing it.
>
> Read the full discussion online.
>
> To add a post to this discussion, reply to this email
> ([email removed])
>
> To start a new discussion for this project, email
> [email removed]
>
> You are receiving this email because you subscribed to this discussion on
> CodePlex. You can unsubscribe or change your settings on codePlex.com.
>
> Please note: Images and attachments will be removed from emails. Any posts
> to this discussion will also be available online at codeplex.com



--
thanks

cliff
Coordinator
May 16, 2012 at 9:57 AM

Public static readonly - yes. 

private - no.

May 16, 2012 at 11:59 AM
are the rules for a private const also to be camelCased? That seems a
bit inconsistent.

On Wed, May 16, 2012 at 5:58 AM, [email removed] wrote:
> From: andyr
>
> Public static readonly - yes.
>
> private - no.
>
> Read the full discussion online.
>
> To add a post to this discussion, reply to this email
> ([email removed])
>
> To start a new discussion for this project, email
> [email removed]
>
> You are receiving this email because you subscribed to this discussion on
> CodePlex. You can unsubscribe or change your settings on codePlex.com.
>
> Please note: Images and attachments will be removed from emails. Any posts
> to this discussion will also be available online at codeplex.com



--
thanks

cliff
Coordinator
May 16, 2012 at 12:41 PM

All Const AaBb style.

May 16, 2012 at 12:46 PM
Ok, so static readonly is like a const and should follow the same
rules as const whether public or private.

On Wed, May 16, 2012 at 8:41 AM, [email removed] wrote:
> From: andyr
>
> All Const AaBb style.
>
> Read the full discussion online.
>
> To add a post to this discussion, reply to this email
> ([email removed])
>
> To start a new discussion for this project, email
> [email removed]
>
> You are receiving this email because you subscribed to this discussion on
> CodePlex. You can unsubscribe or change your settings on codePlex.com.
>
> Please note: Images and attachments will be removed from emails. Any posts
> to this discussion will also be available online at codeplex.com



--
thanks

cliff
Coordinator
May 16, 2012 at 12:53 PM
Edited May 16, 2012 at 12:57 PM

No. Like this:

                switch (kindOfElement)
                {
                    case NamedElementKinds.Locals:
                    case NamedElementKinds.Parameters:
                    case NamedElementKinds.PrivateInstanceFields:
                    case NamedElementKinds.PrivateStaticFields:
                    case NamedElementKinds.PrivateStaticReadonly:
                        rule.Prefix = string.Empty;
                        rule.NamingStyleKind = NamingStyleKinds.aaBb;
                        break;
                    case NamedElementKinds.Interfaces:
                        rule.Prefix = "I";
                        rule.NamingStyleKind = NamingStyleKinds.AaBb;
                        break;
                    case NamedElementKinds.TypeParameters:
                        rule.Prefix = "T";
                        rule.NamingStyleKind = NamingStyleKinds.AaBb;
                        break;
                    default:
                        rule.Prefix = string.Empty;
                        rule.NamingStyleKind = NamingStyleKinds.AaBb;
                        break;
                }

May 30, 2012 at 3:04 AM

I didn't see the justification or discussion in this rule change -- all of the (StyleCop-compliant) codebases I have seen use UpperCamelCase for const as well as static readonly fields.

Coordinator
May 30, 2012 at 7:26 AM
Edited Jun 7, 2012 at 6:16 PM
There is a discussion thread and/or bug on it here somewhere. (can't access web at present)
Public static - uppercase
Private static - lowercase
If I recall.
Jun 3, 2012 at 1:24 AM

I filed http://stylecop.codeplex.com/workitem/7052 dealing with one part of this, but I too never intended for "private static readonly" fields to be forced lowercase.  The change back then allowed both upper and lower and now it's forced to lower.  Private static readonly's are essentially constants and should be treated the same as those.

Perhaps this should be one of the few values that are configurable as it essentially invalidates thousands of files here for myself.  I cannot upgrade from a previous version until this is fixed...

Jun 3, 2012 at 4:17 PM

I'll chime in on this again.  Since a static, readonly field can never be changed, it should be treated like a const field regardless of it's scope.  If I could declare a const instantiation of a class or struct, i would, but I can't.  static, readonly allows me to do that and treat it like a const in code.  I'd argue that the rule should change to treat them both the same.  Other than the code shown above, what's that argument for a private static readonly field starting with a lower-case letter?

Coordinator
Jun 3, 2012 at 10:11 PM
Edited Jun 3, 2012 at 10:13 PM

I'm tending to agree on this and cant' find the original bug that I fixed.

I'm proposing to change the naming in the next build:

PrivateStaticReadonly - 'AaBb'

The following 4 types remain as 'aaBb':

Locals - Parameters - PrivateInstanceFields - PrivateStaticFields -

 

Is this what everyone is expecting?

Coordinator
Jun 4, 2012 at 2:02 AM

Next build has new rule SA1311:StaticReadonlyFieldsMustBeginWithUpperCaseLetter

Jun 4, 2012 at 12:35 PM

Cool, thanks Andy.

Jun 4, 2012 at 3:57 PM

Great, that change will allow me to upgrade. Thanks!

Jun 5, 2012 at 6:12 AM

Does version 4.7.28.0 include the new rule SA1311? Because I just upgraded but I'm still getting the warning.

Coordinator
Jun 5, 2012 at 9:01 AM
4.7.29 has the new rule


~A.
Jun 6, 2012 at 10:16 PM

OMG! This is very frustrating. I try to stay up to date, but these flip-flops back and forth between standards should not happen this fast. I am getting close to uninstalling. How many more times am I going to have to reformat hundreds of files because this changes?

Coordinator
Jun 6, 2012 at 10:37 PM
Edited Jun 7, 2012 at 6:18 PM
Very sorry. - but it is a flip-flop not flip-flops. I fixed a long standing bug and on consultation with the active community I rolled back the change and introduced a new rule instead. Turn the new rule off if you don't want to comply or can't comply yet.
Aug 1, 2012 at 8:06 AM
Edited Aug 1, 2012 at 8:07 AM

I believe SA1311 should not apply to private static readonly fields.

What if I have

private static readonly Foo Bar;

and I want to encapsulate this field in a getter. What do I call it? Obviously I can't do

public static Foo Bar { get { return Bar; } }

because the names clash. If I were allowed to have the field as lower case then I could encapsulate it as usual, i.e.

public static Foo Bar { get { return bar; } }

As an aside, the solution here is not to make the field public. Fields should not be public, but that is a separate discussion.

Furthermore, private static is lower and private readonly is lower so why, when we combine the two, should we suddenly switch to upper? Regarding the argument that "it is like a constant", a static readonly field may act effectively, as a constant, but it is not a constant. Public constants are compiled into consuming libraries, static readonly fields are not (although as mentioned above fields should anyway not be declared public).

Feb 11, 2013 at 7:15 AM
Edited Feb 11, 2013 at 7:28 AM
Like Adam says; How are we supposed to encapsulate static fields in property getters?
Right now the only way -without disabling the rule- is giving them different names, like:
private static Foo BarField;

public static Foo Bar
{
    get { return BarField; }
}
And that makes no sense at all. I mean;
Isn't one of the earliest rules that came out was the the removal of any m and underscore prefixes from field names?
Shouldn't be a way to encapsulate a field without using a prefix or suffix?
(Please don't go down the "just make the fields public." path.)

It was good when we were able to name a private static field as either AaBb or aaBb IMO.
But if StyleCop will enforce only one naming style for private static fields, AaBb should not be the one.