Refining CQLinq rule for nested visibility

Question

We have NDepend 5.4.1 and we want to alter the queries for field/type/method that could have lower visibility. We want the query to take the scope of the enclosing class into account when deciding whether to consider it a violation.

For example,

internal class X
{
   public int A;
   public void B() { }
   public class C
   {
      // …
   }
}

We don’t want A, B or C to generate a violation saying that any of them should be made internal. If class X was public, on the other hand, and none of A, B and C is used outside the assembly, then they should all generate violations.

To accomplish this, I added the following line to the queries:

 // consider visibility of enclosing class
 f.ParentType.Visibility < f.OptimalVisibility

So for fields, the new query looks like:

// <Name>Fields that could have a lower visibility</Name>
warnif count > 0 from f in JustMyCode.Fields where 
  f.Visibility != f.OptimalVisibility &&
 !f.HasAttribute("NDepend.Attributes.CannotDecreaseVisibilityAttribute".AllowNoMatch()) &&
 !f.HasAttribute("NDepend.Attributes.IsNotDeadCodeAttribute".AllowNoMatch()) &&
 // consider visibility of enclosing class
 f.ParentType.Visibility < f.OptimalVisibility

select new { f, 
             f.Visibility , 
             CouldBeDeclared = f.OptimalVisibility,
             f.MethodsUsingMe }

I altered the query for method visibility and type visibility in a similar manner, except for types I make sure there is an enclosing parent type:

(t.ParentType == null || t.ParentType.Visibility < t.OptimalVisibility)

At first glance and after running some tests, this seems to be doing the right thing. My question is whether this will generate any false positives or miss any violations, since I am not sure whether the enum visibility orderings (comparisons) will do the right thing in all cases.

Answer 1

Here is the NDepend.CodeModel.Visibility enumeration declaration:

   public enum Visibility {
      None = 0,
      Public = 1,
      ProtectedAndInternal = 2,
      ProtectedOrInternal = 3,
      Internal = 4,
      Protected = 5,
      Private = 6
   }

Hence x.ParentType.Visibility < x.OptimalVisibility means x parent type visibility is strictly less restrictive than x optimal visibility.

Notice that Protected is ordered as more restrictive than Internal which is not true neither Internal is more restrictive than Protected. So we had to provide an arbitrary ordering between these two visibility level.

Notice also that a method/field/nested type can be declared in a nested type (recursive) so to be correct we'd need to gather all outter types visibility.

These two facts make me thing that we could construct edge cases where your rule would return false positive, but after tinkering a bit I didn't succeed so.

What we advice is to look at your code base(s) how your custom rules behave to see if they should be modified. If it is fine, then consider it is fine until you eventually find a false positive.

---

There is a long related debate on the Eric Lippert blog about if a member of an internal type should be declared public or internal. There are solid arguments on both side, and the way our code rule is written favor the side it should be declared as internal, and your change favor the side it should be declared as public.