C#: Address review comments. Introduce Member::isEffectivelyPublic() because isEffectivelyPrivate and isEffectivelyInternal are almost always used together.

Author: calumgrant

csharp/ql/src/Language Abuse/MissedReadonlyOpportunity.ql

@@ -36,5 +36,5 @@ where
   canBeReadonly(f) and
   not f.isConst() and
   not f.isReadOnly() and
-  (f.isEffectivelyPrivate() or f.isEffectivelyInternal())
+  not f.isEffectivelyPublic()
 select f, "Field '" + f.getName() + "' can be 'readonly'."

csharp/ql/src/Likely Bugs/Collections/WriteOnlyContainer.ql

@@ -18,7 +18,7 @@ where
   v.getType() instanceof CollectionType and
   (
     v instanceof LocalVariable or
-    v = any(Field f | f.isEffectivelyPrivate() or f.isEffectivelyInternal())
+    v = any(Field f | not f.isEffectivelyPublic())
   ) and
   forex(Access a | a = v.getAnAccess() |
     a = any(ModifierMethodCall m).getQualifier() or

csharp/ql/src/Useless code/DefaultToString.ql

@@ -55,8 +55,11 @@ predicate alwaysDefaultToString(ValueOrRefType t) {
     overriding.getAMethod() instanceof ToStringMethod and
     overriding.getABaseType+() = t
   ) and
-  not t.isAbstract() and
-  not t instanceof Interface
+  (
+    (t.isAbstract() or t instanceof Interface)
+    implies
+    not t.isEffectivelyPublic()
+  )
 }
 
 newtype TDefaultToStringType = TDefaultToStringType0(ValueOrRefType t) { alwaysDefaultToString(t) }

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

@@ -107,6 +107,14 @@ class Modifiable extends Declaration, @modifiable {
     this.isInternal() or
     this.getDeclaringType+().isInternal()
   }
+
+  /**
+   * Holds if this declaration is effectively `public`, because it
+   * and all enclosing types are `public`.
+   */
+  predicate isEffectivelyPublic() {
+    not isEffectivelyPrivate() and not isEffectivelyInternal()
+  }
 }
 
 /** A declaration that is a member of a type. */

csharp/ql/src/semmle/code/csharp/dataflow/internal/Steps.qll

@@ -48,8 +48,7 @@ module Steps {
    * or because one of `m`'s enclosing types is).
    */
   private predicate isEffectivelyInternalOrPrivate(Modifiable m) {
-    m.isEffectivelyInternal() or
-    m.isEffectivelyPrivate()
+    not m.isEffectivelyPublic()
   }
 
   private predicate flowIn(Parameter p, Expr pred, AssignableRead succ) {

csharp/ql/src/semmle/code/csharp/dispatch/Dispatch.qll

@@ -294,7 +294,7 @@ private module Internal {
       |
         succ.(AssignableRead) = a.getAnAccess() and
         pred = a.getAnAssignedValue() and
-        a = any(Modifiable m | m.isEffectivelyInternal() or m.isEffectivelyPrivate())
+        a = any(Modifiable m | not m.isEffectivelyPublic())
       )
     }
 

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

@@ -15,3 +15,9 @@
 | Modifiers.cs:41:20:41:21 | F1 | internal |
 | Modifiers.cs:43:26:43:27 | F2 | internal |
 | Modifiers.cs:47:30:47:31 | F4 | internal |
+| Modifiers.cs:52:19:52:19 | S | public |
+| Modifiers.cs:54:20:54:21 | P1 | public |
+| Modifiers.cs:54:36:54:38 | get_P1 | public |
+| Modifiers.cs:54:52:54:54 | set_P1 | public |
+| Modifiers.cs:55:20:55:21 | P2 | public |
+| Modifiers.cs:55:36:55:38 | get_P2 | public |

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

@@ -7,5 +7,7 @@ where
     m.isEffectivelyInternal() and not m.isInternal() and s = "internal"
     or
     m.isEffectivelyPrivate() and not m.isPrivate() and s = "private"
+    or 
+    m.isEffectivelyPublic() and s = "public"
   )
 select m, s

csharp/ql/test/query-tests/Useless Code/DefaultToString/DefaultToString.cs

@@ -1,7 +1,7 @@
 using System;
 using System.Text;
 
-class DefaultToString
+public class DefaultToString
 {
     void M()
     {
@@ -32,6 +32,11 @@ void M()
 
         IInterface f = null;
         Console.WriteLine(f);  // GOOD
+        IPrivate f = null;
+        Console.WriteLine(f);  // BAD
+
+        IPublic g = null;
+        Console.WriteLine(g);  // GOOD
     }
 
     class A
@@ -51,8 +56,12 @@ class D : C
     {
         override public string ToString() { return "D"; }
     }
-    
-    interface IInterface
+
+    public interface IPublic
+    {
+    }
+
+    private interface IPrivate
     {
     }
 }

csharp/ql/test/query-tests/Useless Code/DefaultToString/DefaultToString.expected

@@ -1,7 +1,8 @@
-| DefaultToString.cs:9:27:9:27 | access to local variable d | Default 'ToString()': $@ inherits 'ToString()' from 'Object', and so is not suitable for printing. | DefaultToString.cs:4:7:4:21 | DefaultToString | DefaultToString |
-| DefaultToString.cs:10:28:10:28 | access to local variable d | Default 'ToString()': $@ inherits 'ToString()' from 'Object', and so is not suitable for printing. | DefaultToString.cs:4:7:4:21 | DefaultToString | DefaultToString |
+| DefaultToString.cs:9:27:9:27 | access to local variable d | Default 'ToString()': $@ inherits 'ToString()' from 'Object', and so is not suitable for printing. | DefaultToString.cs:4:14:4:28 | DefaultToString | DefaultToString |
+| DefaultToString.cs:10:28:10:28 | access to local variable d | Default 'ToString()': $@ inherits 'ToString()' from 'Object', and so is not suitable for printing. | DefaultToString.cs:4:14:4:28 | DefaultToString | DefaultToString |
 | DefaultToString.cs:16:27:16:30 | access to local variable ints | Default 'ToString()': $@ inherits 'ToString()' from 'Object', and so is not suitable for printing. |  | Int32[] | Int32[] |
 | DefaultToString.cs:19:24:19:27 | access to local variable ints | Default 'ToString()': $@ inherits 'ToString()' from 'Object', and so is not suitable for printing. |  | Int32[] | Int32[] |
+| DefaultToString.cs:31:27:31:27 | access to local variable f | Default 'ToString()': $@ inherits 'ToString()' from 'Object', and so is not suitable for printing. | DefaultToString.cs:59:23:59:30 | IPrivate | IPrivate |
 | DefaultToStringBad.cs:8:35:8:35 | access to local variable p | Default 'ToString()': $@ inherits 'ToString()' from 'Object', and so is not suitable for printing. | DefaultToStringBad.cs:14:11:14:16 | Person | Person |
 | DefaultToStringBad.cs:11:38:11:41 | access to local variable ints | Default 'ToString()': $@ inherits 'ToString()' from 'Object', and so is not suitable for printing. |  | Int32[] | Int32[] |
 | WriteLineArray.cs:7:23:7:26 | access to parameter args | Default 'ToString()': $@ inherits 'ToString()' from 'Object', and so is not suitable for printing. |  | String[] | String[] |