SuppressWithNearbyTextFilter only changes after purging cache

[Bananeweizen]

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.

[rnveach]

Only after purging the cache (ctrl-3, purge) and saving again the added/removed comment will have an effect

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 ) .

[Bananeweizen]

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:

• In eclipse-cs identify the "start a new scan" lifecycle point and reset the field in the filter via reflection. Not nice, but scope of change is this repo only then.
• in the checkstyle main project, change the filter implementation. Check not only whether the file is still the same, but also read the filetext again and compare that to a cached version. Compared to now, repeated access to the file would not be avoided anymore, but repeated searching of the contained suppressions still would be avoided. I would assume that this change would not affect performance much, assuming the file cache can return the (almost always unchanged) file quickly.

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.

@rnveach @Calixte Opinions?

[rnveach]

Check not only whether the file is still the same, but also read the filetext again and compare that to a cached version

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.

I currently tend to do the reflection based reset.

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.

[rnveach]

@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.

Checker assumes the instance is always re-created on a run, so Modules are never set to reload files they have loaded after the first time.

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".