review: fix PotentialVariableDeclarationFunction ignoring the static scope #1536

pvojtechovsky · 2017-09-17T19:04:08Z

As reported in #1513 the PotentialVariableDeclarationFunction returns non static field test as potential variable declaration of local variable reference declared in static method.

int test = 0;  //this one should not be returned, because it is not static
public static void main(String[] args)
{
	int test = 1; 
	assertEqualst(1, test); //reference to local test variable
}

pvojtechovsky · 2017-09-23T21:59:14Z

This PR is now based on #1541 because it was needed to debug problem of MainTest

The implementation of this PR fixes two more problems

this implementation contains first lambda expression in Spoon java sources ... and that lambda expression code was not accepted by MainTest#testMain. See my change here @monperrus please check this test fix.
The MavenLauncherTest#spoonMavenLauncherTest is quite fragile - we are probably going to change these number quite often. Should we change it?

This PR is finished from my point of view too.

tdurieux · 2017-09-24T08:46:50Z

src/test/java/spoon/MavenLauncherTest.java

@@ -19,7 +19,7 @@ public void spoonMavenLauncherTest() {
 // with the tests
 launcher = new MavenLauncher("./", MavenLauncher.SOURCE_TYPE.ALL_SOURCE);
 // 235 because of the sub folders of src/main/java and src/test/java
- assertEquals(235, launcher.getModelBuilder().getInputSources().size());
+ assertEquals(236, launcher.getModelBuilder().getInputSources().size());


Maybe we can remove this assertion because it will be horrible to maintain.

monperrus · 2017-09-24T20:23:19Z

src/test/java/spoon/test/filters/FilterTest.java

@@ -883,16 +884,16 @@ public void testClassCastExceptionOnForEach() throws Exception {
 {
 Context context = new Context();
 //contract: if the query produces elements which cannot be cast to forEach consumer, then they are ignored
- launcher.getFactory().Package().getRootPackage().filterChildren(f->{return true;}).forEach((CtType t)->{
+ launcher.getFactory().Package().getRootPackage().filterChildren(null).forEach((CtType t)->{


why do you need to change this contract?

it is not change. It is just shorter way how write the same: "no filter = return all children"

monperrus · 2017-09-24T20:23:34Z

src/test/java/spoon/test/filters/FilterTest.java

 try {
- launcher.getFactory().Package().getRootPackage().filterChildren(f->{return true;}).forEach((CtType t)->{
+ launcher.getFactory().Package().getRootPackage().filterChildren(null).forEach((CtType t)->{


same question

monperrus · 2017-09-24T20:24:43Z

src/test/java/spoon/test/main/MainTest.java

@@ -177,6 +178,9 @@ public void scan(CtElement element) {
 if (reference.getParameters().get(i) instanceof CtArrayTypeReference && ((CtArrayTypeReference) reference.getParameters().get(i)).getComponentType() instanceof CtTypeParameterReference) {
 continue;
 }
+ if (executableDeclaration instanceof CtLambda) {


could you add a comment line to explain why we can not make the assertion for Lambdas?

I do not know whether we cannot make assertions about lambdas. Lambdas are too different (comparing to method) and they are not supported yet by this test. So may be there is necessary to write lambda specific assertion. But I did not understood the purpose of this test, so I would rather let You to fix it.

monperrus · 2017-09-24T20:25:23Z

looks good, commented with minor questions.

monperrus · 2017-09-24T21:20:01Z

Or we can have something like launcher.getModelBuilder().getInputSources().size() >= 230

pvojtechovsky · 2017-09-25T10:19:12Z

I have removed commits which belongs to #1541, so this PR is simpler now. The minor problems were fixed. So it can be merged

pvojtechovsky force-pushed the fixPotVariableAccessScope branch 2 times, most recently from 74b7293 to 794ae41 Compare September 23, 2017 21:35

pvojtechovsky changed the title ~~WiP: fix PotentialVariableDeclarationFunction ignoring the static scope~~ review: fix PotentialVariableDeclarationFunction ignoring the static scope Sep 23, 2017

tdurieux reviewed Sep 24, 2017

View reviewed changes

monperrus reviewed Sep 24, 2017

View reviewed changes

pvojtechovsky added 5 commits September 25, 2017 10:16

reproduce invalid potential variable in non static scope

0e7989c

fix PotentialVariableDeclarationFunction checking static scope now

492a239

fix: MainTest#testMain() checking of lambdas is wrong

82653e7

fix: MavenLauncherTest#spoonMavenLauncherTest

531737c

fix tests

6565a07

pvojtechovsky force-pushed the fixPotVariableAccessScope branch from edd126e to 6565a07 Compare September 25, 2017 09:22

monperrus merged commit ef93941 into INRIA:master Sep 25, 2017

pvojtechovsky deleted the fixPotVariableAccessScope branch September 27, 2017 19:18

surli mentioned this pull request Nov 14, 2017

[RELEASE] Major Spoon Release 6.0.0: done. #1699

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

review: fix PotentialVariableDeclarationFunction ignoring the static scope #1536

review: fix PotentialVariableDeclarationFunction ignoring the static scope #1536

pvojtechovsky commented Sep 17, 2017

pvojtechovsky commented Sep 23, 2017 •

edited

Loading

tdurieux Sep 24, 2017

monperrus Sep 24, 2017

pvojtechovsky Sep 25, 2017

monperrus Sep 24, 2017

monperrus Sep 24, 2017

pvojtechovsky Sep 25, 2017

monperrus commented Sep 24, 2017

monperrus commented Sep 24, 2017 via email

pvojtechovsky commented Sep 25, 2017

review: fix PotentialVariableDeclarationFunction ignoring the static scope #1536

review: fix PotentialVariableDeclarationFunction ignoring the static scope #1536

Conversation

pvojtechovsky commented Sep 17, 2017

pvojtechovsky commented Sep 23, 2017 • edited Loading

tdurieux Sep 24, 2017

Choose a reason for hiding this comment

monperrus Sep 24, 2017

Choose a reason for hiding this comment

pvojtechovsky Sep 25, 2017

Choose a reason for hiding this comment

monperrus Sep 24, 2017

Choose a reason for hiding this comment

monperrus Sep 24, 2017

Choose a reason for hiding this comment

pvojtechovsky Sep 25, 2017

Choose a reason for hiding this comment

monperrus commented Sep 24, 2017

monperrus commented Sep 24, 2017 via email

pvojtechovsky commented Sep 25, 2017

pvojtechovsky commented Sep 23, 2017 •

edited

Loading