Skip to content
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 hashCode/equals from SuppressionFilter and mark SuppressFilterElement as immutable #4734

Closed
romani opened this issue Jul 16, 2017 · 8 comments

Comments

Projects
None yet
2 participants
@romani
Copy link
Member

commented Jul 16, 2017

from #4725

investigate why SuppressFilterElement and SuppressionFilter overrides hashcode/equals.
If not required - remove,
If required - try to avoid suppression of inspection.

Update:

Reason - these methods are here to just make testing easier.
Order of filter elements don't matter anywhere else, which would only be FilterSet which also has an equals/hashcode.

SuppressionFilter should be updated to not have equals in hashCode.
SuppressFilterElement should be updated to reference that it is immutable and keep both methods.
FilterSet will be updated in #6494 since it is API.

@rnveach

This comment has been minimized.

Copy link
Member

commented Jul 16, 2017

http://checkstyle.sourceforge.net/config_filters.html#SuppressionFilter
Since Checkstyle 3.2.
21f0b07#diff-11a82d59329f1ba81ad2ebf5bb923708R62
Since the beginning it has had these methods.

I removed equals/hash methods from SuppressionFilter and FilterSet and thee following tests failed:

[ERROR]   SuppressionsLoaderTest.testLoadFromClasspath:298 Suppressions were not loaded expected: com.puppycrawl.tools.checkstyle.api.FilterSet<[]> but was: com.puppycrawl.tools.checkstyle.api.FilterSet<[]>
[ERROR]   SuppressionsLoaderTest.testLoadFromUrl:88 Failed to load from url expected: com.puppycrawl.tools.checkstyle.api.FilterSet<[]> but was: com.puppycrawl.tools.checkstyle.api.FilterSet<[]>
[ERROR]   SuppressionsLoaderTest.testMultipleSuppression:136 Multiple suppressions were loaded incorrectly expected: com.puppycrawl.tools.checkstyle.api.FilterSet<[com.puppycrawl.tools.checkstyle.filters.SuppressElement@c8234bab, com.puppycrawl.tools.checkstyle.filters.SuppressElement@1e52c1ce, com.puppycrawl.tools.checkstyle.filters.SuppressElement@21371aeb, com.puppycrawl.tools.checkstyle.filters.SuppressElement@cb07a4c8]> but was: com.puppycrawl.tools.checkstyle.api.FilterSet<[com.puppycrawl.tools.checkstyle.filters.SuppressElement@c8234bab, com.puppycrawl.tools.checkstyle.filters.SuppressElement@1e52c1ce, com.puppycrawl.tools.checkstyle.filters.SuppressElement@21371aeb, com.puppycrawl.tools.checkstyle.filters.SuppressElement@cb07a4c8]>
[ERROR]   SuppressionsLoaderTest.testNoSuppressions:64 No suppressions should be loaded, but found: 0 expected: com.puppycrawl.tools.checkstyle.api.FilterSet<[]> but was: com.puppycrawl.tools.checkstyle.api.FilterSet<[]>

which looks like because we are trying to do comparisons of expected/actual.

@rnveach

This comment has been minimized.

Copy link
Member

commented Jul 16, 2017

Removing the methods from SuppressElement creates unused variable errors:

Description	Resource	Path	Location	Type
The value of the field SuppressElement.checkPattern is not used	SuppressElement.java	/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/filters	line 54	Java Problem
The value of the field SuppressElement.columnsCsv is not used	SuppressElement.java	/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/filters	line 69	Java Problem
The value of the field SuppressElement.filePattern is not used	SuppressElement.java	/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/filters	line 48	Java Problem
The value of the field SuppressElement.linesCsv is not used	SuppressElement.java	/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/filters	line 63	Java Problem

but no new tests fail.

@rnveach

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

@romani From my previous comments, it looks like these methods are here to just make testing easier.
Order of filter elements don't matter anywhere else, which would only be FilterSet which also has an equals/hashcode.

We still want to remove them?

@romani romani changed the title investigate why SuppressElement and SuppressionFilter overrides hashcode/equals investigate why SuppressFilterElement and SuppressionFilter overrides hashcode/equals Mar 3, 2019

@romani

This comment has been minimized.

Copy link
Member Author

commented Mar 3, 2019

it looks like these methods are here to just make testing easier

main code should be damages to easy testing, there are exceptions, but this case is not good reason. Testing code can use any sort of ordering to make tests convenient.

Other good reason to keep equals(and as result hashcode) is when we state that object is immutable, so user can rely on equals. So with such idea SuppressFilterElement can keep equals, but SuppressionFilter should not have such methods overridden.

So we can update doc of SuppressFilterElement to reference that it is immutable. SuppressionFilter should be updated to not have equals in hashCode.

@rnveach , does it make sense ?
If yes, lets update title of issue and description.

@rnveach

This comment has been minimized.

Copy link
Member

commented Mar 3, 2019

SuppressionFilter should be updated to not have equals in hashCode.

Ok.

SuppressFilterElement can keep equals
update doc of SuppressFilterElement to reference that it is immutable

I assume hashCode method is also staying for SuppressFilterElement?

What about FilterSet? It is not immutable since it has a removeFilter method. Checker is the only class that uses this method, and it doesn't use it directly except in a test. It is just a public method anyone can use it if they use the direct class. It is not supported by RootModule interface.

@romani

This comment has been minimized.

Copy link
Member Author

commented Mar 3, 2019

I assume hashCode method is also staying for SuppressFilterElement?

yes, they almost always together.

What about FilterSet?

it should not have them also. With such override, user can put object to map, changes object, try to find the same object in map, got nothing.
But this class is API, it should be done as separate issue with breacking compatibility label.

@rnveach rnveach changed the title investigate why SuppressFilterElement and SuppressionFilter overrides hashcode/equals Remove hashCode/equals from SuppressionFilter and mark SuppressFilterElement as immutable Mar 3, 2019

@rnveach

This comment has been minimized.

Copy link
Member

commented Mar 3, 2019

If yes, lets update title of issue and description.

Done.

rnveach added a commit to rnveach/checkstyle that referenced this issue Mar 3, 2019

rnveach added a commit to rnveach/checkstyle that referenced this issue Mar 3, 2019

romani added a commit that referenced this issue Mar 3, 2019

@romani romani added the bug label Mar 3, 2019

@romani romani added this to the 8.19 milestone Mar 3, 2019

@romani romani added miscellaneous and removed bug labels Mar 3, 2019

@rnveach

This comment has been minimized.

Copy link
Member

commented Mar 3, 2019

Fix was merged

@rnveach rnveach closed this Mar 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.