Merge pull request #21412 from aschackmull/java/binary-assignment

Java: Make Assignment extend BinaryExpr.

java/downgrades/de4ded61c8ae83f829aedaf05be73307ba25ca40/upgrade.properties

@@ -0,0 +1,2 @@
+description: Remove inclusion of @assignment in @binaryexpr
+compatibility: full

java/ql/consistency-queries/BinaryExpr.ql

@@ -10,5 +10,5 @@ where
     e.isNthChildOf(be, i) and i != 0 and i != 1 and reason = "Unexpected operand " + i.toString()
   )
   or
-  be.getOp() = " ?? " and reason = "No operator name"
+  be.getOp() = "??" and reason = "No operator name"
 select be, reason

java/ql/lib/change-notes/2026-03-04-binary-assignment.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The class `Assignment` now extends `BinaryExpr`. Uses of `BinaryExpr` may in some cases need slight adjustment.

java/ql/lib/config/semmlecode.dbscheme

@@ -815,6 +815,7 @@ when_branch_else(unique int id: @whenbranch ref);
              | @geexpr
              | @eqexpr
              | @neexpr
+             | @assignment
              | @valueeqexpr
              | @valueneexpr;
 

java/ql/lib/semmle/code/java/Expr.qll

@@ -392,7 +392,7 @@ class ArrayInit extends Expr, @arrayinit {
  * element assignments since there the assignment destination is not directly
  * the array variable but instead an `ArrayAccess`.
  */
-class Assignment extends Expr, @assignment {
+class Assignment extends BinaryExpr, @assignment {
   /** Gets the destination (left-hand side) of the assignment. */
   Expr getDest() { result.isNthChildOf(this, 0) }
 
@@ -417,6 +417,8 @@ class Assignment extends Expr, @assignment {
  * For example, `x = 23`.
  */
 class AssignExpr extends Assignment, @assignexpr {
+  override string getOp() { result = "=" }
+
   override string getAPrimaryQlClass() { result = "AssignExpr" }
 }
 
@@ -445,7 +447,7 @@ class AssignOp extends Assignment, @assignop {
   override Expr getSource() { result.getParent() = this }
 
   /** Gets a string representation of the assignment operator of this compound assignment. */
-  /*abstract*/ string getOp() { result = "??=" }
+  /*abstract*/ override string getOp() { result = "??=" }
 
   /** Gets a printable representation of this expression. */
   override string toString() { result = "..." + this.getOp() + "..." }
@@ -739,155 +741,155 @@ class BinaryExpr extends Expr, @binaryexpr {
   }
 
   /** Gets a printable representation of this expression. */
-  override string toString() { result = "..." + this.getOp() + "..." }
+  override string toString() { result = "... " + this.getOp() + " ..." }
 
   /** Gets a string representation of the operator of this binary expression. */
-  /*abstract*/ string getOp() { result = " ?? " }
+  /*abstract*/ string getOp() { result = "??" }
 }
 
 /** A binary expression using the `*` operator. */
 class MulExpr extends BinaryExpr, @mulexpr {
-  override string getOp() { result = " * " }
+  override string getOp() { result = "*" }
 
   override string getAPrimaryQlClass() { result = "MulExpr" }
 }
 
 /** A binary expression using the `/` operator. */
 class DivExpr extends BinaryExpr, @divexpr {
-  override string getOp() { result = " / " }
+  override string getOp() { result = "/" }
 
   override string getAPrimaryQlClass() { result = "DivExpr" }
 }
 
 /** A binary expression using the `%` operator. */
 class RemExpr extends BinaryExpr, @remexpr {
-  override string getOp() { result = " % " }
+  override string getOp() { result = "%" }
 
   override string getAPrimaryQlClass() { result = "RemExpr" }
 }
 
 /** A binary expression using the `+` operator. */
 class AddExpr extends BinaryExpr, @addexpr {
-  override string getOp() { result = " + " }
+  override string getOp() { result = "+" }
 
   override string getAPrimaryQlClass() { result = "AddExpr" }
 }
 
 /** A binary expression using the `-` operator. */
 class SubExpr extends BinaryExpr, @subexpr {
-  override string getOp() { result = " - " }
+  override string getOp() { result = "-" }
 
   override string getAPrimaryQlClass() { result = "SubExpr" }
 }
 
 /** A binary expression using the `<<` operator. */
 class LeftShiftExpr extends BinaryExpr, @lshiftexpr {
-  override string getOp() { result = " << " }
+  override string getOp() { result = "<<" }
 
   override string getAPrimaryQlClass() { result = "LeftShiftExpr" }
 }
 
 /** A binary expression using the `>>` operator. */
 class RightShiftExpr extends BinaryExpr, @rshiftexpr {
-  override string getOp() { result = " >> " }
+  override string getOp() { result = ">>" }
 
   override string getAPrimaryQlClass() { result = "RightShiftExpr" }
 }
 
 /** A binary expression using the `>>>` operator. */
 class UnsignedRightShiftExpr extends BinaryExpr, @urshiftexpr {
-  override string getOp() { result = " >>> " }
+  override string getOp() { result = ">>>" }
 
   override string getAPrimaryQlClass() { result = "UnsignedRightShiftExpr" }
 }
 
 /** A binary expression using the `&` operator. */
 class AndBitwiseExpr extends BinaryExpr, @andbitexpr {
-  override string getOp() { result = " & " }
+  override string getOp() { result = "&" }
 
   override string getAPrimaryQlClass() { result = "AndBitwiseExpr" }
 }
 
 /** A binary expression using the `|` operator. */
 class OrBitwiseExpr extends BinaryExpr, @orbitexpr {
-  override string getOp() { result = " | " }
+  override string getOp() { result = "|" }
 
   override string getAPrimaryQlClass() { result = "OrBitwiseExpr" }
 }
 
 /** A binary expression using the `^` operator. */
 class XorBitwiseExpr extends BinaryExpr, @xorbitexpr {
-  override string getOp() { result = " ^ " }
+  override string getOp() { result = "^" }
 
   override string getAPrimaryQlClass() { result = "XorBitwiseExpr" }
 }
 
 /** A binary expression using the `&&` operator. */
 class AndLogicalExpr extends BinaryExpr, @andlogicalexpr {
-  override string getOp() { result = " && " }
+  override string getOp() { result = "&&" }
 
   override string getAPrimaryQlClass() { result = "AndLogicalExpr" }
 }
 
 /** A binary expression using the `||` operator. */
 class OrLogicalExpr extends BinaryExpr, @orlogicalexpr {
-  override string getOp() { result = " || " }
+  override string getOp() { result = "||" }
 
   override string getAPrimaryQlClass() { result = "OrLogicalExpr" }
 }
 
 /** A binary expression using the `<` operator. */
 class LTExpr extends BinaryExpr, @ltexpr {
-  override string getOp() { result = " < " }
+  override string getOp() { result = "<" }
 
   override string getAPrimaryQlClass() { result = "LTExpr" }
 }
 
 /** A binary expression using the `>` operator. */
 class GTExpr extends BinaryExpr, @gtexpr {
-  override string getOp() { result = " > " }
+  override string getOp() { result = ">" }
 
   override string getAPrimaryQlClass() { result = "GTExpr" }
 }
 
 /** A binary expression using the `<=` operator. */
 class LEExpr extends BinaryExpr, @leexpr {
-  override string getOp() { result = " <= " }
+  override string getOp() { result = "<=" }
 
   override string getAPrimaryQlClass() { result = "LEExpr" }
 }
 
 /** A binary expression using the `>=` operator. */
 class GEExpr extends BinaryExpr, @geexpr {
-  override string getOp() { result = " >= " }
+  override string getOp() { result = ">=" }
 
   override string getAPrimaryQlClass() { result = "GEExpr" }
 }
 
 /** A binary expression using Java's `==` or Kotlin's `===` operator. */
 class EQExpr extends BinaryExpr, @eqexpr {
-  override string getOp() { result = " == " }
+  override string getOp() { result = "==" }
 
   override string getAPrimaryQlClass() { result = "EQExpr" }
 }
 
 /** A binary expression using the Kotlin `==` operator, semantically equivalent to `Objects.equals`. */
 class ValueEQExpr extends BinaryExpr, @valueeqexpr {
-  override string getOp() { result = " (value equals) " }
+  override string getOp() { result = "(value equals)" }
 
   override string getAPrimaryQlClass() { result = "ValueEQExpr" }
 }
 
 /** A binary expression using Java's `!=` or Kotlin's `!==` operator. */
 class NEExpr extends BinaryExpr, @neexpr {
-  override string getOp() { result = " != " }
+  override string getOp() { result = "!=" }
 
   override string getAPrimaryQlClass() { result = "NEExpr" }
 }
 
 /** A binary expression using the Kotlin `!=` operator, semantically equivalent to `Objects.equals`. */
 class ValueNEExpr extends BinaryExpr, @valueneexpr {
-  override string getOp() { result = " (value not-equals) " }
+  override string getOp() { result = "(value not-equals)" }
 
   override string getAPrimaryQlClass() { result = "ValueNEExpr" }
 }

java/ql/lib/semmle/code/java/PrettyPrintAst.qll

@@ -207,29 +207,12 @@ private class PpArrayInit extends PpAst, ArrayInit {
   override PpAst getChild(int i) { exists(int j | result = this.getInit(j) and i = 1 + 2 * j) }
 }
 
-private class PpAssignment extends PpAst, Assignment {
-  override string getPart(int i) {
-    i = 1 and
-    this instanceof AssignExpr and
-    result = " = "
-    or
-    i = 1 and
-    result = " " + this.(AssignOp).getOp() + " "
-  }
-
-  override PpAst getChild(int i) {
-    i = 0 and result = this.getDest()
-    or
-    i = 2 and result = this.getRhs()
-  }
-}
-
 private class PpLiteral extends PpAst, Literal {
   override string getPart(int i) { i = 0 and result = this.getLiteral() }
 }
 
 private class PpBinaryExpr extends PpAst, BinaryExpr {
-  override string getPart(int i) { i = 1 and result = this.getOp() }
+  override string getPart(int i) { i = 1 and result = " " + this.getOp() + " " }
 
   override PpAst getChild(int i) {
     i = 0 and result = this.getLeftOperand()

java/ql/lib/semmle/code/java/arithmetic/Overflow.qll

@@ -93,8 +93,7 @@ class ArithExpr extends Expr {
     ) and
     forall(Expr e |
       e = this.(BinaryExpr).getAnOperand() or
-      e = this.(UnaryAssignExpr).getOperand() or
-      e = this.(AssignOp).getSource()
+      e = this.(UnaryAssignExpr).getOperand()
     |
       e.getType() instanceof NumType
     )
@@ -114,21 +113,17 @@ class ArithExpr extends Expr {
    */
   Expr getLeftOperand() {
     result = this.(BinaryExpr).getLeftOperand() or
-    result = this.(UnaryAssignExpr).getOperand() or
-    result = this.(AssignOp).getDest()
+    result = this.(UnaryAssignExpr).getOperand()
   }
 
   /**
    * Gets the right-hand operand if this is a binary expression.
    */
-  Expr getRightOperand() {
-    result = this.(BinaryExpr).getRightOperand() or result = this.(AssignOp).getRhs()
-  }
+  Expr getRightOperand() { result = this.(BinaryExpr).getRightOperand() }
 
   /** Gets an operand of this arithmetic expression. */
   Expr getAnOperand() {
     result = this.(BinaryExpr).getAnOperand() or
-    result = this.(UnaryAssignExpr).getOperand() or
-    result = this.(AssignOp).getSource()
+    result = this.(UnaryAssignExpr).getOperand()
   }
 }

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

@@ -179,13 +179,7 @@ private module GuardsInput implements SharedGuards::InputSig<Location, ControlFl
     }
   }
 
-  abstract private class BinExpr extends Expr {
-    Expr getAnOperand() {
-      result = this.(BinaryExpr).getAnOperand() or result = this.(AssignOp).getSource()
-    }
-  }
-
-  class AndExpr extends BinExpr {
+  class AndExpr extends BinaryExpr {
     AndExpr() {
       this instanceof AndBitwiseExpr or
       this instanceof AndLogicalExpr or
@@ -193,7 +187,7 @@ private module GuardsInput implements SharedGuards::InputSig<Location, ControlFl
     }
   }
 
-  class OrExpr extends BinExpr {
+  class OrExpr extends BinaryExpr {
     OrExpr() {
       this instanceof OrBitwiseExpr or
       this instanceof OrLogicalExpr or

java/ql/lib/semmle/code/java/dataflow/Nullness.qll

@@ -53,8 +53,6 @@ private predicate unboxed(Expr e) {
       assign.getDest().getType() instanceof PrimitiveType and assign.getSource() = e
     )
     or
-    exists(AssignOp assign | assign.getSource() = e and assign.getType() instanceof PrimitiveType)
-    or
     exists(EqualityTest eq |
       eq.getAnOperand() = e and eq.getAnOperand().getType() instanceof PrimitiveType
     )
@@ -62,6 +60,7 @@ private predicate unboxed(Expr e) {
     exists(BinaryExpr bin |
       bin.getAnOperand() = e and
       not bin instanceof EqualityTest and
+      not bin instanceof AssignExpr and
       bin.getType() instanceof PrimitiveType
     )
     or

java/ql/lib/semmle/code/java/dataflow/RangeAnalysis.qll

@@ -86,23 +86,7 @@ module Sem implements Semantic<Location> {
 
   class ConstantIntegerExpr = RU::ConstantIntegerExpr;
 
-  abstract class BinaryExpr extends Expr {
-    Expr getLeftOperand() {
-      result = this.(J::BinaryExpr).getLeftOperand() or result = this.(J::AssignOp).getDest()
-    }
-
-    Expr getRightOperand() {
-      result = this.(J::BinaryExpr).getRightOperand() or result = this.(J::AssignOp).getRhs()
-    }
-
-    final Expr getAnOperand() { result = this.getLeftOperand() or result = this.getRightOperand() }
-
-    final predicate hasOperands(Expr e1, Expr e2) {
-      this.getLeftOperand() = e1 and this.getRightOperand() = e2
-      or
-      this.getLeftOperand() = e2 and this.getRightOperand() = e1
-    }
-  }
+  class BinaryExpr = J::BinaryExpr;
 
   class AddExpr extends BinaryExpr {
     AddExpr() { this instanceof J::AddExpr or this instanceof J::AssignAddExpr }

java/ql/lib/semmle/code/java/dataflow/internal/rangeanalysis/SignAnalysisSpecific.qll

@@ -161,13 +161,9 @@ module Private {
       this instanceof J::AssignUnsignedRightShiftExpr and result = TUnsignedRightShiftOp()
     }
 
-    Expr getLeftOperand() {
-      result = this.(J::BinaryExpr).getLeftOperand() or result = this.(J::AssignOp).getDest()
-    }
+    Expr getLeftOperand() { result = this.(J::BinaryExpr).getLeftOperand() }
 
-    Expr getRightOperand() {
-      result = this.(J::BinaryExpr).getRightOperand() or result = this.(J::AssignOp).getRhs()
-    }
+    Expr getRightOperand() { result = this.(J::BinaryExpr).getRightOperand() }
   }
 
   predicate ssaRead = RU::ssaRead/2;

java/ql/lib/semmle/code/java/security/InsecureRandomnessQuery.qll

@@ -73,7 +73,8 @@ module InsecureRandomnessConfig implements DataFlow::ConfigSig {
   predicate isBarrierOut(DataFlow::Node n) { isSink(n) }
 
   predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) {
-    n1.asExpr() = n2.asExpr().(BinaryExpr).getAnOperand()
+    n1.asExpr() = n2.asExpr().(BinaryExpr).getAnOperand() and
+    not n2.asExpr() instanceof AssignExpr
     or
     n1.asExpr() = n2.asExpr().(UnaryExpr).getOperand()
     or

java/ql/lib/semmle/code/java/security/NumericCastTaintedQuery.qll

@@ -31,10 +31,7 @@ class RightShiftOp extends Expr {
     this instanceof AssignUnsignedRightShiftExpr
   }
 
-  private Expr getLhs() {
-    this.(BinaryExpr).getLeftOperand() = result or
-    this.(Assignment).getDest() = result
-  }
+  private Expr getLhs() { this.(BinaryExpr).getLeftOperand() = result }
 
   /**
    * Gets the variable that is shifted.

java/ql/lib/semmle/code/java/security/RandomQuery.qll

@@ -54,9 +54,8 @@ private module PredictableSeedFlowConfig implements DataFlow::ConfigSig {
 private module PredictableSeedFlow = DataFlow::Global<PredictableSeedFlowConfig>;
 
 private predicate predictableCalcStep(Expr e1, Expr e2) {
-  e2.(BinaryExpr).hasOperands(e1, any(PredictableSeedExpr p))
-  or
-  exists(AssignOp a | a = e2 | e1 = a.getDest() and a.getRhs() instanceof PredictableSeedExpr)
+  e2.(BinaryExpr).hasOperands(e1, any(PredictableSeedExpr p)) and
+  not e2 instanceof AssignExpr
   or
   exists(ConstructorCall cc, TypeNumber t | cc = e2 |
     cc.getArgument(0) = e1 and

java/ql/lib/upgrades/9f6026c400996c13842974b24f076a486ad1f69c/upgrade.properties

@@ -0,0 +1,2 @@
+description: Expand @binaryexpr union to include @assignment
+compatibility: full

java/ql/src/Likely Bugs/Arithmetic/OctalLiteral.ql

@@ -19,6 +19,7 @@ where
   lit.getLiteral() = val and
   val.regexpMatch("0[0-7][0-7]+") and
   lit.getParent() instanceof BinaryExpr and
+  not lit.getParent() instanceof Assignment and
   not lit.getParent() instanceof BitwiseExpr and
   not lit.getParent() instanceof ComparisonExpr
 select lit, "Integer literal starts with 0."

java/ql/src/Likely Bugs/Arithmetic/WhitespaceContradictsPrecedence.ql

@@ -138,13 +138,14 @@ int operatorWS(BinaryExpr expr) {
     endOfBinaryLhs(expr, line, lcol) and
     startOfBinaryRhs(expr, line, rcol) and
     parens = getParensNextToOp(expr) and
-    result = rcol - lcol + 1 - expr.getOp().length() - parens
+    result = rcol - lcol - 1 - expr.getOp().length() - parens
   )
 }
 
 /** Find nested binary expressions where the programmer may have made a precedence mistake. */
 predicate interestingNesting(BinaryExpr inner, BinaryExpr outer) {
   inner = outer.getAChildExpr() and
+  not outer instanceof Assignment and
   not inner instanceof AssocNestedExpr and
   not inner instanceof HarmlessNestedExpr and
   not inner.isParenthesized()

java/ql/src/Likely Bugs/Comparison/CompareIdenticalValues.ql

@@ -58,6 +58,7 @@ predicate equal(Expr left, Expr right) {
     sameVariable(left, right, _)
     or
     exists(BinaryExpr bLeft, BinaryExpr bRight | bLeft = left and bRight = right |
+      not bLeft instanceof Assignment and
       bLeft.getKind() = bRight.getKind() and
       equal(bLeft.getLeftOperand(), bRight.getLeftOperand()) and
       equal(bLeft.getRightOperand(), bRight.getRightOperand())

java/ql/src/Likely Bugs/Comparison/UselessComparisonTest.ql

@@ -101,17 +101,10 @@ Expr overFlowCand() {
   |
     bin instanceof AddExpr or
     bin instanceof MulExpr or
-    bin instanceof LeftShiftExpr
-  )
-  or
-  exists(AssignOp op |
-    result = op and
-    positive(op.getDest()) and
-    positive(op.getRhs())
-  |
-    op instanceof AssignAddExpr or
-    op instanceof AssignMulExpr or
-    op instanceof AssignLeftShiftExpr
+    bin instanceof LeftShiftExpr or
+    bin instanceof AssignAddExpr or
+    bin instanceof AssignMulExpr or
+    bin instanceof AssignLeftShiftExpr
   )
   or
   exists(AddExpr add, CompileTimeConstantExpr c |

java/ql/src/Violations of Best Practice/legacy/AutoBoxing.ql

@@ -36,6 +36,7 @@ Variable flowTarget(Expr arg) {
  */
 predicate unboxed(BoxedExpr e) {
   exists(BinaryExpr bin | e = bin.getAnOperand() |
+    not bin instanceof Assignment and
     if bin instanceof EqualityTest or bin instanceof ComparisonExpr
     then bin.getAnOperand() instanceof PrimitiveExpr
     else bin instanceof PrimitiveExpr

java/ql/test-kotlin1/library-tests/exprs/binop.ql

@@ -41,4 +41,5 @@ MaybeElement rhs(BinaryExpr e) {
 }
 
 from Expr e
+where not e instanceof Assignment
 select e, lhs(e), rhs(e)

java/ql/test-kotlin2/library-tests/exprs/binop.ql

@@ -41,4 +41,5 @@ MaybeElement rhs(BinaryExpr e) {
 }
 
 from Expr e
+where not e instanceof Assignment
 select e, lhs(e), rhs(e)

java/ql/test/library-tests/guards/GuardsInline.ql

@@ -11,7 +11,9 @@ string ppGuard(Guard g, Boolean branch) {
   or
   exists(BinaryExpr bin |
     bin = g and
-    result = "'" + bin.getLeftOperand() + bin.getOp() + bin.getRightOperand() + ":" + branch + "'"
+    result =
+      "'" + bin.getLeftOperand() + " " + bin.getOp() + " " + bin.getRightOperand() + ":" + branch +
+        "'"
   )
   or
   exists(SwitchCase cc, Expr s, string match, string value |

java/ql/test/query-tests/StringComparison/StringComparison.expected

@@ -1,3 +1,3 @@
-| StringComparison.java:23:6:23:19 | ... == ... | String values compared with  == . |
-| StringComparison.java:26:6:26:16 | ... == ... | String values compared with  == . |
-| StringComparison.java:29:6:29:20 | ... == ... | String values compared with  == . |
+| StringComparison.java:23:6:23:19 | ... == ... | String values compared with ==. |
+| StringComparison.java:26:6:26:16 | ... == ... | String values compared with ==. |
+| StringComparison.java:29:6:29:20 | ... == ... | String values compared with ==. |