INRIA / INRIA/spoon

[Bug]: Incorrect logic in VisitorPartialEvaluator

Open
#6,159 0 comments 0 reactions 0 assignees View on GitHub
bug
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.