Allow 3rd party Check providers to group modules under custom parent module

[rnveach]

Subset of Issue #11604

Our current module identification process is very strict. This PR is to allow that to be loosened so 3rd parties can create their own sub-modules that our current one doesn't support.

Suppose I had created a new walker XmlWalker which extends AbstractFileSetCheck and I wanted it to allow children modules like AbstractXmlCheck. Checkstyle will recognize XmlWalker because of AbstractFileSetCheck but it will not identify anything extending AbstractXmlCheck even if follows the absolute same makeup as AbstractCheck. This is because we only recognize our abstracts (AbstractFileSetCheck, AbstractCheck, etc...) and nothing else that could be a child module. There is no common class for module children. Our packages.xml file will not help here, because again, we still only pull in things that follow our defined class hierarchy.

With this change we will now be able to pick up these new children, as long as they follow basic rules defined by our isCheckstyleModule, and treat them like they are any other module we have for Checkstyle, as long as the parent module will accept them. isCheckstyleModule defines things as: must extend AutomaticBean, can't be abstract, must have a default constructor, and must not be an internal module to us.

Example of project that has custom children modules: https://github.com/rnveach/checkstyle-extras

[rnveach]

Old Idea:

Our current module identification process is very strict. This PR is to allow that to be loosened so 3rd parties can create their own sub-modules that our current one doesn't support.

To be sure to note, this is not changing isValidCheckstyleClass, which ensures classes we look at are AutomaticBean, not abstract, and a default constructor. That rule still must pass for all classes and cannot be overridden.

DEFAULT_IDENTIFICATION_PREDICATE shows our current identification process. This identification will allow 3rd party modules, but it will not support 3rd party children modules, IE: new Walker children.

Suppose I had created a new walker XmlWalker which extends AbstractFileSetCheck and I wanted it to allow children modules like AbstractXmlCheck. Checkstyle will recognize XmlWalker because of AbstractFileSetCheck but it will not identify anything extending AbstractXmlCheck even if follows the absolute same makeup as AbstractCheck. This is because we only recognize AbstractCheck and nothing else that could be a child module. There is no common class for module children. Our packages.xml file will not help here, because again, we still only pull in things that follow our defined class hierarchy.

With this new recognition, PackageObjectFactory will now be able to pick up these new children, once the new identification is added and treat them like they are any other module we have for Checkstyle.

Reset is more for testing to show it works and not override the default in all areas.

---

@romani You have any thoughts on this PR?

[romani]

Let me find time to look at it closely

[rnveach]

@romani ping

[rnveach]

CI is passing here.
https://github.com/checkstyle/checkstyle/actions/runs/3262167551#artifacts
No reported checker differences.

[romani]

@rnveach,
I do not like our override of static code.
It is static only because now it can be, but it doesn't mean that we can't convert to instantiated code.

Can we provide identification function to ctor of

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/PackageObjectFactory.java

Line 122 in a8f0657

public PackageObjectFactory(Set<String> packageNames, ClassLoader moduleClassLoader) {

Or some other place.

[rnveach]

Your limited in where this logic can go as PackageObjectFactory#createModule is called by the CLI (roots), checker (file sets), and treewalker (abstract checks)
The best way to make this non-static is to remove the logic completely of what classes we pull in and move this to the respective parents to handle.
If I call createModule with Joe, then it will return a class it finds for Joe even if it doesn't fit what our modules are normally. It is up to Checker/TreeWalker/etc to determine if that is an acceptable child or not
The only thing we will still force for a class createModule returns is ModuleReflectionUtil#isValidCheckstyleClass. That is: extends AutomaticBean, not abstract, default constructor, and not our XpathFileGenerator
So, for example, if createModule returns an AbstractCheck and Checker is the caller, it would up to Checker to determine if it will accept the class type is returns

The only concern here is that basically createModule will return the first class it finds that seems valid. It won't search for another if the caller class doesn't accept the returning type. This should only really happen on clashing class names but defining the package will fix this.
The other way like I assume you are thinking, is to pass some type of lambda for it to assist with the acceptance.

[romani]

item:

[rnveach]

Not understanding why codecov is failing as UI is not reporting any decreases I found.

https://codecov.io/gh/checkstyle/checkstyle/pull/11644

99.42% (-0.01%) compared to 3bbdf40

Edit:
This looks like a false positive. Ran it on my local and only meta and gui packages report coverage not 100%, none of which I changed.

[romani]

@rnveach , please update PR description to not mention old ideas (we can move them to comments to preserve ).

Please use PR number in commit, as issue is like poroject umbrella, too much different updates. I think titile of PR is good, so users might notice it in release notes.
Should this be "new feature" or "bug fix" ?

[rnveach]

@romani

please update PR description to not mention old ideas

Done.

Please use PR number in commit

Done.

Should this be "new feature" or "bug fix"

I am thinking new feature, as we never really considered allowing these type of sub-modules. We may have had discussions and code changes implying this, but it was never out right said or probably documented.

[romani]

We need to improve title of PR to be more easy to understand in release notes.

Maybe later on update PR description to put link to xml linter project as example.
Maybe good to update some doc page on how to do custom parent module.

[nrmancuso]

Maybe good to update some doc page on how to do custom parent module.

Agreed, if we do all this work at allow Checkstyle to be more extensible, but do not have some documentation/ recommendation/ examples about how to do it, the value of resolving #11604 to users is probably low.

@rnveach can we add a healthy documentation update to tasks in #11604 ?

[rnveach]

"healthy" is a strong word. Truthfully this PR is the first one that actually does something for 3rd parties.

I can add documentation, but what are our thoughts regarding it?

Are we requesting a blurb on https://checkstyle.org/writingchecks.html with something titled like "How to incorporate sub-checks"?

[romani]

I recommend to merge, let @rnveach use it in separate project, and then write doc.

[nrmancuso]

I can add documentation, but what are our thoughts regarding it?

I do not know yet, hopefully experience from https://github.com/rnveach/checkstyle-extras will tell :)

I recommend to merge, let @rnveach use it in separate project, and then write doc.

@romani see #11644 (review), I would just like to have documentation update task added to list at #11604. I agree that doc update should wait until checkstyle-extras is closer to completion.

[romani]

@rnveach , please and sub task to big issue and let's move further

[rnveach]

have documentation update task added to list

Done.

[nrmancuso]

Nice side effect of "killing" seven mutations too :)