Merge pull request #875 from hvitved/csharp/accessor-call-revert

Approved by calumgrant

Author: semmle-qlci

change-notes/1.20/analysis-csharp.md

@@ -25,6 +25,4 @@
 
 ## Changes to QL libraries
 
-* The class `AccessorCall` (and subclasses `PropertyCall`, `IndexerCall`, and `EventCall`) have been redefined, so the expressions they represent are not necessarily the accesses themselves, but rather the expressions that give rise to the accessor calls. For example, in the property assignment `x.Prop = 0`, the call to the setter for `Prop` is no longer represented by the access `x.Prop`, but instead the whole assignment. Consequently, it is no longer safe to cast directly between `AccessorCall`s and `Access`es, and the predicate `AccessorCall::getAccess()` should be used instead.
-
 ## Changes to the autobuilder

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

@@ -402,17 +402,14 @@ module AssignableInternal {
      * `a`, if any.
      */
     cached
-    Expr getAccessorCallValueArgument(Access a) {
-      a.getTarget() instanceof DeclarationWithAccessors and
-      (
-        exists(AssignExpr ae | tupleAssignmentDefinition(ae, a) |
-          tupleAssignmentPair(ae, a, result)
-        )
-        or
-        exists(Assignment ass | a = ass.getLValue() |
-          result = ass.getRValue() and
-          not ass.(AssignOperation).hasExpandedAssignment()
-        )
+    Expr getAccessorCallValueArgument(AccessorCall ac) {
+      exists(AssignExpr ae | tupleAssignmentDefinition(ae, ac) |
+        tupleAssignmentPair(ae, ac, result)
+      )
+      or
+      exists(Assignment ass | ac = ass.getLValue() |
+        result = ass.getRValue() and
+        not ass.(AssignOperation).hasExpandedAssignment()
       )
     }
   }

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

@@ -513,7 +513,7 @@ class IndexerProperty extends Property {
   IndexerCall getAnIndexerCall() {
     result = getType().(RefType).getAnIndexer().getAnAccessor().getACall() and
     // The qualifier of this indexer call should be a value returned from an access of this property
-    exists(Expr qualifier | qualifier = result.getAccess().getQualifier() |
+    exists(Expr qualifier | qualifier = result.(IndexerAccess).getQualifier() |
       DataFlow::localFlow(DataFlow::exprNode(this.getAnAccess()), DataFlow::exprNode(qualifier))
     )
   }

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

@@ -477,7 +477,7 @@ private module Internal {
 
     override Expr getArgument(int i) { result = getCall().getArgument(i) }
 
-    override Expr getQualifier() { result = getCall().getAccess().getQualifier() }
+    override Expr getQualifier() { result = getCall().(MemberAccess).getQualifier() }
 
     override Accessor getAStaticTarget() { result = getCall().getTarget() }
 

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

@@ -380,6 +380,17 @@ class MemberConstantAccess extends FieldAccess {
   override string toString() { result = "access to constant " + this.getTarget().getName() }
 }
 
+/**
+ * An internal helper class to share logic between `PropertyAccess` and
+ * `PropertyCall`.
+ */
+library class PropertyAccessExpr extends Expr, @property_access_expr {
+  /** Gets the target of this property access. */
+  Property getProperty() { expr_access(this, result) }
+
+  override string toString() { result = "access to property " + this.getProperty().getName() }
+}
+
 /**
  * An access to a property, for example the access to `P` on line 5 in
  *
@@ -393,13 +404,8 @@ class MemberConstantAccess extends FieldAccess {
  * }
  * ```
  */
-class PropertyAccess extends AssignableMemberAccess, @property_access_expr {
-  /** Gets the target of this property access. */
-  Property getProperty() { expr_access(this, result) }
-
+class PropertyAccess extends AssignableMemberAccess, PropertyAccessExpr {
   override Property getTarget() { result = this.getProperty() }
-
-  override string toString() { result = "access to property " + this.getProperty().getName() }
 }
 
 /**
@@ -509,6 +515,17 @@ class ElementRead extends ElementAccess, AssignableRead { }
  */
 class ElementWrite extends ElementAccess, AssignableWrite { }
 
+/**
+ * An internal helper class to share logic between `IndexerAccess` and
+ * `IndexerCall`.
+ */
+library class IndexerAccessExpr extends Expr, @indexer_access_expr {
+  /** Gets the target of this indexer access. */
+  Indexer getIndexer() { expr_access(this, result) }
+
+  override string toString() { result = "access to indexer" }
+}
+
 /**
  * An access to an indexer, for example the access to `c` on line 5 in
  *
@@ -522,17 +539,12 @@ class ElementWrite extends ElementAccess, AssignableWrite { }
  * }
  * ```
  */
-class IndexerAccess extends AssignableMemberAccess, ElementAccess, @indexer_access_expr {
-  /** Gets the target of this indexer access. */
-  Indexer getIndexer() { expr_access(this, result) }
-
+class IndexerAccess extends AssignableMemberAccess, ElementAccess, IndexerAccessExpr {
   override Indexer getTarget() { result = this.getIndexer() }
 
   override Indexer getQualifiedDeclaration() {
     result = ElementAccess.super.getQualifiedDeclaration()
   }
-
-  override string toString() { result = "access to indexer" }
 }
 
 /**
@@ -588,6 +600,17 @@ class VirtualIndexerAccess extends IndexerAccess {
   VirtualIndexerAccess() { targetIsOverridableOrImplementable() }
 }
 
+/**
+ * An internal helper class to share logic between `EventAccess` and
+ * `EventCall`.
+ */
+library class EventAccessExpr extends Expr, @event_access_expr {
+  /** Gets the target of this event access. */
+  Event getEvent() { expr_access(this, result) }
+
+  override string toString() { result = "access to event " + this.getEvent().getName() }
+}
+
 /**
  * An access to an event, for example the accesses to `Click` on lines
  * 7 and 8 in
@@ -605,13 +628,8 @@ class VirtualIndexerAccess extends IndexerAccess {
  * }
  * ```
  */
-class EventAccess extends AssignableMemberAccess, @event_access_expr {
-  /** Gets the target of this event access. */
-  Event getEvent() { expr_access(this, result) }
-
+class EventAccess extends AssignableMemberAccess, EventAccessExpr {
   override Event getTarget() { result = getEvent() }
-
-  override string toString() { result = "access to event " + this.getEvent().getName() }
 }
 
 /**