Java: Consider AssignOps in ArithExpr

[atorralba]

ArithExpr was considering unary and binary operators, but not compound operators, which are equally relevant for arithmetic-related questions. This PR adds AssignOps to the definition of ArithExpr to address that.

[smowton]

To consider while we're here: move out of Overflow.qll, probably to plain old Expr.qll? It seems like a generally useful category of expressions not particularly bound to the overflow queries.

[atorralba]

To consider while we're here: move out of Overflow.qll, probably to plain old Expr.qll? It seems like a generally useful category of expressions not particularly bound to the overflow queries.

I considered it, but the getOrdPrimitiveType predicate gave me a bit of pause, since it references NumType and OrdPrimitiveType, and thus we'd need to move them as well. Meaning that Overflow.qll would become empty (i.e. we'd need to remove it) and we'd have a bunch of predicates related to width and max values in classes of Expr.qll. Not that it would be too bad, but it seemed like a more significant change than what I'd be comfortable doing as a drive-by.

Happy to do it as its own thing in a different PR if you think it makes sense though.

[ebickle]

I gave this a try in a query I'm working on that depends on the arithmetic assign operations and everything worked great. Looks good to me!

[aschackmull]

LGTM

[intrigus-lgtm]

Isn't this change potentially breaking this code?
(Note that this class appears to be unused outside CodeQL itself, only being used indirectly here)

codeql/java/ql/lib/semmle/code/java/Conversions.qll

Lines 126 to 133 in 3e1164f

	/**
	* A numeric conversion. For example, `a * b` converts `a` and
	* `b` to have an appropriate numeric type.
	*
	* See the Java Language Specification, Section 5.4.
	*/
	class NumericConversionContext extends ConversionSite {
	ArithExpr e;

If we consider "Java Language Specification, Section 5.4." to actually refer to https://docs.oracle.com/javase/specs/jls/se7/html/jls-5.html#jls-5.6 then it does not list assignment arithmetic operators.

[ebickle]

I'm no expert, but a few ideas:

What would getType() return for the modified ArithExpr? If it's the type of the left-hand side of the expression, then wouldn't getConversionTarget() on NumericConversionContext still return the correct value?

The Java Language Specification is ambiguous in Section 5.4, but in Section 15.26.2 (Compound Assignment Operators) it states:

Otherwise, the saved value of the left-hand variable and the value of the right-hand operand are used to perform the binary operation indicated by the compound assignment operator. If this operation completes abruptly, then the assignment expression completes abruptly for the same reason and no assignment occurs.

That makes it seem as though a compound arithmetic assignment operator (e.g. *=) should be evaluated as first being a binary arithmetic operation with the two operands (*), then an assignment. I tried writing a few simple programs to verify and ran into some surprising behavior that seems like an oversight in either the JLS or the Java compiler.

Testing with both javac 1.8.0_382 and javac 21 (21.0.0.35.1) (wrapped with Maven 3.8.4 via mvn verify),
this program runs and outputs the value "2" to the console:

int x = 1;
x *= 2.5f;
System.console.println(x); // Prints "2"

This program fails to compile with the error "incompatible types: possible lossy conversion from float to int":

int y = 1;
y = y * 2.5f;
System.console.println(y);

It looks as though x *= y is equivalent to the result of x * y with the normal numeric promotion rules, then conversion rules (in this case, a narrowing primitive conversion) - but without any compiler warnings or errors, then assignment.

[smowton]

Note that the compound assignment operator description says that x op= y expands to x = (T)((x) op (y)), so the reason you're not seeing an error about float->int conversion is because the expression (int)(y * 2.5f) similarly would not produce any error.

Conversions-wise this means that x op= y manifests two conversions, first any widening required from the narrower to the wider of Tx and Ty (plus unboxing etc), then any narrowing conversions to Tx. With regard to the QL I'd recommend checking what the consequences of both "assignment context" and "numeric context" arms identifying the same site.

There's also the potentially surprising situation of x++ being identified as a ConversionSite. I haven't looked at whether that may cause trouble.