Wrong detection of field vs setter access within closure

[mauromol]

This was (part of) the old GRECLIPSE-1766

Consider the following code:

package o 

class O { 
	String foo 
	
	void setFoo(String foo) { 
		println "setter called: $foo" 
		this.foo = foo // (A)  
	} 
	
	void doSomething() { 
		println 'setting foo to "foo"' 
		foo = 'foo' // (B)  
		println 'setting foo to "bar"' 
		def c = { foo = 'bar' } // (C)  
		c() 
	} 
	
	static void main(String[] args) { 
		O o = new O() 
		println 'invoking doSomething()' 
		o.doSomething() 
		println 'setting foo to "foobar"' 
		o.foo = 'foobar' // (D)  
	} 
} 

In (A) and (B) the foo field is accessed directly and Greclipse correctly detects this (by colouring it accordingly, by showing this fact when you hover over foo and/or press F2 and by providing the correct navigation with F3).
In (C) and (D), instead, the foo field is accessed through its setter. While in (D) Greclipse correctly determines that (i.e.: it uses a different colour, F2 and F3 work correctly), in (C) Greclipse thinks that foo is accessed directly, but this is not the case, as you can test by running the main method.

This might appear a trivial problem, but it can be very useful for the developer to distinguish between the two cases, especially (as an example) when foo is annotated with @Bindable: in (C) and (D) listeners will be invoked, in (A) and (B) they won't.

[eric-milles]

Implemented in this method in SimpleTypeLookup. Accessors are passed up when static does not match but fields and properties are not checked the same. Probably should be checking against isStaticExpression all the way through.

    protected ASTNode findDeclaration(String name, ClassNode declaringType, boolean isLhsExpression, boolean isStaticExpression, List<ClassNode> methodCallArgumentTypes) {
        if (declaringType.isArray()) {
            // only length exists on arrays
            if (name.equals("length")) {
                return createLengthField(declaringType);
            }
            // otherwise search on object
            return findDeclaration(name, VariableScope.OBJECT_CLASS_NODE, isLhsExpression, isStaticExpression, methodCallArgumentTypes);
        }

        if (methodCallArgumentTypes != null) {
            ASTNode method = findMethodDeclaration(name, declaringType, methodCallArgumentTypes);
            if (method != null) {
                return method;
            }
            // name may still map to something that is callable; keep looking
        }

        // look for canonical accessor method
        MethodNode accessor = AccessorSupport.findAccessorMethodForPropertyName(name, declaringType, false, !isLhsExpression ? READER : WRITER);
        if (accessor != null && !isSynthetic(accessor) && (accessor.isStatic() == isStaticExpression)) {
            return accessor;
        }

        LinkedHashSet<ClassNode> typeHierarchy = new LinkedHashSet<ClassNode>();
        VariableScope.createTypeHierarchy(declaringType, typeHierarchy, true);

        // look for property
        for (ClassNode type : typeHierarchy) {
            PropertyNode property = type.getProperty(name);
            if (property != null && (property.isStatic() == isStaticExpression)) {
                return property;
            }
        }

        // look for field
        FieldNode field = declaringType.getField(name);
        if (field != null && (field.isStatic() == isStaticExpression)) {
            return field;
        }

        typeHierarchy.clear();
        VariableScope.findAllInterfaces(declaringType, typeHierarchy, true);

        // look for constant in interfaces
        for (ClassNode type : typeHierarchy) {
            if (type == declaringType) {
                continue;
            }
            field = type.getField(name);
            if (field != null && field.isFinal() && field.isStatic()) {
                return field;
            }
        }

        // look for static or synthetic accessor
        if (accessor != null) {
            return accessor;
        }

        if (methodCallArgumentTypes == null) {
            // reference may be in method pointer or static import; look for method as last resort
            return findMethodDeclaration(name, declaringType, null);
        }

        return null;
    }

[eric-milles]

ready to test

[eric-milles]

One note: methods still lack the static/non-static checking. Therefore, you may reference an instance method from a closure within a static field/method and get no warning of the impending runtime error.

Implicit-this methods (no object expression) are represented as ConstantExpression nodes in the AST instead of VariableExpression nodes (for free variables), and take a separate path for resolution.

[mauromol]

Hi Eric, I just updated to 2.9.2.xx-201711060326-e46, but I find a regression here, rather than an improvement. I'm not sure I can follow what you say in your last message... is it a hint about the requirement to open a wider improvement request?

Indeed, what I see now is that in (A), this.foo is detected now as a call to setFoo(String) (which of course is wrong), while in (C) foo = 'bar' is still see as a direct reference to O.foo, rather than a call to O.setFoo(String).

[mauromol]

I retested this with 2.9.2.xx-201711070247-e46 and it seems to be fixed now, thank you! 👍