-
Notifications
You must be signed in to change notification settings - Fork 54
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
SuppressWithNearbyTextFilter only changes after purging cache #589
Comments
Unless you are using Checkstyle's cache file system ( https://checkstyle.org/config.html#Checker_Properties ), this sounds like #465 , which is a eclipse-cs issue. The way to confirm if this is main Checkstyle's issue, is by recreating the issue with the CLI ( https://checkstyle.org/report_issue.html#How_to_report_a_bug.3F ) . |
The new filter caches the last file it read, to avoid repeated file reading if multiple violations in the same file have to be checked for whether a filter applies: https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/filters/SuppressWithNearbyTextFilter.java#L198 That's good from the viewpoint of efficiency, but bad from the viewpoint of lifecycle in eclipse-cs. If there is only a single file with violations (which is basically always the case when triggering a scan from the save editor event), then repeated invocations of the filter in eclipse-cs will NOT reload the file, even though it might have changed in between. Unfortunately there is no concept of "life cycle" for filters to cleanly reset that cached state. I see 2 alternative approaches:
I currently tend to do the reflection based reset. That way the root cause (the lifecycle in eclipse-cs) and the fix remain close to each other. |
The purpose of this check was NOT to re-read the file again. If we are to drop this and always re-read the file, then there is no point in having a check at all. However, this means we are re-reading the file on EVERY violation. This means if a file has 1,000 violations, it will be re-reading the same file 1,000 times.
This will only fix the issue for this one module, and not for underlying issue or issues 3rd parties will likely run into as well. |
@Bananeweizen As I mentioned in #465 , this needs to be a bigger question of the overall flow of things in both repositories. Eclipse-CS is not using the Checkstyle library properly with the lifecycle Checkstyle has built up and defined.
This issue is just another example. IMO, both Eclipse-CS and Checkstyle need to make changes overall to try and accomodate Eclipse-CS fluid lifecycle. Just as a summary, I think Eclipse-CS needs to implement its own Checker as described in #465 , or atleast work with Checkstyle to split classes up for the different lifecycles we both have. Eclipse-CS version needs to implement some kind of "reset" or caching mechanism between runs. This mechanism would be embedded in your own Checker like Checkstyle does with its own. Then it can control the flow of modules and filters, like this one, and call a reset when appropriate to avoid caching issues. Checkstyle would have to move code around so modules can be reset and such. Until we start supporting a real lifecycle in Eclipse-CS, things will just get worse and need more "hacks". |
When using 10.10.0 from #587, the new filter generally works. However, if a file has already been analyzed once by EclipseCS, then adding or removing such a comment and saving the file will NOT lead to a change. Only after purging the cache (ctrl-3, purge) and saving again the added/removed comment will have an effect. Unclear whether that is main checkstyle or us.
The text was updated successfully, but these errors were encountered: