Merge pull request #1075 from hvitved/csharp/get-location-to-string

C#: Simplify dispatch hierarchy for `getLocation()` and `toString()`

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

@@ -10,70 +10,4 @@
  *       maintainability
  */
 
-import csharp
-import semmle.code.csharp.commons.Strings
-import semmle.code.csharp.frameworks.System
-
-/**
- * Holds if expression `e`, of type `t`, invokes `ToString()` either explicitly
- * or implicitly.
- */
-predicate invokesToString(Expr e, ValueOrRefType t) {
-  // Explicit invocation
-  exists(MethodCall mc | mc.getQualifier() = e |
-    mc.getTarget() instanceof ToStringMethod and
-    t = mc.getQualifier().getType()
-  )
-  or
-  // Explicit or implicit invocation
-  e instanceof ImplicitToStringExpr and
-  t = e.stripCasts().getType()
-  or
-  // Implicit invocation via forwarder method
-  t = e.stripCasts().getType() and
-  not t instanceof StringType and
-  exists(AssignableDefinitions::ImplicitParameterDefinition def, Parameter p, ParameterRead pr |
-    e = p.getAnAssignedArgument()
-  |
-    def.getParameter() = p and
-    pr = def.getAReachableRead() and
-    pr.getAControlFlowNode().postDominates(p.getCallable().getEntryPoint()) and
-    invokesToString(pr, _)
-  )
-}
-
-/**
- * Holds if `t`, or any sub type of `t`, inherits the default `ToString()`
- * method from `System.Object` or `System.ValueType`.
- */
-predicate alwaysDefaultToString(ValueOrRefType t) {
-  exists(ToStringMethod m | t.hasMethod(m) |
-    m.getDeclaringType() instanceof SystemObjectClass or
-    m.getDeclaringType() instanceof SystemValueTypeClass
-  ) and
-  not exists(RefType overriding |
-    overriding.getAMethod() instanceof ToStringMethod and
-    overriding.getABaseType+() = t
-  ) and
-  ((t.isAbstract() or t instanceof Interface) implies not t.isEffectivelyPublic())
-}
-
-class DefaultToStringType extends ValueOrRefType {
-  DefaultToStringType() { alwaysDefaultToString(this) }
-
-  // A workaround for generating empty URLs for non-source locations, because qltest
-  // does not support non-source locations
-  override Location getLocation() {
-    result = super.getLocation() and
-    result instanceof SourceLocation
-    or
-    not super.getLocation() instanceof SourceLocation and
-    result instanceof EmptyLocation
-  }
-}
-
-from Expr e, DefaultToStringType t
-where invokesToString(e, t)
-select e,
-  "Default 'ToString()': $@ inherits 'ToString()' from 'Object', and so is not suitable for printing.",
-  t, t.getName()
+import DefaultToString

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

@@ -0,0 +1,57 @@
+import csharp
+import semmle.code.csharp.commons.Strings
+import semmle.code.csharp.frameworks.System
+
+/**
+ * Holds if expression `e`, of type `t`, invokes `ToString()` either explicitly
+ * or implicitly.
+ */
+predicate invokesToString(Expr e, ValueOrRefType t) {
+  // Explicit invocation
+  exists(MethodCall mc | mc.getQualifier() = e |
+    mc.getTarget() instanceof ToStringMethod and
+    t = mc.getQualifier().getType()
+  )
+  or
+  // Explicit or implicit invocation
+  e instanceof ImplicitToStringExpr and
+  t = e.stripCasts().getType()
+  or
+  // Implicit invocation via forwarder method
+  t = e.stripCasts().getType() and
+  not t instanceof StringType and
+  exists(AssignableDefinitions::ImplicitParameterDefinition def, Parameter p, ParameterRead pr |
+    e = p.getAnAssignedArgument()
+  |
+    def.getParameter() = p and
+    pr = def.getAReachableRead() and
+    pr.getAControlFlowNode().postDominates(p.getCallable().getEntryPoint()) and
+    invokesToString(pr, _)
+  )
+}
+
+/**
+ * Holds if `t`, or any sub type of `t`, inherits the default `ToString()`
+ * method from `System.Object` or `System.ValueType`.
+ */
+predicate alwaysDefaultToString(ValueOrRefType t) {
+  exists(ToStringMethod m | t.hasMethod(m) |
+    m.getDeclaringType() instanceof SystemObjectClass or
+    m.getDeclaringType() instanceof SystemValueTypeClass
+  ) and
+  not exists(RefType overriding |
+    overriding.getAMethod() instanceof ToStringMethod and
+    overriding.getABaseType+() = t
+  ) and
+  ((t.isAbstract() or t instanceof Interface) implies not t.isEffectivelyPublic())
+}
+
+class DefaultToStringType extends ValueOrRefType {
+  DefaultToStringType() { alwaysDefaultToString(this) }
+}
+
+query predicate problems(Expr e, string s, DefaultToStringType t, string name) {
+  invokesToString(e, t) and
+  s = "Default 'ToString()': $@ inherits 'ToString()' from 'Object', and so is not suitable for printing." and
+  name = t.getName()
+}

