Menu
▾
▴
#377 RegexpHeader and SuppressionCommentFilter not behaving as CLI does
With the new year comes a new copyright header from our legal dept. RegexpHeader check is not easily adapted to check for new required header but not produce false positive on unedited source file that contains old header comment. To deal with this, I added a SuppressionCommentFilter (or SuppressWithNearbyCommentFilter) that looks for the old header comment.
The combination of RegexpHeader check and comment suppression is working in CLI Checkstyle. The Eclipse plug-in is showing warnings for some files upon initial build. If I rebuild, lots more warnings show up. The fact that the warning shows for some files and not others makes me wonder if it is an order of operations issue.
Issue was first seen in Checkstyle 5.8 plug-in, but persists in 6.1.1.
Ok, I've tracked down the cause of this problem - unfortunately it's caused by a subtle issue which appears due to the Checkstyle plugin caching and reusing the (costly to obtain) Checkstyle configuration in-memory structure - that is the configuration object Checkstyle builds internally from the configuration file.
Due to some caching logic in the Treewalker module this module skips all files it has already checked once (by file name and last modified stamp). Thus the FileContentsHolder module (a child of Treewalker and necessary to make current file contents available to the SuppressionCommentFilter) is not being populated properly anymore. Because of this the SuppressionCommentFilter cannot suppress the warning anymore.
Unfortunately Checkstyle core right now offers no API to reset the internal Treewalker cache - and on the other side I am not prepared to remove the plugins internal configuration cache/reuse, because that would severely affect performance (imagine the Checkstyle configuration object would need to be obtained every time a Java file is saved and hence an incremental build triggered).
The CLI does not exhibit this problem since each run is a fresh JVM with a fresh Checkstyle configuration object loaded from the configuration file.
However, in the plugin there might be a way to work around this issue since it (probably) never appears on incremental builds, since in this case the file(s) in question have been touched and received a new last modified timestamp.
Full builds - where the problem appears prominently as per my own testing - are less performance critical, here I likely could get away with dropping the cached configuration object and obtaining it anew. without anyone noticing...
Lars, thanks for taking on this ticket so quickly and for the detailed explanation. After reading what is going on I think I finally understand why some of our other comment suppressions would inexplicably fail from time to time.
Would selecting 'Check Code with Checkstyle' from the context menu work the same as the full build case you mentioned?
Yes, for the time being "Check Code with Checkstyle" would also NOT reuse the cached configuration, since it cannot be guaranteed that any or all files had been modified in between.
I've filed the issue over at the Checkstyle core project, where it will be hopefully be fixed for the next version. In this case I will revert the workaround introduced with this fix.
For reference: https://github.com/checkstyle/checkstyle/issues/569