WhitespaceContradictsPrecedence.ql

/**
 * @name Whitespace contradicts operator precedence
 * @description Nested expressions where the formatting contradicts the grouping enforced by operator precedence
 *              are difficult to read and may even indicate a bug.
 * @kind problem
 * @problem.severity warning
 * @precision very-high
 * @id java/whitespace-contradicts-precedence
 * @tags maintainability
 *       readability
 *       external/cwe/cwe-783
 */

import java

/**
 * A binary expression using the operator
 * `+`, `-`, `*`, `/`, or `%`.
 */
class ArithmeticExpr extends BinaryExpr {
  ArithmeticExpr() {
    this instanceof AddExpr or
    this instanceof SubExpr or
    this instanceof MulExpr or
    this instanceof DivExpr or
    this instanceof RemExpr
  }
}

/**
 * A binary expression using the operator
 * `<<`, `>>`, or `>>>`.
 */
class ShiftExpr extends BinaryExpr {
  ShiftExpr() {
    this instanceof LShiftExpr or
    this instanceof RShiftExpr or
    this instanceof URShiftExpr
  }
}

/**
 * A binary expression using the operator
 * `==`, `!=`, `<`, `>`, `<=`, or `>=`.
 */
class RelationExpr extends BinaryExpr {
  RelationExpr() {
    this instanceof EqualityTest or
    this instanceof ComparisonExpr
  }
}

/**
 * A binary expression using the operator
 * `&&` or `||`.
 */
class LogicalExpr extends BinaryExpr {
  LogicalExpr() {
    this instanceof AndLogicalExpr or
    this instanceof OrLogicalExpr
  }
}

/**
 * A binary expression of the form `x op y`, which is itself an operand (say, the left) of
 * another binary expression `(x op y) op' y` such that `(x op y) op' y' = x op (y op' y)`,
 * disregarding overflow.
 */
class AssocNestedExpr extends BinaryExpr {
  AssocNestedExpr() {
    exists(BinaryExpr parent, int idx | this.isNthChildOf(parent, idx) |
      // `+`, `*`, `&&`, `||` and the bitwise operations are associative.
      (
        this instanceof AddExpr or
        this instanceof MulExpr or
        this instanceof BitwiseExpr or
        this instanceof LogicalExpr
      ) and
      parent.getKind() = this.getKind()
      or
      // Equality tests are associate over each other.
      this instanceof EqualityTest and parent instanceof EqualityTest
      or
      // (x*y)/z = x*(y/z)
      this instanceof MulExpr and parent instanceof DivExpr and idx = 0
      or
      // (x/y)%z = x/(y%z)
      this instanceof DivExpr and parent instanceof RemExpr and idx = 0
      or
      // (x+y)-z = x+(y-z)
      this instanceof AddExpr and parent instanceof SubExpr and idx = 0
    )
  }
}

/**
 * A binary expression nested inside another binary expression where the inner operator "obviously"
 * binds tighter than the outer one. This is obviously subjective.
 */
class HarmlessNestedExpr extends BinaryExpr {
  HarmlessNestedExpr() {
    exists(BinaryExpr parent | this = parent.getAChildExpr() |
      parent instanceof RelationExpr and
      (this instanceof ArithmeticExpr or this instanceof ShiftExpr)
      or
      parent instanceof LogicalExpr and this instanceof RelationExpr
    )
  }
}

predicate startOfBinaryRhs(BinaryExpr expr, int line, int col) {
  exists(Location rloc | rloc = expr.getRightOperand().getLocation() |
    rloc.getStartLine() = line and rloc.getStartColumn() = col
  )
}

predicate endOfBinaryLhs(BinaryExpr expr, int line, int col) {
  exists(Location lloc | lloc = expr.getLeftOperand().getLocation() |
    lloc.getEndLine() = line and lloc.getEndColumn() = col
  )
}

/** Compute the number of parenthesis characters next to the operator. */
int getParensNextToOp(BinaryExpr expr) {
  exists(Expr left, Expr right, int pleft, int pright |
    left = expr.getLeftOperand() and
    right = expr.getRightOperand() and
    (if left.isParenthesized() then isParenthesized(left, pleft) else pleft = 0) and
    (if right.isParenthesized() then isParenthesized(right, pright) else pright = 0) and
    result = pleft + pright
  )
}

/** Compute whitespace around the operator. */
int operatorWS(BinaryExpr expr) {
  exists(int line, int lcol, int rcol, int parens |
    endOfBinaryLhs(expr, line, lcol) and
    startOfBinaryRhs(expr, line, rcol) and
    parens = getParensNextToOp(expr) and
    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 inner instanceof AssocNestedExpr and
  not inner instanceof HarmlessNestedExpr and
  not inner.isParenthesized()
}

from BinaryExpr inner, BinaryExpr outer, int wsouter, int wsinner
where
  interestingNesting(inner, outer) and
  wsinner = operatorWS(inner) and
  wsouter = operatorWS(outer) and
  wsinner % 2 = 0 and
  wsouter % 2 = 0 and
  wsinner > wsouter
select outer, "Whitespace around nested operators contradicts precedence."