csharp/ql/src/external/ExternalArtifact.qll

@@ -1,6 +1,17 @@
 import csharp
 
-class ExternalDefect extends @externalDefect, Element {
+class ExternalElement extends @external_element {
+  /** Gets a textual representation of this element. */
+  string toString() { none() }
+
+  /** Gets the location of this element. */
+  Location getLocation() { none() }
+
+  /** Gets the file containing this element. */
+  File getFile() { result = getLocation().getFile() }
+}
+
+class ExternalDefect extends ExternalElement, @externalDefect {
   string getQueryPath() {
     exists(string path |
       externalDefects(this, path, _, _, _) and
@@ -19,7 +30,7 @@ class ExternalDefect extends @externalDefect, Element {
   }
 }
 
-class ExternalMetric extends @externalMetric, Element {
+class ExternalMetric extends ExternalElement, @externalMetric {
   string getQueryPath() { externalMetrics(this, result, _, _) }
 
   float getValue() { externalMetrics(this, _, _, result) }
@@ -29,7 +40,7 @@ class ExternalMetric extends @externalMetric, Element {
   override string toString() { result = getQueryPath() + ": " + getLocation() + " - " + getValue() }
 }
 
-class ExternalData extends @externalDataElement {
+class ExternalData extends ExternalElement, @externalDataElement {
   string getDataPath() { externalData(this, result, _, _) }
 
   string getQueryPath() { result = getDataPath().regexpReplaceAll("\\.[^.]*$", ".ql") }
@@ -44,7 +55,7 @@ class ExternalData extends @externalDataElement {
 
   date getFieldAsDate(int index) { result = getField(index).toDate() }
 
-  string toString() { result = getQueryPath() + ": " + buildTupleString(0) }
+  override string toString() { result = getQueryPath() + ": " + buildTupleString(0) }
 
   private string buildTupleString(int start) {
     start = getNumFields() - 1 and result = getField(start)

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

@@ -18,8 +18,18 @@ class Attributable extends @attributable {
   /** Gets a textual representation of this element. */
   string toString() { none() }
 
-  /** Gets the location of this element, if any. */
-  Location getLocation() { none() }
+  /**
+   * Holds if this element is at the specified location.
+   * The location spans column `startcolumn` of line `startline` to
+   * column `endcolumn` of line `endline` in file `filepath`.
+   * For more information, see
+   * [Locations](https://help.semmle.com/QL/learn-ql/ql/locations.html).
+   */
+  predicate hasLocationInfo(
+    string filepath, int startline, int startcolumn, int endline, int endcolumn
+  ) {
+    this.(Element).getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
+  }
 }
 
 /**

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

@@ -283,8 +283,6 @@ class Method extends Callable, Virtualizable, Attributable, @method {
 
   override string toString() { result = Callable.super.toString() }
 
-  override Location getLocation() { result = Callable.super.getLocation() }
-
   override Parameter getRawParameter(int i) {
     if this.isStatic() then result = this.getParameter(i) else result = this.getParameter(i - 1)
   }
@@ -354,8 +352,6 @@ class Constructor extends DotNet::Constructor, Callable, Member, Attributable, @
 
   override string toString() { result = Callable.super.toString() }
 
-  override Location getLocation() { result = Callable.super.getLocation() }
-
   override Parameter getRawParameter(int i) {
     if this.isStatic() then result = this.getParameter(i) else result = this.getParameter(i - 1)
   }
@@ -419,8 +415,6 @@ class Destructor extends DotNet::Destructor, Callable, Member, Attributable, @de
   override Location getALocation() { destructor_location(this, result) }
 
   override string toString() { result = Callable.super.toString() }
-
-  override Location getLocation() { result = Callable.super.getLocation() }
 }
 
 /**
@@ -447,8 +441,6 @@ class Operator extends Callable, Member, Attributable, @operator {
 
   override string toString() { result = Callable.super.toString() }
 
-  override Location getLocation() { result = Callable.super.getLocation() }
-
   override Parameter getRawParameter(int i) { result = getParameter(i) }
 }
 

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

@@ -22,7 +22,7 @@ class Element extends DotNet::Element, @element {
    * Where an element has multiple locations (for example a source file and an assembly),
    * gets only the source location.
    */
-  override Location getLocation() { result = ExprOrStmtParentCached::bestLocation(this) }
+  final override Location getLocation() { result = ExprOrStmtParentCached::bestLocation(this) }
 
   /** Gets a location of this element, including sources and assemblies. */
   override Location getALocation() { none() }

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

@@ -43,8 +43,6 @@ class DeclarationWithAccessors extends AssignableMember, Virtualizable, Attribut
   override Type getType() { none() }
 
   override string toString() { result = AssignableMember.super.toString() }
-
-  override Location getLocation() { result = AssignableMember.super.getLocation() }
 }
 
 /**
@@ -360,8 +358,6 @@ class Accessor extends Callable, Modifiable, Attributable, @callable_accessor {
   override Location getALocation() { accessor_location(this, result) }
 
   override string toString() { result = getName() }
-
-  override Location getLocation() { result = Callable.super.getLocation() }
 }
 
 /**

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

@@ -362,8 +362,6 @@ class ValueOrRefType extends DotNet::ValueOrRefType, Type, Attributable, @value_
   }
 
   override string toString() { result = Type.super.toString() }
-
-  override Location getLocation() { result = Type.super.getLocation() }
 }
 
 /**

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

@@ -222,8 +222,6 @@ class Parameter extends DotNet::Parameter, LocalScopeVariable, Attributable, Top
 
   override string toString() { result = this.getName() }
 
-  override Location getLocation() { result = LocalScopeVariable.super.getLocation() }
-
   /**
    * Gets the default value of this parameter, if any. For example, the
    * default value of `numberOfTries` is `3` in
@@ -400,8 +398,6 @@ class Field extends Variable, AssignableMember, Attributable, TopLevelExprParent
   override FieldAccess getAnAccess() { result = Variable.super.getAnAccess() }
 
   override string toString() { result = Variable.super.toString() }
-
-  override Location getLocation() { result = Variable.super.getLocation() }
 }
 
 /**

csharp/ql/src/semmle/code/csharp/exprs/Expr.qll

@@ -39,8 +39,6 @@ private import dotnet
  * (`QualifiableExpr`), or a literal (`Literal`).
  */
 class Expr extends DotNet::Expr, ControlFlowElement, @expr {
-  override string toString() { result = "Expression" }
-
   override Location getALocation() { expr_location(this, result) }
 
   /** Gets the type of this expression. */
@@ -717,3 +715,7 @@ class RefExpr extends Expr, @ref_expr {
 class DiscardExpr extends Expr, @discard_expr {
   override string toString() { result = "_" }
 }
+
+private class UnknownExpr extends Expr, @unknown_expr {
+  override string toString() { result = "Expression" }
+}

csharp/ql/test/library-tests/attributes/AttributeElements.expected

@@ -1,15 +1,15 @@
 | attributes.cs:41:7:41:9 | Foo | attributes.cs:40:2:40:22 | [AttributeUsage(...)] | System.AttributeUsageAttribute |
 | attributes.cs:44:17:44:19 | foo | attributes.cs:43:6:43:16 | [Conditional(...)] | System.Diagnostics.ConditionalAttribute |
 | attributes.cs:49:23:49:23 | x | attributes.cs:49:14:49:16 | [Foo(...)] | Foo |
-| file://:0:0:0:0 | attributes, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null | attributes.cs:10:12:10:24 | [AssemblyTitle(...)] | System.Reflection.AssemblyTitleAttribute |
-| file://:0:0:0:0 | attributes, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null | attributes.cs:11:12:11:30 | [AssemblyDescription(...)] | System.Reflection.AssemblyDescriptionAttribute |
-| file://:0:0:0:0 | attributes, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null | attributes.cs:12:12:12:32 | [AssemblyConfiguration(...)] | System.Reflection.AssemblyConfigurationAttribute |
-| file://:0:0:0:0 | attributes, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null | attributes.cs:13:12:13:26 | [AssemblyCompany(...)] | System.Reflection.AssemblyCompanyAttribute |
-| file://:0:0:0:0 | attributes, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null | attributes.cs:14:12:14:26 | [AssemblyProduct(...)] | System.Reflection.AssemblyProductAttribute |
-| file://:0:0:0:0 | attributes, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null | attributes.cs:15:12:15:28 | [AssemblyCopyright(...)] | System.Reflection.AssemblyCopyrightAttribute |
-| file://:0:0:0:0 | attributes, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null | attributes.cs:16:12:16:28 | [AssemblyTrademark(...)] | System.Reflection.AssemblyTrademarkAttribute |
-| file://:0:0:0:0 | attributes, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null | attributes.cs:17:12:17:26 | [AssemblyCulture(...)] | System.Reflection.AssemblyCultureAttribute |
-| file://:0:0:0:0 | attributes, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null | attributes.cs:22:12:22:21 | [ComVisible(...)] | System.Runtime.InteropServices.ComVisibleAttribute |
-| file://:0:0:0:0 | attributes, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null | attributes.cs:25:12:25:15 | [Guid(...)] | System.Runtime.InteropServices.GuidAttribute |
-| file://:0:0:0:0 | attributes, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null | attributes.cs:37:12:37:26 | [AssemblyVersion(...)] | System.Reflection.AssemblyVersionAttribute |
-| file://:0:0:0:0 | attributes, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null | attributes.cs:38:12:38:30 | [AssemblyFileVersion(...)] | System.Reflection.AssemblyFileVersionAttribute |
+| attributes.dll:0:0:0:0 | attributes, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null | attributes.cs:10:12:10:24 | [AssemblyTitle(...)] | System.Reflection.AssemblyTitleAttribute |
+| attributes.dll:0:0:0:0 | attributes, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null | attributes.cs:11:12:11:30 | [AssemblyDescription(...)] | System.Reflection.AssemblyDescriptionAttribute |
+| attributes.dll:0:0:0:0 | attributes, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null | attributes.cs:12:12:12:32 | [AssemblyConfiguration(...)] | System.Reflection.AssemblyConfigurationAttribute |
+| attributes.dll:0:0:0:0 | attributes, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null | attributes.cs:13:12:13:26 | [AssemblyCompany(...)] | System.Reflection.AssemblyCompanyAttribute |
+| attributes.dll:0:0:0:0 | attributes, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null | attributes.cs:14:12:14:26 | [AssemblyProduct(...)] | System.Reflection.AssemblyProductAttribute |
+| attributes.dll:0:0:0:0 | attributes, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null | attributes.cs:15:12:15:28 | [AssemblyCopyright(...)] | System.Reflection.AssemblyCopyrightAttribute |
+| attributes.dll:0:0:0:0 | attributes, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null | attributes.cs:16:12:16:28 | [AssemblyTrademark(...)] | System.Reflection.AssemblyTrademarkAttribute |
+| attributes.dll:0:0:0:0 | attributes, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null | attributes.cs:17:12:17:26 | [AssemblyCulture(...)] | System.Reflection.AssemblyCultureAttribute |
+| attributes.dll:0:0:0:0 | attributes, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null | attributes.cs:22:12:22:21 | [ComVisible(...)] | System.Runtime.InteropServices.ComVisibleAttribute |
+| attributes.dll:0:0:0:0 | attributes, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null | attributes.cs:25:12:25:15 | [Guid(...)] | System.Runtime.InteropServices.GuidAttribute |
+| attributes.dll:0:0:0:0 | attributes, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null | attributes.cs:37:12:37:26 | [AssemblyVersion(...)] | System.Reflection.AssemblyVersionAttribute |
+| attributes.dll:0:0:0:0 | attributes, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null | attributes.cs:38:12:38:30 | [AssemblyFileVersion(...)] | System.Reflection.AssemblyFileVersionAttribute |

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

@@ -0,0 +1,17 @@
+import csharp
+import Useless_code.DefaultToString
+
+class MyDefaultToStringType extends DefaultToStringType {
+  // A workaround for generating empty URLs for non-source locations, because qltest
+  // does not support non-source locations
+  override predicate hasLocationInfo(
+    string filepath, int startline, int startcolumn, int endline, int endcolumn
+  ) {
+    exists(Location l | l = this.getLocation() |
+      if l instanceof SourceLocation
+      then l.hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
+      else
+        any(EmptyLocation el).hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
+    )
+  }
+}

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

@@ -1 +0,0 @@
-Useless code/DefaultToString.ql