Understanding SA1019 violations

Mar 21, 2011 at 1:31 PM

The message generated for each SA1019 is not very specific as to the actual cause of the violation. It would be of assistance if the message cited the exact position (line and column) of the violation.

I get the message applying to muliple lines of a LINQ expression, even to a line like:

.ToList();

Developer
Mar 22, 2011 at 7:18 PM
Edited Mar 22, 2011 at 7:28 PM

Looks like this is new behavior as well.  I copied all my StyleCop warnings from a large solution with a 4.4 installation (~850 warnings), installed the 4.5.0.3 preview (>1000 warnings) and started sifting through some results.  SA1019 didn't fire at all in 4.4 but is indeed now firing (multiple times) for fluent-style multi-line code like:

Edit: easier example:

    List<int> things = new List<int>() { 1, 1, 2, 3, 5, 10 };
    var duplicatedThings = things
        .GroupBy(i => i)
        .Where(g => g.Count() > 1)
        .Select(g => g.Key)
        .ToList();
Developer
Mar 22, 2011 at 7:36 PM

Just uninstalled 4.5, brought back 4.4.0.14 and the warnings disappear.  My settings in both installs show up as fully-defaulted in the StyleCop Settings tool.  So it's certainly acting like introduced behavior.

Mar 22, 2011 at 8:39 PM
karak wrote:

Looks like this is new behavior as well.  I copied all my StyleCop warnings from a large solution with a 4.4 installation (~850 warnings), installed the 4.5.0.3 preview (>1000 warnings) and started sifting through some results.  SA1019 didn't fire at all in 4.4 but is indeed now firing (multiple times) for fluent-style multi-line code like:

Edit: easier example:

    List<int> things = new List<int>() { 1, 1, 2, 3, 5, 10 };
var duplicatedThings = things
.GroupBy(i => i)
.Where(g => g.Count() > 1)
.Select(g => g.Key)
.ToList();

This is the sort of code that was firing the warning in my client's code.

Coordinator
Mar 22, 2011 at 8:45 PM
Edited Mar 24, 2011 at 9:50 AM

This new violation was caused by a fix to this piece of code (6 months ago but after 4.4.0 RTW):

    if (previousNode == null) 
    {
             CsTokenType tokenType = previousNode.Value.CsTokenType;  
             if (tokenType == CsTokenType.WhiteSpace ||
                    tokenType == CsTokenType.EndOfLine ||
                    tokenType == CsTokenType.SingleLineComment ||
                    tokenType == CsTokenType.MultiLineComment)
             {  
                 this.AddViolation(tokenNode.Value.FindParentElement(), tokenNode.Value.LineNumber, Rules.MemberAccessSymbolsMustBeSpacedCorrectly);
             }
     }

The previousNode == null always caused an internal NullReference exception on the line following it inside the if.

It was fixed to be previousNode != null which is what is causing it to fire the violations now.

The current rule is now firing correctly. Member Access Symbols must not have whitespace directly before it.

So thoughts please?

Developer
Mar 23, 2011 at 2:51 AM
Edited Mar 23, 2011 at 3:01 AM

Personally I think that the usage of fluent interfaces are not graceful.  There generally other alternatives.  For instance, the fluent example I wrote above could also be written as:

    List<int> things = new List<int>() { 1, 1, 2, 3, 5, 10 };
    var duplicatedThings = (from i in things
                            group i by i into g
                            where g.Count() > 1
                            select g.Key).ToList();

 



For (and against) fixing / re-enbling the rule?  Here are some of my thoughts...

+ The rule was intended and presumably documented as a rule already; this is clearly "fixing a broken rule" more than it is "introducing a new rule".

+ The style guidlines the rules are based on suggest this sort of style is not appropriate.  IMO this is similar to saying "x = y = z;" is inappropriate as it is similarly abusing the same principles.

- The rule wasn't firing, so an upgrade of StyleCop introduces new failures.

+ Upgrade introduces new failures anyway in the form of new rules.  IMO this is an accepted behavior of upgrading.

- Fluent usage wasn't around as much when the original style guidelines were introduced.

+ Fluent usage still isn't particularly popular IMO.

+ The fact that there are multiple vectors to write the same code is a detriment for consistency;  removal of fluent style helps nudge developers into a consistent style by using the non-fluent forms of accomplishing the same thing.

+ A team that makes heavy usage of fluent could always disable this rule.  IMO it is better to have the rules available and work.

+ Compared to properly supporting fluent usage (researching/implementing/supporting/testing new rules aimed towards ensuring usage of fluent styles are implemented consistently), simply fixing this rule is a breeze.

Mar 23, 2011 at 2:07 PM
andyr wrote:

This new violation was caused by a fix tothis piece of code (6 months ago but after 4.4.0 RTW):

    if (previousNode == null) 
    {
             CsTokenType tokenType = previousNode.Value.CsTokenType;  
             if (tokenType == CsTokenType.WhiteSpace ||
                    tokenType == CsTokenType.EndOfLine ||
                    tokenType == CsTokenType.SingleLineComment ||
                    tokenType == CsTokenType.MultiLineComment)
             {  
                 this.AddViolation(tokenNode.Value.FindParentElement(), tokenNode.Value.LineNumber, Rules.MemberAccessSymbolsMustBeSpacedCorrectly);
             }
     }

The previousNode == null always caused an internal NullReference exception on the line following it inside the if.

It was fixed to be previousNode != null which is what is causing it to fire the violations now.

The current rule is now firing correctly. Member Access Symbols must not have whitespace directly before it.

So thoughts please?

This is fixed with 4.5.0.5

Apr 12, 2011 at 6:21 AM

And is also fixed with 4.5.10.0