[SA1126] and "Redundant name qualifier"

May 30, 2012 at 5:52 AM

I have an abstract base class A, and a non abstract child class B.

 

Parent class A contains a protected static method "TheParentProtectedStaticMethod". Calling this in child class B, with no name qualifier raises SA1126:

 

SA1126: The call to TheParentProtectedStaticMethod must begin with the 'this.', 'base.', 'object.' or 'TheAbstractParentClass.' prefix to indicate the intended method call.

 

The problem is that if I add the correct qualifier, which is the name of the parent class (TheAbstractParentClass), I am then violating a R# setting "Remove redundant qualifier". Obviously in these cases I would change my R# settings to match StyleCop, however if I disable this check I will potentially miss out on many other places where a redundant name qualifier may be in use, which presents another style issue.

 

Do I need to raise this issue with R#, to have more granular control over the redundant name qualifier rule?

 

I have just noticed this after upgrading to 4.7.27.0. When I upgrade, I do not let StyleCop reset my settings, I look out for any changes and modify R# settings myself.

Coordinator
May 30, 2012 at 10:20 AM

This is all correct. If you turn off the R# check you'll be fine as StyleCop will find any issues.

May 30, 2012 at 12:48 PM

I turned off this setting, however I don't think StyleCop is catching what I think should be style violations.

For example:

using System;

DateTime test;
System.DateTime test2;

 

With the R# setting 'Remove redundant qualifier', the second declaration would have the 'System' qualifier highlighted as being redundant, and a quick fix would remove it. Without this setting StyleCop does not complain meaning that the same type could be declared in multiple ways throughout the file.

As an example with a longer namespace, none of the following are flagged in any way:

NameG test;
NameA.NameB.NameC.NameD.NameE.NameF.NameG test2;
NameC.NameD.NameE.NameF.NameG test3;
NameE.NameF.NameG test4;

Coordinator
May 30, 2012 at 1:18 PM

So  a couple of options.

1. Dont' turn it off the R# check. Then either Supress the StyleCop violation or turn the rule off.

2. Turn off the R# check - and risk having some other prrfixes you could remove.

Remember the big difference here is that StyleCop can only 'see' the codepane you are in and analyse that. R# compiles the code and has full access to much more data.

May 30, 2012 at 11:48 PM

Is it possible for StyleCop to look at the using directives at the top of the file, and from that work out which name qualifiers would be redundant?

 

If this is not possible I will submit a request to JetBrains to see if they can expand this code inspection.

May 31, 2012 at 1:50 AM

Did this change in 4.7.27? I have code that had no warnings with 4.7.23 but now produces lots of SA1126 errors.

 

One example

