Fix access lookup in lambda for noclasspath mode. #814

msteinbeck · 2016-09-02T17:19:10Z

This is a work in progress pull request showing my current work on an enhanced variable lookup for SingleNameReference tokens in lambda expressions (noclasspath mode). It took me some time to understand JDTTreeBuilder and all its helper classes but I think this is going into right direction, though.

As you might already have noticed, there are two test cases that fail. The first one is a good example for #813 and can be fixed by using int sourceEnd = qualifiedNameReference.sourceEnd();. The second test case fails because methodJDT.binding is null for anonymous classes`. Since I'm not sure how to create a type reference to an anonymous class (yet), I was not able to properly handle this case.

Furthermore, getVariableDeclaration is not finished yet as there might be a variable declared in a super class/interface. This case can be handled by iterating through all super classes/interfaces of a TypeBinding and checking if the potential variable has been declared in one of them. Unfortunately, I did not managed to create a TypeDeclaration from a TypeBinding yet. Maybe you have an idea how to do that.

However, in it's current state this pr produces much more precise AST in noclasspath mode as a SingleNameReference in a lambda expression is not always mapped to a CtLocalVariableReference. In addition, getVariableDeclaration reduces the amount of cloned code and ensures that a variable is visible in current context.

Until now, a token within a lambda (noclasspath mode) is always considered a variable access (in particular a CtLocalVariable), though, it also could refer to a type---a class for instance. This commit adds two features: 1) An improved variable lookup method which ensures that a found variable is visible in current context. Furthermore, the amount of cloned code has been reduced. 2) Check if a given token (SingleNameReference) is a variable or type access and create an appropriate AST node instead of always creating a local variable reference.

msteinbeck · 2016-09-03T14:17:45Z

The recent commit fixes the anonymous class issue by creating an 'empty' CtTypeReference if methodJDT.binding == null.

Fixes INRIA#813 as well.

msteinbeck · 2016-09-03T15:04:41Z

I fixed the second test case as well by replacing:

int sourceEnd = (int) (positions[qualifiedNameReference.indexOfFirstFieldBinding - 1] >>> 32) - 2;

with:

int sourceEnd = qualifiedNameReference.sourceEnd();

as described in #813.

The following things are missing:

properly handle fields of super classes/interfaces and statically imported fields
add a test case showing that a type access is mapped correctly (not mapped to a variable access)
handle catch variables

monperrus · 2016-09-04T13:36:23Z

Thanks, it looks interesting. However, "enhance" is vague, a PR is either a "bug fix" or a "new feature". What's this one? Could you add the associated test cases? Thanks.

msteinbeck · 2016-09-04T13:44:58Z

Could you add the associated test cases?

Working on it.

What's this one?

It is a bug fix.

monperrus · 2016-09-04T14:14:36Z

ok, I've renamed the PR.

…iables declared in super classes/interfaces.

msteinbeck · 2016-09-04T14:24:59Z

The recent commit adds the first lines to search for variables imported statically or declared by super classes/interfaces. Anyhow, the creation of the CtField is still missing since I'm not entirely sure how to properly create one.

msteinbeck · 2016-09-04T15:12:20Z

I have added a small test showing that a TypeAccess (Strings in our example) is mapped to a local variable access. Further tests with different variable access types, for instance, field access, statically imported variables and so on are coming soon. However, this simple test should give you an idea of what I'm trying to fix.

msteinbeck · 2016-09-04T16:45:18Z

I just added another test with a field access. In current master this test fails to compile the code due to #813.

msteinbeck · 2016-09-04T18:01:35Z

Recent commit should properly find variables in super classes/interfaces. I will add some test cases tomorrow.

Furthermore, fixed the test class FieldAccessInLambda.java.

…ble.

tdurieux · 2016-09-06T09:08:27Z

src/main/java/spoon/support/compiler/jdt/ContextBuilder.java

