[Bug]: Incorrect logic in VisitorPartialEvaluator
- Dominant language
- Java
- Stars
- 2k
- Forks
- 392
- Avg merge
- 11h 24m
- Merged PRs (30d)
- 36
Description
# Bugs
While using `CtCodeElement#partiallyEvaluate` I noticed invocations like `Integer.toString(number, 16)` where not being evaluated since the logic only accepts null targets or `CtLiteral` targets. I tried cloning the repo and modifying the source-code, with the intent of possibly making a pull-request.
While modifying some of the logic in that file, I also added handling for `CtElement`s that were not being evaluated (e.g. `CtTry`).
After that, some tests were failing, since the misleading method ` R evaluate(R element);` made me think that "if an element of type _R_ is evaluated, an element of type _R_ will be returned, for any _R_".
That may be true for some kinds of Rs (I'm pretty sure it's true for `CtExpression`s), but not for **all** Rs. For example, a `CtInvocation` may be evaluated to a `CtLiteral`.
This already causes errors on its own. For example, a `CtFor` like
```java
// Yes, this is valid Java code; the first part of a for-loop
// can be any statement, not just variable assignments
for ("".toString(); ;) { }
```
will fail to evaluate (throwing a `ClassCastException`), since the first element will not evaluate to a `CtStatement`.
While looking at the `visitCtFor` method in `VisitorPartialEvaluator` I also noted other bugs, like the `forLoop` not being cloned (meaning the original CtFor was being modified, breaking [the method's contract](https://spoon.gforge.inria.fr/mvnsites/spoon-core/apidocs/spoon/reflect/code/CtCodeElement.html#partiallyEvaluate():~:text=The%20element%20is%20always%20cloned%2C%20even%20if%20nothing%20has%20been%20evaluated.)). Another bug involves the fact that, since there is no clone, the elements are being appended to the original loop, so a loop like this
```java
for (int i = 0; ; ) { }
```
gets converted to a loop like this
```java
for (int i = 0, i = 0; ; ) { }
```
Another problem is that the actual block isn't even getting evaluated (the same problem that `CtTry` had).
# A different approach
I could probably take a more in-depth look at the code and find similar bugs, but honestly I think the solution is to approach the problem with a different strategy. Maybe `VisitorPartialEvaluator#evaluate` can be implemented like
```java
if (element == null) return null;
element = element.clone();
element.accept(this);
return element;
```
and each `visitXxxx` can replace the parameter values. e.g.
```java
// Modification of the previous visitCtBlock
@Override
public void visitCtStatementList(CtStatementList statements) {
ArrayList newStatements = new ArrayList<>();
for (CtStatement s : statements.getStatements()) {
CtElement res = evaluate(s);
if (res != null) {
if (res instanceof CtStatement) {
newStatements.add((CtStatement) res);
} else {
// the context expects statement. We cannot simplify in this case
newStatements.add(s);
}
}
// do not copy unreachable statements
if (flowEnded) {
break;
}
}
statements.setStatements(newStatements);
}
```
In the case of `CtInvocation`s, they could just call `.replace(...)` on the parameter.
This would eliminate the need to handle `setResult`, the forgotten `clone()`s, and non-evaluated elements (like the `CtTry`, or the body of the `CtFor`).
However, the problems caused by the misleading method ` R evaluate(R element);` would still occur. As I already said, I have a copy of the source-code, and I don't mind replacing that method with methods like ` CtExpression evaluate(CtExpression)`, `CtCodeElement evaluate(CtCodeElement)` and so forth, in my fork; but maybe you-all don't want to change the public API that much.
If that's a problem, my first suggestion probably doesn't fit your backwards-compatibility policy, since it depends on modifying the parameter instead of cloning it.
# Meta info
### Spoon Version
master branch ([f80cf26](https://github.com/INRIA/spoon/commit/f80cf26bca17eb6989bb9dcf39c48dd44c3eee67) at the time of writing this).
### JVM Version
22
### What operating system are you using?
Windows 10
Contributor guide
Assessment
This issue has not been assessed yet.