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

Remove usage of DetailAST.branchContains #5124

Open
rnveach opened this Issue Sep 18, 2017 · 5 comments

Comments

Projects
None yet
2 participants
@rnveach
Member

rnveach commented Sep 18, 2017

Identified at #4727 (comment) ,

branchContains is a dangerous method to use all the time since there is no restriction on how deep it can search.

The referenced issue fixed a bug where we used branchContains and it was scanning deeper than the check originally intended. It was supposed to be looking for a variable with a final modifier, but because of branchContains it scanned into an anonymous class in the assignment and picked up the final there.

There may be other cases in our code where it might be searching too much.
Example:

if (method.branchContains(TokenTypes.PARAMETER_DEF)

Because we are doing a branchContains on the METHOD_DEF it is also searching inside the SLIST of the method and may pick-up anonymous classes while it is looking for PARAMETER_DEF.

I feel branchContains should be completely removed as it's search area can't be controlled. If we don't want to remove it, it's use should be heavily controlled.

We should swap it's current uses with either findFirstToken or a new method in each check that specifically controls where and what we are looking for.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 21, 2017

Member

this method just do search in tree, nothing related to any logic of any Check.

user can launch this method of root and receive even more interesting results, that probably not up to its expectations.

This looks like just simple mis-usage of some method by engineer.
We can probably move it Utils , but it can be misused from there too.

Please share more points to support removal of it.
Do you think that in all use cases it is error-prone ?

Member

romani commented Sep 21, 2017

this method just do search in tree, nothing related to any logic of any Check.

user can launch this method of root and receive even more interesting results, that probably not up to its expectations.

This looks like just simple mis-usage of some method by engineer.
We can probably move it Utils , but it can be misused from there too.

Please share more points to support removal of it.
Do you think that in all use cases it is error-prone ?

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Sep 21, 2017

Member

@romani I will check again, but when I looked the majority of cases where calling the method on modifiers looking for static/public/etc... Unless we can define those modifiers inside or on an annotation, I don't think there are other cases to support.

I still think it should be removed because it's use case will be rare.
With modifiers, there is no reason to not just use findFirstToken which searches only the first level children which is what most of the use cases are.

Member

rnveach commented Sep 21, 2017

@romani I will check again, but when I looked the majority of cases where calling the method on modifiers looking for static/public/etc... Unless we can define those modifiers inside or on an annotation, I don't think there are other cases to support.

I still think it should be removed because it's use case will be rare.
With modifiers, there is no reason to not just use findFirstToken which searches only the first level children which is what most of the use cases are.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 23, 2017

Member

ones we come to immutable AST model, such method will go utils for sure.

Member

romani commented Sep 23, 2017

ones we come to immutable AST model, such method will go utils for sure.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Sep 25, 2017

Member

@romani It can't really go outside the AST class because it has to build up all the children trees recursively and cache them through getBranchTokenTypes.
If we were to remove the cache and place it inside a util out of the AST, then this is even more reason to just remove it. Without the cache, this method will be even slower if used in areas with deep trees.

Member

rnveach commented Sep 25, 2017

@romani It can't really go outside the AST class because it has to build up all the children trees recursively and cache them through getBranchTokenTypes.
If we were to remove the cache and place it inside a util out of the AST, then this is even more reason to just remove it. Without the cache, this method will be even slower if used in areas with deep trees.

@romani romani added the approved label Sep 28, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 28, 2017

Member

ok, let start this process.
BUT in this issue lets remove only references , as removal of method itself will be breaking compatibility.

Member

romani commented Sep 28, 2017

ok, let start this process.
BUT in this issue lets remove only references , as removal of method itself will be breaking compatibility.

@romani romani changed the title from Remove DetailAST.branchContains and all references to Remove usage of DetailAST.branchContains Sep 28, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Oct 25, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Oct 25, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Oct 25, 2017

romani added a commit that referenced this issue Oct 26, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Oct 26, 2017

@romani romani added this to the 8.4 milestone Oct 29, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Nov 7, 2017

romani added a commit that referenced this issue Nov 9, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Nov 16, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Nov 18, 2017

romani added a commit that referenced this issue Nov 18, 2017

romani added a commit that referenced this issue Nov 22, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Nov 24, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Nov 26, 2017

romani added a commit that referenced this issue Nov 26, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Dec 19, 2017

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