+		// try to find the variable on stack beginning with the most recent element
+		for (final ASTPair astPair : stack) {
+			// the variable may have been declared directly by one of these elements
+			final List<U> variables = astPair.element.getElements(new AbstractFilter<U>() {


Why do you use an AbstractFilter instead of EarlyTerminatingScanner?

I think there was a reason but let me have a look if an EarlyTerminatingScanner is feasible.

msteinbeck · 2016-09-06T12:03:12Z

Recent commit uses EarlyTerminatingScanner instead of AbstractFilter making it faster in finding variables.

…bleAccessNoClasspath`.

…ode.

msteinbeck · 2016-09-06T14:51:44Z

I think the PR should be ready now. I will write a summary later depicting what I improved/fixed.

msteinbeck · 2016-09-07T08:48:10Z

So, here is the summary:

I found that JDTTreeBuilder in noclasspath mode always creates a variable access for SingleNameReferences without proper binding (link), though, a SingleNameReference may point to a type access as well. (I've added a small example here). It is easy to see that 'something' imported non-statically must be a type access because you can not import non-static variables. Therefore, I added a simple if statement to JDTTreeBuilder checking if a singleNameReference points to variable or not. If it doesn't a type access is created instead (yes, it is some kind of heuristic but I will discuss this later).
Until now, JDTTreeBuilderHelper#createVariableAccessNoClasspath creates a reference to a lambda parameter or reference to a local variable only, though, a SingleNameReference may point to a field, a catch variable, and so on, as well. Thus, I added some lines to this method determining the actual type of a (potential) variable. In order to retrieve the actual type another method (getVariableDeclaration, which will be explained later) is called. If the given name does not point to a variable (variable == null) null is returned indicating that it must be something else. This return value is used by the JDTTreeBuilder as described above.
Since JDTTreeBuilderHelper#createVariableAccessNoClasspath allows to create a reference to a (potential) variable of arbitrary type, we need a function to determine the type of SingleNameReferences pointing to a variable. Hence, I was looking for a function providing this features and found something similar in ContextBuilder, more precisely, getLocalVariableDeclaration and getCatchVariableDeclaration. Unfortunately, they only support local variables and catch variables. By looking closely to the implementation, one can see that one of the two methods is a clone of the other. Thus, I decided to generalize them by adding a method for arbitrary variables types (hey, something we were looking for) which is used them (link).
There is a small but very significant TODO in getLocalVariableDeclaration saying that we need to validate the actual scope of the found variable. I will give you an example:

public class A {
    private final String title;
    ...
    public void aMethod(final String title) {
    ....
    }

    public void anotherMethod() {
        System.out.println(title); // <------
    }
}

In method anotherMethod, title obviously points to the field of A and not to the parameter of aMethod. Now, the current implementation may return the parameter which definitely is a bug! So, getVariableDeclaration not only generalizes getLocalVariableDeclaration and getCatchVariableDeclaration, but also makes sure that the found variable is in current scope! Furthermore, neither getCatchVariableDeclaration, nor getLocalVariableDeclaration examined super classes/interfaces and static imports for potential variables with matching name. I added a test case showing that this feature is necessary though.

From getVariableDeclaration's point of view, there is a situation in which we can not strictly decide whether a token points to a variable or a function call. It occurs if a static import of an unknown source for the given token is present. However, for noclasspath mode I added a heuristic checking if the name of the import consists of upper case characters only as, by Java Language Specification, this pattern is used for constant variables. I think it is the best we can do in this situation. Anyway, this heuristic does not affect JDTTreeBuilder#visit(SingleNameReference singleNameReference, BlockScope scope) as a function calls get visited by JDTTreeBuilder#visit(MessageSend messageSend, BlockScope scope)
As a side effect of the fixes mentioned above, one of the existing test cases fails due to Suspicious calculation of sourceEnd in JDTTreeBuilderHelper#createTypeAccessNoClasspath #813. The reason why it didn't failed before is, that without this pr the variable access was not mapped properly for this test and, therefore, (luckily) the execution path was omitted. I fixed it as well by applying the patch described in Suspicious calculation of sourceEnd in JDTTreeBuilderHelper#createTypeAccessNoClasspath #813.
Finally, I added a few test cases for all kind of variable accesses (hopefully I didn't miss one) with given source showing that my pr does not affect the regular case (link).

So, I would be appreciated if you could review this commit the next days (and maybe merge it into master) because I we really need the changes for our study we want to conduct the next days :). I know that this pr contains a lot of changes but maybe I could explain why they are necessary.

tdurieux · 2016-09-07T10:52:11Z

src/test/java/spoon/test/lambda/LambdaTest.java

+			public boolean matches(final CtTypeAccess element) {
+				return element.getAccessedType().getSimpleName().equals("Strings");
+			}
+		}).size(), 1);


The expected value (1) must be the second parameter of assertEquals

monperrus · 2016-09-07T12:30:17Z

I think this is a very nice PR, thanks a lot. It improves much the noclasspath mode. Since it's a large PR, I wait until tomorrow for merging, to let the other contributors have a look at it.

monperrus · 2016-09-08T12:48:29Z

Thanks a lot for your hard work on this touchy issue.

msteinbeck and others added 3 commits September 2, 2016 17:11

fixed a comment.

77b49a1

Properly handle parameter accesses in anonymous classes.

ae3b66a

Fixed source position calculation in createTypeAccessNoClasspath

99a3ba6

Fixes INRIA#813 as well.

monperrus changed the title ~~[WIP] Enhanced access lookup in lambda for noclasspath mode.~~ [WIP] fix access lookup in lambda for noclasspath mode. Sep 4, 2016

Added first lines to search for statically imported variables and var…

65f2fba

…iables declared in super classes/interfaces.

Added a simple test checking a type access in a lambda.

1de6f8e

Marcel Steinbeck added 2 commits September 4, 2016 18:15

Create concrete variable access.

5a3bb7a

Added a test showing that a field access is properly handled in lambdas.

54b6edf

Find variables declared in super classes/fields.

f12fbe8

msteinbeck added 6 commits September 5, 2016 11:16

Added several Tests for fields accesses.

91c1211

Improved variable lookup for fields.

da0bc7d

Added an example that statically imports a field from an interface.

efcb5b8

Furthermore, fixed the test class FieldAccessInLambda.java.

Added an example that statically imports a field from an unknown class.

fc91399

Avoid Exception in getVariableDeclaration.

741998b

Added an example that requires super class/interface lookup for varia…

e37b376

…ble.

tdurieux reviewed Sep 6, 2016
View reviewed changes

msteinbeck added 2 commits September 6, 2016 11:55

Fixed scope checking for declared variables.

838a958

Use EarlyTerminatingScanner instead of AbstractFilter.

5a710c4

Consider all types CtVariable and fixed documentation of `createVaria…

20a46a9

…bleAccessNoClasspath`.

msteinbeck added 3 commits September 6, 2016 15:03

Create a more accurate CtField from super class/interface.

bad5a9d

Removed unused import from JDTTreeBuilderHelper.

249a107

Added some heuristics for statically imported fields in noclasspath m…

bf081de

…ode.

msteinbeck changed the title ~~[WIP] fix access lookup in lambda for noclasspath mode.~~ Fix access lookup in lambda for noclasspath mode. Sep 6, 2016

Added missing EOF.

05fbc89

tdurieux reviewed Sep 7, 2016
View reviewed changes

msteinbeck added 2 commits September 7, 2016 14:18

Fixed expected and actual value in JUnit tests.

2c126dd

Fixed typo.

0420e46

Replaced some spaces with tabs.

5e5a776

monperrus merged commit 4322383 into INRIA:master Sep 8, 2016

tdurieux mentioned this pull request Sep 19, 2016

changelog Release 5.3.0 #828

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix access lookup in lambda for noclasspath mode. #814

Fix access lookup in lambda for noclasspath mode. #814

msteinbeck commented Sep 2, 2016 •

edited

Loading

msteinbeck commented Sep 3, 2016

msteinbeck commented Sep 3, 2016 •

edited

Loading

monperrus commented Sep 4, 2016

msteinbeck commented Sep 4, 2016

monperrus commented Sep 4, 2016

msteinbeck commented Sep 4, 2016

msteinbeck commented Sep 4, 2016

msteinbeck commented Sep 4, 2016

msteinbeck commented Sep 4, 2016

tdurieux Sep 6, 2016

msteinbeck Sep 6, 2016

msteinbeck commented Sep 6, 2016

msteinbeck commented Sep 6, 2016

msteinbeck commented Sep 7, 2016 •

edited

Loading

tdurieux Sep 7, 2016 •

edited

Loading

monperrus commented Sep 7, 2016

monperrus commented Sep 8, 2016

Fix access lookup in lambda for noclasspath mode. #814

Fix access lookup in lambda for noclasspath mode. #814

Conversation

msteinbeck commented Sep 2, 2016 • edited Loading

msteinbeck commented Sep 3, 2016

msteinbeck commented Sep 3, 2016 • edited Loading

monperrus commented Sep 4, 2016

msteinbeck commented Sep 4, 2016

monperrus commented Sep 4, 2016

msteinbeck commented Sep 4, 2016

msteinbeck commented Sep 4, 2016

msteinbeck commented Sep 4, 2016

msteinbeck commented Sep 4, 2016

tdurieux Sep 6, 2016

Choose a reason for hiding this comment

msteinbeck Sep 6, 2016

Choose a reason for hiding this comment

msteinbeck commented Sep 6, 2016

msteinbeck commented Sep 6, 2016

msteinbeck commented Sep 7, 2016 • edited Loading

tdurieux Sep 7, 2016 • edited Loading

Choose a reason for hiding this comment

monperrus commented Sep 7, 2016

monperrus commented Sep 8, 2016

msteinbeck commented Sep 2, 2016 •

edited

Loading

msteinbeck commented Sep 3, 2016 •

edited

Loading

msteinbeck commented Sep 7, 2016 •

edited

Loading

tdurieux Sep 7, 2016 •

edited

Loading