Set the type in factory methods of `CtBinaryOperator` and `CtUnaryOperator`
- Dominant language
- Java
- Stars
- 2k
- Forks
- 392
- Avg merge
- 11h 24m
- Merged PRs (30d)
- 36
Description
### Motivation
The factory for creating a `CtBinaryOperator` ([`CodeFactory#createBinaryOperator`](https://spoon.gforge.inria.fr/mvnsites/spoon-core/apidocs/spoon/reflect/factory/CodeFactory.html#createBinaryOperator(spoon.reflect.code.CtExpression,spoon.reflect.code.CtExpression,spoon.reflect.code.BinaryOperatorKind))) (same for `CtUnaryOperator`) does not set the type of the operator:
https://github.com/INRIA/spoon/blob/5d6a2d3e81333c700dce690da5eb9f5a7ba068c6/src/main/java/spoon/reflect/factory/CodeFactory.java#L89-L91
https://github.com/INRIA/spoon/blob/5d6a2d3e81333c700dce690da5eb9f5a7ba068c6/src/main/java/spoon/support/DefaultCoreFactory.java#L289-L293
This is not documented and can result in code crashing when it relies on the type being set.
For example `VisitorPartialEvaluator` relies on it to be present:
https://github.com/INRIA/spoon/blob/5d6a2d3e81333c700dce690da5eb9f5a7ba068c6/src/main/java/spoon/support/reflect/eval/VisitorPartialEvaluator.java#L217-L220
This can make it difficult to use with code created by the program through the factories, because one cannot always set the type to that of its left and right hand. For example, `((short) s + (byte) b)` will be of type `int`.
A real-word example would be this code, which represents the range of values a for-loop iterates over:
```java
public record ForLoopRange(
CtLocalVariableReference loopVariable,
CtExpression start,
CtExpression end) {
public static Optional fromCtFor(CtFor ctFor) {
// ...
}
public CtExpression length() {
CtExpression length = this.end;
// special case init with 0, because end - 0 = end
if (!SpoonUtil.isIntegerLiteral(this.start, 0)) {
length = this.loopVariable.getFactory().createBinaryOperator(
this.end,
this.start,
BinaryOperatorKind.MINUS
);
}
return length;
}
}
```
The `length()` function returns an expression that represents the number of elements the loop iterates over. For example `for (int i = 1; i < length + 1; i++)` would produce the length expression `(length + 1) - 1`. The `+ 1 - 1` can be removed and soon `PartialEvaluator` will be able to do remove it (I am working on implementing #5309).
The code would crash when invoking `PartialEvaluator`, because the type of the length expression is not set. Other types like `char`, `long`, ... are allowed, so I cannot unconditionally set the type to `int`. The start and end must not even have the same type; `for (char i = 'a'; i <= 122; i++)`.
The `length()` expression is used to give the user a suggestion to replace a for-loop with a function call like [`Arrays.copyOf`](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/Arrays.html#copyOf(T%5B%5D,int)). I do not want to suggest `Arrays.copyOf(array, (length + 1) - 1)`, when the `PartialEvaluator` could fix this. My current solution is to check for that case, but I can imagine that I will generate more complicated binary expressions in the future.
### Implementation
In #5291 I added support for promoting types, which makes it trivial to compute the resulting type of a binary operator.
I have not used the inference for anything except for tests (and there only for `CtBinaryOperator`):
https://github.com/INRIA/spoon/blob/5d6a2d3e81333c700dce690da5eb9f5a7ba068c6/src/test/java/spoon/test/eval/EvalTest.java#L330-L361
The code is well tested (through the partial evaluator tests, but one could add extra tests for this) and I am confident that it does not infer the wrong type. The `getPromotedType` method returns an `Optional.empty` when the combination of types is not valid with the operator. For the tests only valid operations are created, so I added an `orElseThrow`, but for the factory method it would be better to set the type to `null`. Who knows what downstream libraries might use `CtBinaryOperator` for.
### Alternatives
1. One could not change anything and let users deal with it. I do not like that one, because even if I had an extra function that infers the type correctly, and should be called to create binary operators, it is easy to forget calling it. I had a similar issue with `CtLiteral` and `PartialEvaluator` had the same problem, because it did not call `createLiteral(value)` (I think it called `createLiteral().setValue(value)`)
2. Provide an `@Internal` helper method to infer the type; `Optional> OperatorHelper.getTypeOf(CtBinaryOperator ctBinaryOperator)`
Contributor guide
Assessment
This issue has not been assessed yet.