Support multi-file validation FileSet Modules in Checkstyle

[romani]

Right now Checkstyle is single-file validation tool, with cache that heavily depends on this design pattern.

We need to find solution to support multi-file validation Checks.

[GitToasterhub]

Oh.This issue is quite interesting and challenging.What is interesting for me,can we consider this issue like an idea for GSOC?

[romani]

@GitToasterhub , please stay off issues that do not have "approved" label.
This issue is very complicated , there is no even good idea on how to make it from admins of project.

GSoC have only doable and clear tasks that has to be done and merged to repo on the moment of GSoC completion. List of GSoC projects will not be changed.

[romani]

additional problem is multi file support is that when all violations are collected from file and put them to loggers. in case of XML logger, it opens and close file tag. So reporting violation from multi-file Checks might also to the same files so in XML we will have multiple tags for the same file. Order of violations in Default logger might also be unsorted, there will be grouped by file violations and then random order of violations from multi-file Checks.

but it works like this for many many probably it is not a problem at all, it is like this for 10 years - https://github.com/checkstyle/checkstyle/blame/1cc3442facbbe8e62806996a4bacd92e809ff9a9/src/main/java/com/puppycrawl/tools/checkstyle/Checker.java#L293.

[rnveach]

reporting violation from multi-file Checks might also to the same files so in XML we will have multiple tags for the same file.
they will be grouped by file violations and then random order of violations from multi-file Checks.

This can be seen for TranslationCheck as it is already doing this.
I have no problems with this random violation order in my own project. All violations need to be corrected in the end anyways.

[romani]

After long thoughts ..... I am ok to extend checkstyle to be multi-file validator, as for now only text processing (no class-path), only for FileSetChecks (TreeWalker should not be changed, and should stay single file module).

some details of current hacks are at #4869 (comment)
all "protected" method should be removed.
We change API of void finishProcessing() to be SortedSet<LocalizedMessage> finishProcessing().

@rnveach , if you are agree, please feel free to make issue as approved.

[rnveach]

#3539 also needs to be solved for full multi-file support, otherwise the cache will either hide violations or create false violations.

only for FileSetChecks (TreeWalker should not be changed, and should stay single file module).

Why not include Checks too? It won't be much to handle them too.

all "protected" method should be removed.
We change API of void finishProcessing() to be SortedSet finishProcessing().

As a multi-file module, it is the job the FileSet/Check to call fireFileStarted and fireFileFinished as only it knows what files it is processing again. Without AbstractFileSetCheck.fireErrors, the current log implementation doesn't know which file it is working with. To remove it, LocalizedMessage would have to include the file name, and Checker would have to do the fire calls itself and group the messages by file name.

[romani]

otherwise the cache will either hide violations or create false violations.

I posted proposal for solution in issue #3539

Why not include Checks too?

lets grow functionality step by step. For now we need only FileSet. Multifile mode force API to change. Lets change it on this level first and see what other conner case we forgot.

LocalizedMessage would have to include the file name

yes, this requirement as we converting Checkstyle to multi-file mode so violation should have file on which is fired.

and Checker would have to do the fire calls itself and group the messages by file name.

Checker already do

final SortedSet<LocalizedMessage> fileMessages = processFile(file);
fireErrors(fileName, fileMessages);

so we will do:

final SortedSet<LocalizedMessage> messages =
       fileSetChecks.forEach(FileSetCheck::finishProcessing);
fireErrors(messages);

no more need for setMessageDispatcher(). Check return all to Checker, Checker give collection of violations to AuditListener, AuditListener base on LocallizedMessage object to proper reporting. Grouping will not be possible and all violations on file might be already flushed to stream.

@rnveach am I missing smth ?

[rnveach]

No that is everything.

Lets change it on this level first and see what other conner case we forgot

This is similar to how I have implemented and there hasn't been any issues. finishProcessing is what all modules need to implement multi-file support.

[romani]

finishProcessing is what all modules need to implement multi-file support.

for FileSet Check yes.
For Java checks ... I would rather postpone this support a bit and collect user storied on what validations they would like to make.

---

if you agree, please label issue as approved.

[rnveach]

for FileSet Check yes. For Java checks ...

I was referring to both. Java is no different then non-Java.

[romani]

This issue is blocked by cache usage discussion at #3539.
As we agree on cache role for single-file Checks and multi-file Checks, we can proceed with this issue.

[rnveach]

Adding support will need to handle exceptions properly as seen in #6340 (comment) . Checker should handle exceptions itself like it does for non-multi-file checks and not require the checks to change them into violations. Halt on exception should be looked into for this too.