Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

review: fix LocalVariableScopeFunction - CtFor, CtTryWithResource, Local class, shadow variables, ... #1154

Merged
merged 5 commits into from
Feb 9, 2017
Merged

review: fix LocalVariableScopeFunction - CtFor, CtTryWithResource, Local class, shadow variables, ... #1154

merged 5 commits into from
Feb 9, 2017

Conversation

pvojtechovsky
Copy link
Collaborator

See how ParentFunction #1153 is used in fixed LocalVariableScopeFunction.

This PR will be compilable after #1153 is merged.

I will add tests too, but after #1141 is finished, because there will be conflicts in test class.

@pvojtechovsky pvojtechovsky force-pushed the fixLocalVariableScopeFunction branch from b07c20e to 405fd77 Compare February 1, 2017 23:20
@pvojtechovsky
Copy link
Collaborator Author

This PR improves implementation of LocalVariableScopeFunction. It now correctly handles CtFor, CtTryWithResource. Tests actually fails because of issue #1159. They will pass after this issue is fixed.

I have extracted the tests from QueryTest to VariableReferencesTest, because QueryTest was too complicated and did not covered LocalVariables well. I plan (in another PR) to move remaining stuff from QueryTest to FieldReferenceTest, which needs different structure of test model. And then I will delete QueryTest.

Please have a look at ways how variable declarations are used in AllLocalVars and let me know if I have forgot some case.

@pvojtechovsky pvojtechovsky changed the title WiP: fix LocalVariableScopeFunction - loops, tryWithResource review: fix LocalVariableScopeFunction - CtFor, CtTryWithResource Feb 1, 2017
@pvojtechovsky pvojtechovsky force-pushed the fixLocalVariableScopeFunction branch 3 times, most recently from d3964d2 to 07bf832 Compare February 4, 2017 19:45
@pvojtechovsky pvojtechovsky changed the title review: fix LocalVariableScopeFunction - CtFor, CtTryWithResource WiP: fix LocalVariableScopeFunction - CtFor, CtTryWithResource Feb 6, 2017
@pvojtechovsky pvojtechovsky force-pushed the fixLocalVariableScopeFunction branch 2 times, most recently from 1139a97 to cebeff0 Compare February 6, 2017 21:28
@pvojtechovsky
Copy link
Collaborator Author

Here are more things. I am trying it all together, because it shares tests. It would need addition effort to implement separate tests of these fixes/features

  1. feature SiblingsFunction - visits prev/next/all siblings of input element. Is used to resolve local variable references and declarations
  2. fix of bug: CtLocalVariableReference#getDeclaration is not correct #1170, by new function LocalVariableReferencePossibleDeclarationFunction and its usage in CtLocalVariableReference#getDeclaration ... the name of this function is wild. Do you have better one?
  3. new implementation of LocalVariableReferenceFunction and LocalVariableScopeFunction
  4. much better, readable and extensible tests

This PR is not compilable because it needs #1164

@pvojtechovsky pvojtechovsky changed the title WiP: fix LocalVariableScopeFunction - CtFor, CtTryWithResource review: fix LocalVariableScopeFunction - CtFor, CtTryWithResource, Local class, shadow variables, ... Feb 6, 2017
@pvojtechovsky pvojtechovsky force-pushed the fixLocalVariableScopeFunction branch from cebeff0 to 6c1f1d4 Compare February 7, 2017 13:52
@pvojtechovsky
Copy link
Collaborator Author

Travis is ill?

@pvojtechovsky pvojtechovsky force-pushed the fixLocalVariableScopeFunction branch from 6c1f1d4 to 486f44b Compare February 7, 2017 18:18
@pvojtechovsky
Copy link
Collaborator Author

I have renamed LocalVariableReferencePossibleDeclarationFunction to VariableReferencePossibleDeclarationFunction, because this function can be used now to search for declarations or conflicts of all CtVariable instances. It is possible to switch off visiting of fields, because they are not needed in algorithm, which looks for conflicts with local variable.

@surli surli mentioned this pull request Feb 7, 2017
Closed
@pvojtechovsky pvojtechovsky mentioned this pull request Feb 7, 2017
Closed
@spoon-bot
Copy link
Collaborator

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.6.0-20170207.133111-28

New API: fr.inria.gforge.spoon:spoon-core:jar:5.6.0-SNAPSHOT

Detected changes: 0.

@pvojtechovsky
Copy link
Collaborator Author

What this pull_request-INRIA-spoon-master-docker problem means?

@pvojtechovsky pvojtechovsky mentioned this pull request Feb 7, 2017
Merged
monperrus
monperrus reviewed Feb 8, 2017
View reviewed changes
src/main/java/spoon/reflect/visitor/filter/VariableReferencePossibleDeclarationFunction.java Outdated
* }
* </pre>
*/
public class VariableReferencePossibleDeclarationFunction implements CtConsumableFunction<CtElement> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename to PotentialVariableDeclarationFunction?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. Thanks!

monperrus
monperrus reviewed Feb 8, 2017
View reviewed changes
src/test/java/spoon/test/query_function/AllLocalVars.java Outdated
import java.util.function.Consumer;

/*
* The values of the fields must be a sequence starting from 1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of this class? I don't really get it from the name and doc, and the "new AllLocalVars()" alone later seems strange.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the class which is used to build spoon model, which is then used for searching for variable declarations and then back their variable definitions. It belongs to package ...testclass...

new AllLocalVars(), runs all the code of this class and checks that all assertions are correct - that all variable references really refers the expected variable declaration - that test model input is valid.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Basically, you check the behavior of Java itself.

What about putting it as a real test class itself "JavaBehaviorTest" and annotate all methods with @Test? Together with a good Javadoc/warning, comment it should be maintainable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Basically, you check the behavior of Java itself.

I check that all the numbers which are later used in VariableReferencesTest are assigned well, following java behavior.

"JavaBehaviorTest", annotate methods with @test

ok, sounds good.

@monperrus
Copy link
Collaborator

monperrus commented Feb 8, 2017 via email

@spoon-bot
Copy link
Collaborator

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.6.0-20170207.234605-29

New API: fr.inria.gforge.spoon:spoon-core:jar:5.6.0-SNAPSHOT

Detected changes: 0.

@pvojtechovsky pvojtechovsky force-pushed the fixLocalVariableScopeFunction branch from 2653785 to 3f32392 Compare February 9, 2017 18:13
@spoon-bot
Copy link
Collaborator

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.6.0-20170209.143117-32

New API: fr.inria.gforge.spoon:spoon-core:jar:5.6.0-SNAPSHOT

Detected changes: 0.

@monperrus
Copy link
Collaborator

ready for merge?

@pvojtechovsky
Copy link
Collaborator Author

yes, I am done with that

@monperrus monperrus merged commit 05592e5 into INRIA:master Feb 9, 2017
@monperrus
Copy link
Collaborator

thanks!

@pvojtechovsky pvojtechovsky deleted the fixLocalVariableScopeFunction branch February 11, 2017 12:13
@tdurieux tdurieux mentioned this pull request Mar 15, 2017
Closed
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@monperrus monperrus monperrus left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pvojtechovsky @spoon-bot @monperrus