New issue

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

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

Already on GitHub? Sign in to your account

DetailAST should invalidate its methods cache (aka lazy-load) #3466

Closed
MEZk opened this Issue Sep 24, 2016 · 4 comments

Comments

Projects
None yet
3 participants
@MEZk
Contributor

MEZk commented Sep 24, 2016

Problem description:

Methods from DetailAST such as getBranchTokenTypes, branchContains, getColumnNo, getLineNo, getChildCount use lazy-load approach. These methods can be invoked by the checks which are not subscribed to receive AST with comment nodes first and than be invoked by the checks which are subscribed to get AST with comment nodes. As a result the inner state of the DetailAST object might be "cached" without comment nodes in AST after the first invocation as the methods use lazy-load. Such methods will return invalid results when they are invoked for the AST which can be accessed in checks that are subscribed to receive comment nodes and for which are not.

Possible sollution:

One way forward to resolve the problem is to invalidate DetailAST methods "cache".

Resolving of the issue will allow us to fix #3102 .

MEZk added a commit to MEZk/checkstyle that referenced this issue Sep 27, 2016

MEZk added a commit to MEZk/checkstyle that referenced this issue Sep 30, 2016

MEZk added a commit to MEZk/checkstyle that referenced this issue Sep 30, 2016

@romani romani added this to the 7.2 milestone Sep 30, 2016

romani added a commit that referenced this issue Sep 30, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 30, 2016

Member

fix is merged

Member

romani commented Sep 30, 2016

fix is merged

@romani romani closed this Sep 30, 2016

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Sep 30, 2016

Member

@romani Fix invalidated branchTokenTypes cache but it did not invalidate childCount which was also in issue's description.
Since we also cache childCount and don't reset it on add/set children/siblings methods, it will keep count from non-comment trees into comment trees, as long as we don't call setFirstChild which is the only method to invalidate it currently. To avoid calling setFirstChild, issue can be seen with code samples like public /* comment */ static void method () {} where a comment/javadoc is between 2 other similar nodes.

Shouldn't it be added to this issue?

Member

rnveach commented Sep 30, 2016

@romani Fix invalidated branchTokenTypes cache but it did not invalidate childCount which was also in issue's description.
Since we also cache childCount and don't reset it on add/set children/siblings methods, it will keep count from non-comment trees into comment trees, as long as we don't call setFirstChild which is the only method to invalidate it currently. To avoid calling setFirstChild, issue can be seen with code samples like public /* comment */ static void method () {} where a comment/javadoc is between 2 other similar nodes.

Shouldn't it be added to this issue?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Oct 1, 2016

Member

We did not found problem with childCount , ut was extended to check count.

If you still see a problem, please reproduce it with UT and do PR , could be done in separate issue.

Member

romani commented Oct 1, 2016

We did not found problem with childCount , ut was extended to check count.

If you still see a problem, please reproduce it with UT and do PR , could be done in separate issue.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Oct 1, 2016

Member

getChildCount was called on METHOD_DEF in UT while the comments are located as a child of MODIFIERS in input. So the UT isn't directed at the correct location to show this issue.
I will create PR to showcase.

Member

rnveach commented Oct 1, 2016

getChildCount was called on METHOD_DEF in UT while the comments are located as a child of MODIFIERS in input. So the UT isn't directed at the correct location to show this issue.
I will create PR to showcase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment