Skip to content
This repository has been archived by the owner on Feb 6, 2019. It is now read-only.

Type of BinaryExpr is not correctly resolved in certain situations #364

Closed
malteskoruppa opened this issue Jan 10, 2018 · 14 comments
Closed
Labels

Comments

@malteskoruppa
Copy link

Consider the following expression:

String s = false + "str";

Trying to resolve the type of the subexpression false + "str" gives me the following ResolvedType: PrimitiveTypeUsage{name='boolean'}.

That's wrong of course, it should be a string. When the literals are the other way around, i.e., when I try to resolve "str" + false in the following expression:

String s = "str" + false;

...it works fine, I get a ReferenceType{java.lang.String, typeParametersMap=TypeParametersMap{nameToValue={}}}. That's what I would have expected in the first case too.

Is this a bug?

I tried to resolve the expression as follows. I used JavaParserFacade.get(typeSolver).getType(node). Here, typeSolver is a CombinedTypeSolver of a ReflectionTypeSolver and a JavaParserTypeSolver of a directory with a file that contains this above expression. And of course, node is the Expression that corresponds to the sub-expression false + "str" (like, expressionStmt.getExpression().asVariableDeclarationExpr() .getVariable(0).getInitializer().get()).

@ftomassetti
Copy link
Member

It most certainly seems like a bug. Here JSS is assuming your code is compiling, and when it is compilable code the result of a sum has the same type of the element on the left I think.
So in this case we should report a special type to indicate an error or maybe throw an exception.

@malteskoruppa
Copy link
Author