class BaseClass { protected X MyProperty { get; set; }

class DerivedClass : BaseClass

{

public DerivedClass()

{

MyProperty = blah;

}

}

There is only one possible resolution for MyProperty, but stylecop 4.7.27 is now wanting a this/base/object prefix on the usage of it in the derived class - which previously it didn't. If I add the prefix R# marks it as redundant - but either way, this used to pass stylecop without the prefix.

Coordinator
May 31, 2012 at 9:51 AM

Yes it did. The rules for checking base. and this. were completely re-written. Bacause StyleCop doesn't have access to all the methods on the base class (it only sees the codefile its analysing)  it cannot be sure. You can prefix it or suppress it.

May 31, 2012 at 5:04 PM

Good to know. Thanks for the info Andy.

Jun 3, 2012 at 11:34 AM

I posted on the R# discussion forums and an issue has been opened.

 

http://devnet.jetbrains.com/message/5460793#5460793

 

I am not quite sure what the interpretation of the rule should be. Assuming no limitations in the R# and StyleCop inspection engines, should a base class static member be prefixed or not?

Coordinator
Jun 3, 2012 at 2:42 PM

Happy for some debate on this. What do people think? Should static members of a base class be prefixed with the name of the base type?

Jun 3, 2012 at 4:36 PM
i'd vote for it should not but i don't have a good reason.

On Sun, Jun 3, 2012 at 9:42 AM, [email removed] wrote:
> From: andyr
>
> Happy for some debate on this. What do people think? Should static members
> of a base class be prefixed with the name of the base type?
>
> 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
Jun 5, 2012 at 2:53 PM
Currently we have the following situation for methods. I'll ignore cases where there are different overloads in different base classes and virtual/non-virtual.
  1. Non-static methods always need a qualifier: this, or base (if overriding or overwriting and the base method is to be called). When missing, in the first case there is SA1101, in the second case SA1126 (since we don't know if the method in the same class or in the base class is to be called).
  2. Static methods in the same class have no qualifier (name of the class). If there is one, then Resharper complains that it is redundant. StyleCop doesn't care (i.e. there is no warning if the qualifier is there or if it isn't there).
  3. Static methods in a class of a different namespace need a qualifier (otherwise the compiler doesn't know what to call). The exception: If the current class is derived from that class in a different namespace, then it behaves like the next item.
  4. Static methods in a base class have the problem mentioned in this post: Resharper says the class name is redundant. StyleCop says the class name is necessary (otherwise there is SA1126).

The problem has an added difficulty: If I call this.Equals(a) in my class, if I don't have the appropriate method in my class, then the method in the base class is called. Since object defines this method, it will be called (if there are no classes in between). If I later add the method to my class, the compiler will bind to that method instead.

If I call the static methods Equals(a, b) in my class (without a qualifier), then the same behaviour occurs. However, if I add the qualifier, then that exact method is always called. Note that I can't call Foo.Equals(a, b) until that method is defined, in contrast to a non-static methods where this.Equals(a) works.

Now my recommendation:

For consistency with non-static methods, it would be better to always add the class name for static calls. However, due to the different behaviour for static method calls, I think that not adding the class name (unless necessary) would be better, i.e. the way Resharper currently handles it.

Some sample code to see the different calling possibilities:

namespace Foo1
{
    public class Bar1
    {
        public void M1()
        {
            S1();
            Bar1.S1();

            M1();
            this.M1();
        }

        public static void S1()
        {
        }
    }

    public class Bar2 : Bar1
    {
        public void M2()
        {
            S1();
            Bar1.S1();

            M1();
            this.M1();
        }

        public new void M1()
        {
            base.M1();

            S2();
            Bar2.S2();
        }

        public static void S2()
        {
            S1();
            Bar1.S1();
        }
    }
}

namespace Foo2
{
    using Foo1;

    public class Bar3
    {
        public void M1()
        {
            Bar1.S1();
        }
    }

    public class Bar4 : Bar2
    {
        public new void M1()
        {
            S1();
            Bar1.S1();

            S2();
            Bar2.S2();

            base.M1();
        }

        public new static void S1()
        {
        }
    }
}
Coordinator
Jun 5, 2012 at 3:31 PM

Thanks Daniel, great summary. The issue here is that StyleCop cannot detect when not to report a SA1126 violation. It has no access to the base classes methods to check whereas ReSharper does because it compiles the code to analyse it. So, in order to comply with your recommendation we'd have to insist on the prefix and then you can suppress when you deem it not required. Thoughts?

Jun 5, 2012 at 6:02 PM
I'd argue that if SC can't consistently determine something, then it
shouldn't consistently enforce it.

On Tue, Jun 5, 2012 at 10:31 AM, andyr <notifications@codeplex.com> wrote:
> From: andyr
>
> Thanks Daniel, great summary. The issue here is that StyleCop cannot detect
> when not to report a SA1126 violation. It has no access to the base classes
> methods to check whereas ReSharper does because it compiles the code to
> analyse it. So, in order to comply with your recommendation we'd have to
> insist on the prefix and then you can suppress when you deem it not
> required. Thoughts?
>
> 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
Jun 5, 2012 at 6:35 PM
Edited Jun 6, 2012 at 12:48 AM
I did suggest deleting sa1100 sa1101 and sa1126 completely.
(all 3 of these cannot be completely sure in all cases)


~A.
Jun 6, 2012 at 8:07 AM

Can you see if it is a static method call? If you can, my suggestion would be to turn off SA1126 for static methods.

Coordinator
Jun 6, 2012 at 10:26 AM
Edited Jun 6, 2012 at 10:19 PM
Hi Daniel. No. All stylecop has is :
1. The text of the method call
2. The names of the methods of the class we are analysing
3. The names of any methods appearing in any other partial parts of this class
4. The name of the base class of this class.
Jun 6, 2012 at 11:12 AM

In that case I would suggest removing SA1126 (at least until you have a way to determine which calls are static). Or alternatively have SA1126 be off by default.

Coordinator
Jun 6, 2012 at 11:13 AM
Edited Jun 6, 2012 at 10:19 PM
Same logic applies to 1100 and 1101 too.
1100, 1101 and 1126 cannot always be 100% certain that an issue exists.
If we think 1126 should be removed then so should the others.
Jun 6, 2012 at 10:31 PM
remove them all or default to unchecked.

On Wed, Jun 6, 2012 at 6:14 AM, [email removed] wrote:
> From: andyr
>
> Same logic applies to 1100 and 1101 too.
>
> 1100, 1101 and 1126 cannot always be 100% certain that an issue exists.
>
>
> ~A.
>
> On 6 Jun 2012, at 11:12, DanielRose <[email removed]> wrote:
>
> From: DanielRose
>
> In that case I would suggest removing SA1126 (at least until you have a way
> to determine which calls are static). Or alternatively have SA1126 be off by
> default.
>
> 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