C#: Introduce class Overridable

csharp/ql/lib/semmle/code/csharp/Assignable.qll

@@ -205,7 +205,7 @@ private class RefArg extends AssignableAccess {
    */
   predicate isAnalyzable(Parameter p) {
     exists(Callable callable | callable = this.getUnboundDeclarationTarget(p) |
-      not callable.(Virtualizable).isOverridableOrImplementable() and
+      not callable.(Overridable).isOverridableOrImplementable() and
       callable.hasBody()
     )
   }

csharp/ql/lib/semmle/code/csharp/Implements.qll

@@ -4,7 +4,7 @@
  * Provides logic for determining interface member implementations.
  *
  * Do not use the predicates in this library directly; use the methods
- * of the class `Virtualizable` instead.
+ * of the class `Overridable` instead.
  */
 
 import csharp
@@ -35,7 +35,7 @@ private import Conversion
  * `implements(A.M, I.M, B)` and `implements(C.M, I.M, C)`.
  */
 cached
-predicate implements(Virtualizable m1, Virtualizable m2, ValueOrRefType t) {
+predicate implements(Overridable m1, Overridable m2, ValueOrRefType t) {
   exists(Interface i |
     i = m2.getDeclaringType() and
     t.getABaseInterface+() = i and
@@ -66,7 +66,7 @@ predicate implements(Virtualizable m1, Virtualizable m2, ValueOrRefType t) {
  * for type `C`, because `C.M()` conflicts.
  */
 pragma[nomagic]
-private Virtualizable getAnImplementedInterfaceMemberForSubType(Virtualizable m, ValueOrRefType t) {
+private Overridable getAnImplementedInterfaceMemberForSubType(Overridable m, ValueOrRefType t) {
   result = getACompatibleInterfaceMember(m) and
   t = m.getDeclaringType()
   or
@@ -78,7 +78,7 @@ private Virtualizable getAnImplementedInterfaceMemberForSubType(Virtualizable m,
 }
 
 pragma[noinline]
-private predicate hasMemberCompatibleWithInterfaceMember(ValueOrRefType t, Virtualizable m) {
+private predicate hasMemberCompatibleWithInterfaceMember(ValueOrRefType t, Overridable m) {
   m = getACompatibleInterfaceMember(t.getAMember())
 }
 
@@ -88,7 +88,7 @@ private predicate hasMemberCompatibleWithInterfaceMember(ValueOrRefType t, Virtu
  * the interface member is accessed.
  */
 pragma[nomagic]
-private Virtualizable getACompatibleInterfaceMember(Virtualizable m) {
+private Overridable getACompatibleInterfaceMember(Overridable m) {
   result = getACompatibleInterfaceMemberAux(m) and
   (
     // If there is both an implicit and an explicit compatible member
@@ -100,14 +100,14 @@ private Virtualizable getACompatibleInterfaceMember(Virtualizable m) {
 }
 
 pragma[nomagic]
-private Virtualizable getACompatibleExplicitInterfaceMember(Virtualizable m, ValueOrRefType declType) {
+private Overridable getACompatibleExplicitInterfaceMember(Overridable m, ValueOrRefType declType) {
   result = getACompatibleInterfaceMemberAux(m) and
   declType = m.getDeclaringType() and
   m.implementsExplicitInterface()
 }
 
 pragma[nomagic]
-private Virtualizable getACompatibleInterfaceMemberAux(Virtualizable m) {
+private Overridable getACompatibleInterfaceMemberAux(Overridable m) {
   result = getACompatibleInterfaceAccessor(m) or
   result = getACompatibleInterfaceIndexer(m) or
   result = getACompatibleInterfaceMethod(m)

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

@@ -180,29 +180,15 @@ class Member extends DotNet::Member, Modifiable, @member {
   override predicate isStatic() { Modifiable.super.isStatic() }
 }
 
+private class TOverridable = @virtualizable or @callable_accessor;
+
 /**
- * A member where the `virtual` modifier is valid. That is, a method,
- * a property, an indexer, or an event.
+ * A declaration that can be overridden or implemented. That is, a method,
+ * a property, an indexer, an event, or an accessor.
  *
- * Equivalently, these are the members that can be defined in an interface.
+ * Unlike `Virtualizable`, this class includes accessors.
  */
-class Virtualizable extends Member, @virtualizable {
-  /** Holds if this member has the modifier `override`. */
-  predicate isOverride() { this.hasModifier("override") }
-
-  /** Holds if this member is `virtual`. */
-  predicate isVirtual() { this.hasModifier("virtual") }
-
-  override predicate isPublic() {
-    Member.super.isPublic() or
-    this.implementsExplicitInterface()
-  }
-
-  override predicate isPrivate() {
-    super.isPrivate() and
-    not this.implementsExplicitInterface()
-  }
-
+class Overridable extends Declaration, TOverridable {
   /**
    * Gets any interface this member explicitly implements; this only applies
    * to members that can be declared on an interface, i.e. methods, properties,
@@ -216,19 +202,10 @@ class Virtualizable extends Member, @virtualizable {
   predicate implementsExplicitInterface() { exists(this.getExplicitlyImplementedInterface()) }
 
   /** Holds if this member can be overridden or implemented. */
-  predicate isOverridableOrImplementable() {
-    not this.isSealed() and
-    not this.getDeclaringType().isSealed() and
-    (
-      this.isVirtual() or
-      this.isOverride() or
-      this.isAbstract() or
-      this.getDeclaringType() instanceof Interface
-    )
-  }
+  predicate isOverridableOrImplementable() { none() }
 
   /** Gets the member that is immediately overridden by this member, if any. */
-  Virtualizable getOverridee() {
+  Overridable getOverridee() {
     overrides(this, result)
     or
     // For accessors (which are `Callable`s), the extractor generates entries
@@ -242,7 +219,7 @@ class Virtualizable extends Member, @virtualizable {
   }
 
   /** Gets a member that immediately overrides this member, if any. */
-  Virtualizable getAnOverrider() { this = result.getOverridee() }
+  Overridable getAnOverrider() { this = result.getOverridee() }
 
   /** Holds if this member is overridden by some other member. */
   predicate isOverridden() { exists(this.getAnOverrider()) }
@@ -273,10 +250,10 @@ class Virtualizable extends Member, @virtualizable {
    * `A.M.getImplementee(B) = I.M` and
    * `C.M.getImplementee(C) = I.M`.
    */
-  Virtualizable getImplementee(ValueOrRefType t) { implements(this, result, t) }
+  Overridable getImplementee(ValueOrRefType t) { implements(this, result, t) }
 
   /** Gets the interface member that is immediately implemented by this member, if any. */
-  Virtualizable getImplementee() { result = this.getImplementee(_) }
+  Overridable getImplementee() { result = this.getImplementee(_) }
 
   /**
    * Gets a member that immediately implements this interface member, if any.
@@ -301,10 +278,10 @@ class Virtualizable extends Member, @virtualizable {
    * `I.M.getAnImplementor(B) = A.M` and
    * `I.M.getAnImplementor(C) = C.M`.
    */
-  Virtualizable getAnImplementor(ValueOrRefType t) { this = result.getImplementee(t) }
+  Overridable getAnImplementor(ValueOrRefType t) { this = result.getImplementee(t) }
 
   /** Gets a member that immediately implements this interface member, if any. */
-  Virtualizable getAnImplementor() { this = result.getImplementee() }
+  Overridable getAnImplementor() { this = result.getImplementee() }
 
   /**
    * Gets an interface member that is (transitively) implemented by this
@@ -334,8 +311,8 @@ class Virtualizable extends Member, @virtualizable {
    * - If this member is `D.M` then `I.M = getAnUltimateImplementee()`.
    */
   pragma[nomagic]
-  Virtualizable getAnUltimateImplementee() {
-    exists(Virtualizable implementation, ValueOrRefType implementationType |
+  Overridable getAnUltimateImplementee() {
+    exists(Overridable implementation, ValueOrRefType implementationType |
       implements(implementation, result, implementationType)
     |
       this = implementation
@@ -354,7 +331,7 @@ class Virtualizable extends Member, @virtualizable {
    * Note that this is generally *not* equivalent with
    * `getImplementor().getAnOverrider*()` (see `getImplementee`).
    */
-  Virtualizable getAnUltimateImplementor() { this = result.getAnUltimateImplementee() }
+  Overridable getAnUltimateImplementor() { this = result.getAnUltimateImplementee() }
 
   /** Holds if this interface member is implemented by some other member. */
   predicate isImplemented() { exists(this.getAnImplementor()) }
@@ -366,7 +343,7 @@ class Virtualizable extends Member, @virtualizable {
    * Holds if this member overrides or implements (transitively)
    * `that` member.
    */
-  predicate overridesOrImplements(Virtualizable that) {
+  predicate overridesOrImplements(Overridable that) {
     this.getOverridee+() = that or
     this.getAnUltimateImplementee() = that
   }
@@ -375,12 +352,49 @@ class Virtualizable extends Member, @virtualizable {
    * Holds if this member overrides or implements (reflexively, transitively)
    * `that` member.
    */
-  predicate overridesOrImplementsOrEquals(Virtualizable that) {
+  predicate overridesOrImplementsOrEquals(Overridable that) {
     this = that or
     this.overridesOrImplements(that)
   }
 }
 
+/**
+ * A member where the `virtual` modifier is valid. That is, a method,
+ * a property, an indexer, or an event.
+ *
+ * Equivalently, these are the members that can be defined in an interface.
+ *
+ * Unlike `Overridable`, this class excludes accessors.
+ */
+class Virtualizable extends Overridable, Member, @virtualizable {
+  /** Holds if this member has the modifier `override`. */
+  predicate isOverride() { this.hasModifier("override") }
+
+  /** Holds if this member is `virtual`. */
+  predicate isVirtual() { this.hasModifier("virtual") }
+
+  override predicate isPublic() {
+    Member.super.isPublic() or
+    this.implementsExplicitInterface()
+  }
+
+  override predicate isPrivate() {
+    super.isPrivate() and
+    not this.implementsExplicitInterface()
+  }
+
+  override predicate isOverridableOrImplementable() {
+    not this.isSealed() and
+    not this.getDeclaringType().isSealed() and
+    (
+      this.isVirtual() or
+      this.isOverride() or
+      this.isAbstract() or
+      this.getDeclaringType() instanceof Interface
+    )
+  }
+}
+
 /**
  * A parameterizable declaration. Either a callable (`Callable`), a delegate
  * type (`DelegateType`), or an indexer (`Indexer`).

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

@@ -315,7 +315,7 @@ class Indexer extends DeclarationWithGetSetAccessors, Parameterizable, @indexer
  * An accessor. Either a getter (`Getter`), a setter (`Setter`), or event
  * accessor (`EventAccessor`).
  */
-class Accessor extends Callable, Modifiable, Attributable, @callable_accessor {
+class Accessor extends Callable, Modifiable, Attributable, Overridable, @callable_accessor {
   override ValueOrRefType getDeclaringType() { result = this.getDeclaration().getDeclaringType() }
 
   /** Gets the assembly name of this accessor. */
@@ -376,6 +376,10 @@ class Accessor extends Callable, Modifiable, Attributable, @callable_accessor {
     not (result instanceof AccessModifier and exists(this.getAnAccessModifier()))
   }
 
+  override predicate isOverridableOrImplementable() {
+    this.getDeclaration().isOverridableOrImplementable()
+  }
+
   override Accessor getUnboundDeclaration() { accessors(this, _, _, _, result) }
 
   override Location getALocation() { accessor_location(this, result) }

csharp/ql/lib/semmle/code/csharp/controlflow/Guards.qll

@@ -1086,7 +1086,7 @@ module Internal {
      */
     private Callable customNullCheck(Parameter p, BooleanValue retVal, boolean isNull) {
       result.getReturnType() instanceof BoolType and
-      not result.(Virtualizable).isOverridableOrImplementable() and
+      not result.(Overridable).isOverridableOrImplementable() and
       p.getCallable() = result and
       not p.isParams() and
       p.getType() = any(Type t | t instanceof RefType or t instanceof NullableType) and

csharp/ql/lib/semmle/code/csharp/dataflow/ExternalFlow.qll

@@ -360,11 +360,7 @@ private class UnboundCallable extends Callable {
 
   predicate overridesOrImplementsUnbound(UnboundCallable that) {
     exists(Callable c |
-      this.(Virtualizable).overridesOrImplementsOrEquals(c) or
-      this = c.(OverridableCallable).getAnUltimateImplementor() or
-      this = c.(OverridableCallable).getAnOverrider+()
-    |
-      this != c and
+      this.(OverridableCallable).overridesOrImplements(c) and
       that = c.getUnboundDeclaration()
     )
   }