No, the type should be a String. See JLS 15.18.1 (https://docs.oracle.com/javase/specs/jls/se9/html/jls-15.html#jls-15.18.1) on the string concatenation operator:

If only one operand expression is of type String, then string conversion (§5.1.11) is performed on the other operand to produce a string at run time.

In other words, if either the left or the right side is a String, then the result of the concatenation is also a String. Please also note that the expression false + "str" is perfectly valid Java code that will compile fine. So you need not report a special type or throw an exception, you simply need to report the type String, as is already the case when the expression is the other way around ("str" + false). 😃

@ftomassetti
Copy link
Member

Oh, ok, I did not expect that. It should return a string then.

@malteskoruppa
Copy link
Author

Indeed, it is quite an unusual expression... but it's valid. 😄

@malteskoruppa
Copy link
Author

I just noticed that this also happens for other types, not only booleans:

'a' + "str" is reported as char instead of String. (It becomes the String: "astr")
1 + 2 + "foo" is reported as int instead of String. (It becomes the String: "3foo"... note that + is left-associative)

That is, JSS seems to always report the type of the left operand for +, but in case one operand is a String, the result is a String. Only when both operands are numeric does all the binary numeric promotion stuff happen... see JLS 15.18.1, 15.18.2, and 5.6.2 😃

@malteskoruppa
Copy link
Author

A similar problem appears for the remainder operator %. JSS reports the type of the left operand instead of the resulting type of the whole expression. Consider:

Object o = 3 % 2.71f;
System.out.println(o.getClass());

This prints: class java.lang.Float. However, JSS reports the type of the expression 3 % 2.71f as PrimitiveTypeUsage{name='int'} when it should be: PrimitiveTypeUsage{name='float'}. (Note that the result of 3 % 2.71f is 0.29f).

For mulitplication and division, this works fine (i.e., 3 * 2.71f and 3 / 2.71f are correctly reported as float), but for remainder, the reported type is incorrect. Note that Java does indeed support floating-point numbers on both sides of the remainder operator. See https://docs.oracle.com/javase/specs/jls/se9/html/jls-15.html#jls-15.17.3:

In C and C++, the remainder operator accepts only integral operands, but in the Java programming language, it also accepts floating-point operands.

I am going to test a few other operators at the start of next week, so maybe we'll find out that this bug affects other operators as well. So far, the following two operators are not handled correctly in all situations:

  • + for Strings
  • % for floats/doubles

@malteskoruppa malteskoruppa changed the title Type of BinaryExpr with boolean and String is not correctly resolved. Type of BinaryExpr is not correctly resolved in certain situations Jan 12, 2018
@ftomassetti
Copy link
Member

Nice, thank you @malteskoruppa

@malteskoruppa
Copy link
Author

A similar problem also occurs with the binary and operator &. Java Symbol Solver simply assumes the binary expression has the type of the left operand, when in reality, binary numeric promotion should be performed to determine the type correctly (see JLS 15.22.1: https://docs.oracle.com/javase/specs/jls/se9/html/jls-15.html#jls-15.22.1).

Examples:

System.out.println(((Object) ('G' & 6)).getClass()); // prints java.lang.Integer, but JSS reports char
System.out.println(((Object) ('z' & 1L)).getClass()); // prints java.lang.Long, but JSS reports char
System.out.println(((Object) (0x01 & 2L)).getClass()); // prints Java.lang.Long, but JSS reports int

Until this point, we can say that JSS erroneously reports the type of the left operand (instead of taking both operands into account) when trying to determine the type of a binary expression for the following operators:

  • PLUS
  • REMAINDER
  • BINARY_AND

@malteskoruppa
Copy link
Author

Ok, exactly the same bug as for BINARY_AND described in the previous post happens for BINARY_OR and XOR too.

So up to now, JSS erroneously reports the type of the left operand in certain situations when determining the type of a binary expression for the following operators:

  • PLUS
  • REMAINDER
  • BINARY_AND
  • BINARY_OR
  • XOR

I have tested all other operators too, except for the shift operators (<<, >>, >>>). They are working fine in all constellations that I tested (and I was pretty thorough). 😄 As for the shift operators, I'll test them tomorrow, then we'll have a good overview.

@malteskoruppa
Copy link
Author

(I accidentally clicked the wrong button and closed by mistake 😉)

@malteskoruppa
Copy link
Author

malteskoruppa commented Jan 16, 2018

Ok, I finished with my experiments, here comes the conclusion.

First, for the shift operators (which I hadn't tested yet), it mostly works, because in the case of shift operators, the type of the whole binary expression is usually the type of the left operand, and that is exactly the type JSS seems to report.

However, I noticed that unary numerical promotion is not implemented here. Consider the following snippet concerning shift operators from https://docs.oracle.com/javase/specs/jls/se9/html/jls-15.html#jls-15.19:

Unary numeric promotion (§5.6.1) is performed on each operand separately.
(...)
The type of the shift expression is the promoted type of the left-hand operand.

This means that in the following cases, for example....

Object test1 = 'B' << 1;
byte b = 8;
Object test2 = b >> 2;
short s = 0x0f;
Object test3 = s >>> 3;

...the types of the left operands of the shift expressions are char, byte, and short, respectively. Thus, JSS reports the type of the whole shift expression as char, byte, and short, respectively. However, due to unary numeric promotion, the left-hand operands are all converted to int to perform the shift operation. Thus, the type of all three shift expressions is int, respectively, contrarily to what JSS reports.

So to sum all of this up, JSS has problems in resolving the types of certain binary expressions. This is partly due to the typing rules of the operators not being implemented correctly (as in the case of false + "str", see above), partly due to unary/binary numeric promotion not being implemented (as in the case explained above).

The operators for which I could induce the erroneous behavior are:

  • PLUS
  • REMAINDER
  • BINARY_AND
  • BINARY_OR
  • XOR
  • LEFT_SHIFT
  • SIGNED_RIGHT_SHIFT
  • UNSIGNED_RIGHT_SHIFT

Examples can be found in the above posts. 😃

The correct type inference rules for all types of binary expressions can be found in JLS 15.17 through 15.24. Unary and binary numeric promotion are detailed in JLS 5.6.1 and 5.6.2, respectively.

@matozoid
Copy link
Contributor

I think @ftomassetti recently made a fix for this. Can I close this?

@ftomassetti
Copy link
Member

Well, I have solved it for the PLUS operator, but this remain open for other operations. I will create a new task on JP

@ftomassetti
Copy link
Member

Moved to JP, closing

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants