Merge pull request github#6084 from tamasvajk/feature/effective-publicness

C#: Fix isEffectively* visibility predicates

Author: tamasvajk

csharp/change-notes/2021-06-15-effective-visibility.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* The `isEffectivelyPrivate`, `isEffectivelyInternal` and `isEffectivelyPublic` predicates on `Modifiable` have been reworked to handle `private protected` and `internal protected` visibilities and explicitly implemented interfaces.

csharp/ql/src/semmle/code/csharp/Member.qll

@@ -96,27 +96,59 @@ class Modifiable extends Declaration, @modifiable {
   /** Holds if this declaration is `async`. */
   predicate isAsync() { this.hasModifier("async") }
 
+  private predicate isReallyPrivate() {
+    this.isPrivate() and
+    not this.isProtected() and
+    // Rare case when a member is defined with the same name in multiple assemblies with different visibility
+    not this.isPublic()
+  }
+
   /**
-   * Holds if this declaration is effectively `private` (either directly or
-   * because one of the enclosing types is `private`).
+   * Holds if this declaration is effectively `private`. A declaration is considered
+   * effectively `private` if it can only be referenced from
+   * - the declaring and its nested types, similarly to `private` declarations, and
+   * - the enclosing types.
+   *
+   * Note that explicit interface implementations are also considered effectively
+   * `private` if the implemented interface is itself effectively `private`. Finally,
+   * `private protected` members are not considered effectively `private`, because
+   * they can be overriden within the declaring assembly.
    */
   predicate isEffectivelyPrivate() {
-    this.isPrivate() or
-    this.getDeclaringType+().isPrivate()
+    this.isReallyPrivate() or
+    this.getDeclaringType+().(Modifiable).isReallyPrivate() or
+    this.(Virtualizable).getExplicitlyImplementedInterface().isEffectivelyPrivate()
+  }
+
+  private predicate isReallyInternal() {
+    (
+      this.isInternal() and not this.isProtected()
+      or
+      this.isPrivate() and this.isProtected()
+    ) and
+    // Rare case when a member is defined with the same name in multiple assemblies with different visibility
+    not this.isPublic()
   }
 
   /**
-   * Holds if this declaration is effectively `internal` (either directly or
-   * because one of the enclosing types is `internal`).
+   * Holds if this declaration is effectively `internal`. A declaration is considered
+   * effectively `internal` if it can only be referenced from the declaring assembly.
+   *
+   * Note that friend assemblies declared in `InternalsVisibleToAttribute` are not
+   * considered. Explicit interface implementations are also considered effectively
+   * `internal` if the implemented interface is itself effectively `internal`. Finally,
+   * `internal protected` members are not considered effectively `internal`, because
+   * they can be overriden outside the declaring assembly.
    */
   predicate isEffectivelyInternal() {
-    this.isInternal() or
-    this.getDeclaringType+().isInternal()
+    this.isReallyInternal() or
+    this.getDeclaringType+().(Modifiable).isReallyInternal() or
+    this.(Virtualizable).getExplicitlyImplementedInterface().isEffectivelyInternal()
   }
 
   /**
-   * Holds if this declaration is effectively `public`, because it
-   * and all enclosing types are `public`.
+   * Holds if this declaration is effectively `public`, meaning that it can be
+   * referenced outside the declaring assembly.
    */
   predicate isEffectivelyPublic() { not isEffectivelyPrivate() and not isEffectivelyInternal() }
 }
@@ -159,6 +191,11 @@ class Virtualizable extends Member, @virtualizable {
     implementsExplicitInterface()
   }
 
+  override predicate isPrivate() {
+    super.isPrivate() and
+    not implementsExplicitInterface()
+  }
+
   /**
    * Gets any interface this member explicitly implements; this only applies
    * to members that can be declared on an interface, i.e. methods, properties,

csharp/ql/test/library-tests/modifiers/Effectively.expected

@@ -24,3 +24,8 @@
 | Modifiers.cs:60:22:60:23 | I1 | public |
 | Modifiers.cs:62:14:62:15 | M1 | public |
 | Modifiers.cs:63:14:63:15 | M2 | public |
+| Modifiers.cs:68:14:68:15 | M1 | internal |
+| Modifiers.cs:71:18:71:19 | C2 | public |
+| Modifiers.cs:73:17:73:18 | M1 | internal |
+| Modifiers.cs:75:32:75:33 | M2 | internal |
+| Modifiers.cs:76:33:76:34 | M3 | public |

csharp/ql/test/library-tests/modifiers/Modifiers.cs

@@ -62,4 +62,17 @@ public interface I1
         void M1();
         void M2() => throw null;
     }
+
+    internal interface I2
+    {
+        void M1() => throw null;
+    }
+
+    public class C2 : I2
+    {
+        void I2.M1() => throw null;
+
+        protected private void M2() { }
+        protected internal void M3() { }
+    }
 }

csharp/ql/test/library-tests/modifiers/Modifiers.expected

@@ -49,3 +49,12 @@
 | Modifiers.cs:62:14:62:15 | M1 | file://:0:0:0:0 | public |
 | Modifiers.cs:63:14:63:15 | M2 | file://:0:0:0:0 | public |
 | Modifiers.cs:63:14:63:15 | M2 | file://:0:0:0:0 | virtual |
+| Modifiers.cs:66:24:66:25 | I2 | file://:0:0:0:0 | internal |
+| Modifiers.cs:68:14:68:15 | M1 | file://:0:0:0:0 | public |
+| Modifiers.cs:68:14:68:15 | M1 | file://:0:0:0:0 | virtual |
+| Modifiers.cs:71:18:71:19 | C2 | file://:0:0:0:0 | public |
+| Modifiers.cs:73:17:73:18 | M1 | file://:0:0:0:0 | private |
+| Modifiers.cs:75:32:75:33 | M2 | file://:0:0:0:0 | private |
+| Modifiers.cs:75:32:75:33 | M2 | file://:0:0:0:0 | protected |
+| Modifiers.cs:76:33:76:34 | M3 | file://:0:0:0:0 | internal |
+| Modifiers.cs:76:33:76:34 | M3 | file://:0:0:0:0 | protected |