Bug with variable declaration resolving after clone

[fedorov-s-n]

Problem

After cloning AST subtree variable references are resolved to wrong declaration that makes it difficult to perform further modifications.

Example

Suppose there's a code:

public class A {

    int field;

    public void b(int param) {
        IntUnaryOperator f1 = x -> x;
        IntBinaryOperator f2 = (a, b) -> a + b;
        try {
            System.out.println(f1.applyAsInt(f2.applyAsInt(field, param)));
        } catch (RuntimeException e) {
            throw e;
        }
    }
}

Goal is to clone class A, rename copy to B and perform some modifications with B.

Problems with example

1. Local variables references f1, f2 in B.b() still refer to declarations in A.b(). The same is true about Catch variable reference e
2. Method parameter reference and and class field reference are resolved to declarations in A.b() and A correspondingly because they resolves using type reference that points to A
3. Resolving variables using plain field reference (local and catch variables) and dynamic lookup (field and parameter variables) are different strategies, and that's not clear that different variable types use different strategies. Developers who use Spoon API should look to sources to understand it.
4. In scenario that extensively uses resolving declaration by reference lookups took longer than field read.

Proposed solution

1. remove setDeclaringType and setDeclaringExecutable methods, references are not declared! (fields and parameters are)
2. add setDeclaration method to CtVariableReference
3. cache declaration in reference if it was already resolved, allow to invalidate cache
4. set unified resolving strategy for all variable types

Prototype code:

public interface CtVariableReference<T, V extends CtVariable<T>> extends CtReference {

    @Override
    public V getDeclaration();

    public void setDeclaration(V declaration);
}

public abstract class CtVariableReferenceImpl<T, V extends CtVariable<T>> extends CtReferenceImpl implements CtVariableReference<T, V> {

    private V declaration = null;

    @Override
    public V getDeclaration() {
        if (declaration == null) {
            declaration = lookupDeclaration();
        }
        return declaration;
    }

    /**
     *
     * @param declaration if not null, set declaration of this variable
     * reference; if null, set declaration as unknown and lookup it on next call
     * to `getDeclaration()`
     *
     */
    @Override
    public void setDeclaration(V declaration) {
        this.declaration = declaration;
    }

    protected abstract V lookupDeclaration();
}

public class CtLocalVariableReferenceImpl<T> extends CtVariableReferenceImpl<T, CtLocalVariable<T>> implements CtLocalVariableReference<T> {

    @Override
    protected CtLocalVariable<T> lookupDeclaration() {
        CtElement element = this;
        Optional<CtLocalVariable> optional;
        String name = getSimpleName();
        do {
            CtStatementList block = element.getParent(CtStatementList.class);
            optional = block.getStatements()
                .stream()
                .filter(s -> s instanceof CtLocalVariable)
                .map(s -> (CtLocalVariable) s)
                .filter(var -> name.equals(var.getSimpleName()))
                .findFirst();
            element = block;
        } while (!optional.isPresent());
        return optional.get();
    }
}

public class CtCatchVariableReferenceImpl<T> extends CtVariableReferenceImpl<T, CtCatchVariable<T>> implements {

    @Override
    protected CtCatchVariable<T> lookupDeclaration() {
        CtElement element = this;
        String name = getSimpleName();
        CtCatchVariable var;
        do {
            CtCatch catchBlock = element.getParent(CtCatch.class);
            var = catchBlock.getParameter();
            element = catchBlock;
        } while (!name.equals(var.getSimpleName()));
        return var;
    }
}

public class CtFieldReferenceImpl<T> extends CtVariableReferenceImpl<T, CtField<T>> implements CtFieldReference<T> {

    @Override
    protected CtField<T> lookupDeclaration() {
        CtElement element = this;
        Optional<CtField> optional;
        String name = getSimpleName();
        do {
            CtType type = element.getParent(CtType.class);
            optional = ((List<CtField>) type.getFields())
                .stream()
                .filter(f -> name.equals(f.getSimpleName()))
                .findFirst();
            element = type;
        } while (!optional.isPresent());
        return optional.get();
    }
}

public class CtParameterReferenceImpl<T> extends CtVariableReferenceImpl<T, CtParameter<T>> implements CtParameterReference<T> {

    @Override
    protected CtParameter<T> lookupDeclaration() {
        CtElement element = this;
        Optional<CtParameter> optional;
        String name = getSimpleName();
        do {
            CtExecutable executable = element.getParent(CtExecutable.class);
            optional = ((List<CtParameter>) executable.getParameters())
                .stream()
                .filter(var -> name.equals(var.getSimpleName()))
                .findFirst();
            element = executable;
        } while (!optional.isPresent());
        return optional.get();
    }
}

+/-

• +Uninfied access to changing declaration
• +Allows to keep references after cloning
• -Requires change to API
• -Introduces lookup failures for local and catch variables (if parent not initialized)

[monperrus]

Hi,

1. Local variables references f1, f2 in B.b() still refer to declarations in A.b(). The same is true about Catch variable reference e
   1. Method parameter reference and and class field reference are resolved to declarations in A.b() and A correspondingly because they resolves using type reference that points to A

That's clearly a bug. What is already there before the major clone refactoring? or is it a regression?

1. Resolving variables using plain field reference (local and catch variables) and dynamic lookup (field and parameter variables) are different strategies, and that's not clear that different variable types use different strategies.

The design intent is clearly to only have dynamic lookup. It should also be the case for local and catch variables (so as to move and clone all elements easily, and even make some dynamic checks about scoping issues in setters. We'll have a look at this.

Developers who use Spoon API should look to sources to understand it.

This is indeed a problem. The end users should never have to look at the source code.

[fedorov-s-n]

That's clearly a bug. Was is already there before the major clone refactoring? or is it a regression?

It was the same in spoon 5.0

The design intent is clearly to only have dynamic lookup. It should also be the case for local and catch variables (so as to move and clone all elements easily, and even make some dynamic checks about scoping issues in setters). We'll have a look at this.

Then it looks like I propose to change things back to 'as designed' :)

[fedorov-s-n]

What about caching?

[monperrus]

Let's first get it correct and then we'll study how to optimize for instance with caching :-).

PR welcome of course ^-^

[fedorov-s-n]

PR

hmm, sorry, don't know this shortcut. what did you mean?

[monperrus]

PR = Pull Request :-)

[fedorov-s-n]

@GerardPaligot FYI I have some changes I'm going to contribute

[fedorov-s-n]

Please consider #765 as well because it contains resolving of fields in case of entire class cloning