C++: Cut down on conversions between dbtypes and Element

cpp/ql/src/AlertSuppression.ql

@@ -47,21 +47,16 @@ class SuppressionComment extends CppStyleComment {
 
   /** Gets the scope of this suppression. */
   SuppressionScope getScope() {
-    this = result.getSuppressionComment()
+    result = this
   }
 }
 
 /**
  * The scope of an alert suppression comment.
  */
-class SuppressionScope extends @comment {
+class SuppressionScope extends ElementBase {
   SuppressionScope() {
-    mkElement(this) instanceof SuppressionComment
-  }
-
-  /** Gets a suppression comment with this scope. */
-  SuppressionComment getSuppressionComment() {
-    result = mkElement(this)
+    this instanceof SuppressionComment
   }
 
   /**
@@ -72,12 +67,7 @@ class SuppressionScope extends @comment {
   * [LGTM locations](https://lgtm.com/help/ql/locations).
   */
   predicate hasLocationInfo(string filepath, int startline, int startcolumn, int endline, int endcolumn) {
-    mkElement(this).(SuppressionComment).covers(filepath, startline, startcolumn, endline, endcolumn)
-  }
-
-  /** Gets a textual representation of this element. */
-  string toString() {
-    result = "suppression range"
+    this.(SuppressionComment).covers(filepath, startline, startcolumn, endline, endcolumn)
   }
 }
 

cpp/ql/src/Architecture/Refactoring Opportunities/ClassesWithManyFields.ql

@@ -46,40 +46,40 @@ predicate masterVde(VariableDeclarationEntry master, VariableDeclarationEntry vd
   exists(VariableDeclarationEntry previous | previousVde(previous, vde) and masterVde(master, previous))
 }
 
-class VariableDeclarationGroup extends @var_decl {
+class VariableDeclarationGroup extends ElementBase {
   VariableDeclarationGroup() {
-    not previousVde(_, mkElement(this))
+    this instanceof VariableDeclarationEntry and
+    not previousVde(_, this)
   }
   Class getClass() {
-    vdeInfo(mkElement(this), result, _, _)
+    vdeInfo(this, result, _, _)
   }
 
   // pragma[noopt] since otherwise the two locationInfo relations get join-ordered
   // after each other
   pragma[noopt]
   predicate hasLocationInfo(string path, int startline, int startcol, int endline, int endcol) {
-    exists(Element thisElement, VariableDeclarationEntry last, Location lstart, Location lend |
-      thisElement = mkElement(this) and
-      masterVde(thisElement, last) and
+    exists(VariableDeclarationEntry last, Location lstart, Location lend |
+      masterVde(this, last) and
       this instanceof VariableDeclarationGroup and
       not previousVde(last, _) and
-      exists(VariableDeclarationEntry vde | vde=mkElement(this) and vde instanceof VariableDeclarationEntry and vde.getLocation() = lstart) and
+      exists(VariableDeclarationEntry vde | vde=this and vde instanceof VariableDeclarationEntry and vde.getLocation() = lstart) and
       last.getLocation() = lend and
       lstart.hasLocationInfo(path, startline, startcol, _, _) and
       lend.hasLocationInfo(path, _, _, endline, endcol)
     )
   }
 
-  string toString() {
-    if previousVde(mkElement(this), _) then
+  string describeGroup() {
+    if previousVde(this, _) then
       result = "group of "
              + strictcount(string name
                          | exists(VariableDeclarationEntry vde
-                                | masterVde(mkElement(this), vde) and
+                                | masterVde(this, vde) and
                                   name = vde.getName()))
              + " fields here"
     else
-      result = "declaration of " + mkElement(this).(VariableDeclarationEntry).getVariable().getName()
+      result = "declaration of " + this.(VariableDeclarationEntry).getVariable().getName()
   }
 }
 
@@ -111,4 +111,4 @@ where n = strictcount(string fieldName
       c = vdg.getClass() and
       if c.hasOneVariableGroup() then suffix = "" else suffix = " - see $@"
 select c, kindstr(c) + " " + c.getName() + " has " + n + " fields, which is too many" + suffix + ".",
-       vdg, vdg.toString()
+       vdg, vdg.describeGroup()

cpp/ql/src/CPython/Extensions.qll

@@ -77,11 +77,6 @@ class PythonClass extends Variable, CObject {
          /* This needs to be kept in sync with extractor-python/semmle/passes/type.py */
          result = "C_type$" + this.getTpName()
     }
-
-    /** Gets a textual representation of this element. */
-    override string toString() {
-        result = Variable.super.toString()
-    }
 }
 
 /**
@@ -518,12 +513,6 @@ class PythonExtensionFunction extends Function {
 }
 
 class TypedPythonExtensionProperty extends PythonGetSetTableEntry, CObject {
-
-    override
-    string toString() {
-        result = PythonGetSetTableEntry.super.toString()
-    }
-
     PythonClass getPropertyType() {
         result = py_return_type(this.getGetter())
     }

cpp/ql/src/Likely Bugs/Format/SnprintfOverflow.ql

@@ -98,7 +98,7 @@ predicate flowsToDefImpl(
   or
   // `x++`
   exists (CrementOperation crem
-  | mkElement(def) = crem and
+  | def = crem and
     crem.getOperand() = v.getAnAccess() and
     flowsToExpr(source, crem.getOperand(), pathMightOverflow))
   or

cpp/ql/src/Security/CWE/CWE-764/LockFlow.qll

@@ -27,7 +27,7 @@ predicate tryLockCondition(VariableAccess access,
     (cond = call.getParent*() and
      cond.isCondition() and
      failNode = cond.getASuccessor() and
-     unresolveElement(failNode) instanceof BasicBlockWithReturn))
+     failNode instanceof BasicBlockWithReturn))
 }
 
 /**

cpp/ql/src/Security/CWE/CWE-764/UnreleasedLock.ql

@@ -29,7 +29,7 @@ predicate failedLock(MutexType t, BasicBlock lockblock, BasicBlock failblock) {
   exists (ControlFlowNode lock |
     lock = lockblock.getEnd() and
     lock = t.getLockAccess() and
-    lock.getAFalseSuccessor() = mkElement(failblock)
+    lock.getAFalseSuccessor() = failblock
   )
 }
 

cpp/ql/src/semmle/code/cpp/Declaration.qll

@@ -309,7 +309,7 @@ abstract class AccessHolder extends Declaration {
     isDirectPublicBaseOf*(base, derived)
     or
     exists(DirectAccessHolder n |
-      this.getEnclosingAccessHolder*() = mkElement(n) and
+      this.getEnclosingAccessHolder*() = n and
       // Derivations using (4.2) or (4.3) at least once.
       n.thisCanAccessClassTrans(base, derived)
     )
@@ -379,7 +379,7 @@ abstract class AccessHolder extends Declaration {
     everyoneCouldAccessMember(memberClass, memberAccess, derived)
     or
     exists(DirectAccessHolder n |
-      this.getEnclosingAccessHolder*() = mkElement(n) and
+      this.getEnclosingAccessHolder*() = n and
       // Any other derivation.
       n.thisCouldAccessMember(memberClass, memberAccess, derived)
     )
@@ -396,11 +396,11 @@ abstract class AccessHolder extends Declaration {
  * `DirectAccessHolder`s. If a `DirectAccessHolder` contains an `AccessHolder`,
  * then the contained `AccessHolder` inherits its access rights.
  */
-private class DirectAccessHolder extends @declaration {
+private class DirectAccessHolder extends Element {
   DirectAccessHolder() {
-    mkElement(this) instanceof Class
+    this instanceof Class
     or
-    exists(FriendDecl fd | fd.getFriend() = mkElement(this))
+    exists(FriendDecl fd | fd.getFriend() = this)
   }
 
   /**
@@ -486,7 +486,7 @@ private class DirectAccessHolder extends @declaration {
     )
     or
     // Rule (5.4) followed by Rule (5.2)
-    exists(Class between | mkElement(this).(AccessHolder).canAccessClass(between, derived) |
+    exists(Class between | this.(AccessHolder).canAccessClass(between, derived) |
         between.accessOfBaseMember(memberClass, memberAccess)
                .hasName("private") and
         this.isFriendOfOrEqualTo(between)
@@ -539,12 +539,10 @@ private class DirectAccessHolder extends @declaration {
   }
 
   private predicate isFriendOfOrEqualTo(Class c) {
-    exists(FriendDecl fd | fd.getDeclaringClass() = c | mkElement(this) = fd.getFriend())
+    exists(FriendDecl fd | fd.getDeclaringClass() = c | this = fd.getFriend())
     or
-    mkElement(this) = c
+    this = c
   }
-
-  string toString() { result = mkElement(this).(Declaration).toString() }
 }
 
 /**

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

@@ -46,17 +46,26 @@ Element mkElement(@element e) {
 }
 
 /**
- * A C/C++ element. This class is the base class for all C/C++
- * elements, such as functions, classes, expressions, and so on.
+ * A C/C++ element with no member predicates other than `toString`. Not for
+ * general use. This class does not define a location, so classes wanting to
+ * change their location without affecting other classes can extend
+ * `ElementBase` instead of `Element` to create a new rootdef for `getURL`,
+ * `getLocation`, or `hasLocationInfo`.
  */
-class Element extends @element {
-  Element() {
+class ElementBase extends @element {
+  ElementBase() {
     this = resolveElement(_)
   }
 
   /** Gets a textual representation of this element. */
   string toString() { none() }
+}
 
+/**
+ * A C/C++ element. This class is the base class for all C/C++
+ * elements, such as functions, classes, expressions, and so on.
+ */
+class Element extends ElementBase {
   /** Gets the primary file where this element occurs. */
   File getFile() { result = this.getLocation().getFile() }
 

cpp/ql/src/semmle/code/cpp/controlflow/BasicBlocks.qll

@@ -60,7 +60,7 @@ private cached module Cached {
   private predicate non_primitive_basic_block_entry_node(ControlFlowNode node) {
     not primitive_basic_block_entry_node(node) and
     not exists(node.getAPredecessor()) and
-    successors_extended(unresolveElement(node), _)
+    successors_extended(node, _)
   }
 
   /**
@@ -80,7 +80,7 @@ private cached module Cached {
    * reuse predicates already computed for `PrimitiveBasicBlocks`.
    */
   private predicate equalsPrimitiveBasicBlock(BasicBlock bb) {
-    primitive_basic_block_entry_node(mkElement(bb))
+    primitive_basic_block_entry_node(bb)
     and
     not exists(int i |
       i > 0 and
@@ -96,11 +96,11 @@ private cached module Cached {
   }
 
   private predicate non_primitive_basic_block_member(ControlFlowNode node, BasicBlock bb, int pos) {
-    (not equalsPrimitiveBasicBlock(bb) and node = mkElement(bb) and pos = 0)
+    (not equalsPrimitiveBasicBlock(bb) and node = bb and pos = 0)
     or
-    (not (unresolveElement(node) instanceof BasicBlock) and
+    (not (node instanceof BasicBlock) and
      exists (ControlFlowNode pred
-     | successors_extended(unresolveElement(pred),unresolveElement(node))
+     | successors_extended(pred, node)
      | non_primitive_basic_block_member(pred, bb, pos - 1)))
   }
 
@@ -117,7 +117,7 @@ private cached module Cached {
   predicate bb_successor_cached(BasicBlock pred, BasicBlock succ) {
     exists(ControlFlowNode last |
       basic_block_member(last, pred, bb_length(pred)-1) and
-      last.getASuccessor() = mkElement(succ)
+      last.getASuccessor() = succ
     )
   }
 }
@@ -143,15 +143,10 @@ predicate bb_successor = bb_successor_cached/2;
  *    A - B < C - D  AB is a basic block and CD is a basic block (B has two outgoing edges)
  * ```
  */
-class BasicBlock extends @cfgnode {
+class BasicBlock extends ControlFlowNodeBase {
 
   BasicBlock() {
-    basic_block_entry_node(mkElement(this))
-  }
-
-  /** Gets a textual representation of this element. */
-  string toString() {
-    result = "BasicBlock"
+    basic_block_entry_node(this)
   }
 
   predicate contains(ControlFlowNode node) {
@@ -187,7 +182,7 @@ class BasicBlock extends @cfgnode {
   }
 
   ControlFlowNode getStart() {
-    result = mkElement(this)
+    result = this
   }
 
   /** Gets the number of `ControlFlowNode`s in this basic block. */
@@ -248,9 +243,9 @@ class BasicBlock extends @cfgnode {
    * point or a `catch` clause of a reachable `try` statement.
    */
   predicate isReachable() {
-    exists(Function f | f.getBlock() = mkElement(this))
+    exists(Function f | f.getBlock() = this)
     or
-    exists(TryStmt t, BasicBlock tryblock | mkElement(this) = t.getACatchClause() and tryblock.isReachable() and tryblock.contains(t))
+    exists(TryStmt t, BasicBlock tryblock | this = t.getACatchClause() and tryblock.isReachable() and tryblock.contains(t))
     or
     exists(BasicBlock pred | pred.getASuccessor() = this and pred.isReachable())
   }
@@ -272,7 +267,7 @@ predicate unreachable(ControlFlowNode n) {
  */
 class EntryBasicBlock extends BasicBlock {
   EntryBasicBlock() {
-    exists (Function f | mkElement(this) = f.getEntryPoint())
+    exists (Function f | this = f.getEntryPoint())
   }
 }