[Bug]: Incorrect logic in VisitorPartialEvaluator

[Clipi-12]

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 CtElements that were not being evaluated (e.g. CtTry).
After that, some tests were failing, since the misleading method <R extends CtElement> 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 CtExpressions), 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

// 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). 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

for (int i = 0; ; ) { }

gets converted to a loop like this

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

if (element == null) return null;
element = element.clone();
element.accept(this);
return element;

and each visitXxxx can replace the parameter values. e.g.

// Modification of the previous visitCtBlock
@Override
public void visitCtStatementList(CtStatementList statements) {
	ArrayList<CtStatement> 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 CtInvocations, 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 extends CtElement> 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 <T> CtExpression<T> evaluate(CtExpression<T>), 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 at the time of writing this).

JVM Version

22

What operating system are you using?

Windows 10