Merge pull request #759 from calumgrant/cs/interface-tostring

C#: Remove FPs from cs/call-to-object-tostring

Author: rdmarsh2

change-notes/1.20/analysis-csharp.md

@@ -16,6 +16,7 @@
 | Dereferenced variable may be null (cs/dereferenced-value-may-be-null) | Improved results | The query has been rewritten from scratch, and the analysis is now based on static single assignment (SSA) forms. The query is now enabled by default in LGTM. |
 | SQL query built from user-controlled sources (cs/sql-injection), Improper control of generation of code (cs/code-injection), Uncontrolled format string (cs/uncontrolled-format-string), Clear text storage of sensitive information (cs/cleartext-storage-of-sensitive-information), Exposure of private information (cs/exposure-of-sensitive-information) | More results | Data sources have been added from user controls in `System.Windows.Forms`. |
 | Use of default ToString() (cs/call-to-object-tostring) | Fewer false positives | Results have been removed for `char` arrays passed to `StringBuilder.Append()`, which were incorrectly marked as using `ToString`. |
+| Use of default ToString() (cs/call-to-object-tostring) | Fewer results | Results have been removed when the object is an interface or an abstract class. |
 
 ## Changes to code extraction
 

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

@@ -54,6 +54,11 @@ predicate alwaysDefaultToString(ValueOrRefType t) {
   not exists(RefType overriding |
     overriding.getAMethod() instanceof ToStringMethod and
     overriding.getABaseType+() = t
+  ) and
+  (
+    (t.isAbstract() or t instanceof Interface)
+    implies
+    not t.isEffectivelyPublic()
   )
 }
 

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()
     {
@@ -29,6 +29,12 @@ void M()
 
         var sb = new StringBuilder();
         sb.Append(new char[] { 'a', 'b', 'c' }, 0, 3); // GOOD
+
+        IPrivate f = null;
+        Console.WriteLine(f);  // BAD
+
+        IPublic g = null;
+        Console.WriteLine(g);  // GOOD
     }
 
     class A
@@ -48,6 +54,14 @@ class D : C
     {
         override public string ToString() { return "D"; }
     }
+
+    public interface IPublic
+    {
+    }
+
+    private interface IPrivate
+    {
+    }
 }
 
 // semmle-extractor-options: /r:System.Runtime.Extensions.dll