Issue #11604: allow 3rd party checks to print their own custom asts

[rnveach]

Issue #11604

Currently 3rd parties have no way to provide their own AST to do some of the basics that Checkstyle does like printing the ASTs for users to understand it.

This provides a new option in the CLI so custom printers can be used. Other printers were very slightly reworked to this new method to show its usage.

It is hoped to use this method for other CLI functionality like printXpathBranch and printSuppressions.

[pbludov]

Couple of minors

[pbludov]

No need to catch CheckstyleException here. It just wraps one CheckstyleException with another:

com.puppycrawl.tools.checkstyle.api.CheckstyleException: Unable to work with custom tree printer
	at com.puppycrawl.tools.checkstyle.Main.runCli(Main.java:308)
	at com.puppycrawl.tools.checkstyle.Main.execute(Main.java:197)
	at com.puppycrawl.tools.checkstyle.Main.main(Main.java:132)
Caused by: com.puppycrawl.tools.checkstyle.api.CheckstyleException: Unable to work with custom tree printer
	at com.puppycrawl.tools.checkstyle.Main.callTreeStringPrinter(Main.java:360)
	at com.puppycrawl.tools.checkstyle.Main.runCli(Main.java:304)
	... 2 more

[rnveach]

Done in the next push.

[rnveach]

This wording was identified as out of date and confusing for options where -t is not used.

Examples:

PS M:\> java -jar .\checkstyle-10.5.0-all.jar -s out -t .
Option '-t' cannot be used with other options.
PS M:\> java -jar .\checkstyle-10.5.0-all.jar -s out -T .
Option '-t' cannot be used with other options.
PS M:\> java -jar .\checkstyle-10.5.0-all.jar -s out -J .
Option '-t' cannot be used with other options.

[nrmancuso]

What alternatives have been considered to solve this problem? Extension of CLI/API like this deserves it's own issue/discussion IMO. Won't we need to add "alternative" options for others, such as -g, -s, and -b?

[rnveach]

Won't we need to add "alternative" options for others, such as -g, -s, and -b?

See the end of my first post at #12510 (comment) .
It is hoped to use this method for other CLI functionality like printXpathBranch and printSuppressions.

I was planning things in future PRs after this one.

deserves it's own issue

I can create the issue if that is what is needed.

What alternatives have been considered to solve this problem?

There is no alternative in Checkstyle as we don't allow any extension of the CLI. If this PR isn't acceptable, then there isn't a lot of good options.

3rd parties could create a hack of a check who's only function is to print the tree of the file it is given, which would require a configuration file for nothing more than specifying the tree printer which I am passing in as an argument in this PR.

Other than that, 3rd parties will have to duplicate Checkstyle's CLI as they will need the same options (plus their own) if we were to allow them to use checkstyle/contribution#604 .

[nrmancuso]

There is no alternative in Checkstyle as we don't allow any extension of the CLI

Is allowing extension of CLI another option? Then people that want to extend CLI functionality can do whatever they want, and not be bothered by activities in main project.

[rnveach]

Is allowing extension of CLI another option?

I am open to this option, but I am not fully seeing how to be honest.

Any extension is based picocli allowing this as it is the main controller of the CLI and probably some overridable methods to allow such custom execution.

https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/Main.java#L629
I am going to assume first this would have to be made a public class so 3rd parties can extend it. I am not sure if picocli will accept that.

https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/Main.java#L113-L114
Somehow this has to be overridable that 3rd parties can provide their new CLI instance with their new options.

https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/Main.java#L188
https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/Main.java#L195
Then we would need to override these remaining methods for the main logic and ensure they stay of main repo's way if a 3rd party option is provided. runCheckstyle method might also have to be made protected so 3rd parties can call it.

If this is possible, I am seeing this require main repo make classes public and methods protected to be able to do an extends on it.

And even if we do this, things like generateXpathSuppressionsFile will probably not be possible without even more hacking entry points. The prints are easiest to integrate hence why I started this PR with them. It may be possible if that section of code was method'ed off to the side.

[nrmancuso]

If we want to inject dependencies, we could do something like:

public class Main {
    
    CliOptions clioptions; // injected, can read concrete class name from property file, bean, etc.
    CliRunner cliRunner; // injected, replaces `runCli` and other functionality, also can specify class via property file, bean, etc.

    public static void main(String... args){
        Main m = new Main();
        m.run(args);
    }

    private void run(String... args) {
        int e = cliRunner.execute(cliOptions);
        Runtime.getRuntime().exit(e);
    }
...

[strkkk]

I dont like this design, to be honest.

method name like printFileAst assumes that

1. there is no return, similar to System.out.println() and similar methods.
2. Result is the AST, which is weird since return type is String, which means any text.

Plus, name of the interface is confusing, since even existing implementations dont do any printing of the AST, they just convert it into string. Printing is performed outside, in Main.java line 369.

Therefore I suggest to rename it to something like FileConverter (or something like this) with meaningful method name.

[rnveach]

@nrmancuso @strkkk @pbludov
Is there any agreement we can make on design before I continue? @nrmancuso Was mentioning extension over something being built into the project. For @strkkk 's comment, everything was originally printFileAst so thats why I went with this, but if we head down extension, then naming becomes not an issue as 3rd parties will be the ones to name it themselves.

[nrmancuso]

Can we make

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

Line 629 in a8f0657

private static class CliOptions {

a public class to allow extension of options? There is bound to be more things that some third party may need to do with the CLI, and this way they would not be blocked by limitations of checksytle CLI.

[romani]

I do not like this extension of our API, i think it should be done completely in thirdparty projects such custom support.

[romani]

no more ideas or points to reconcile disfigurements.
I am closing this PR.
We will focus in our api on what we need, thirparty projects can clone us and hack us as they want to get what they need, we will not make it as our contract to support.
We already have bunch of implementations and other unfortunate decisions in our API we need to focus on our needs only.

[romani]

can we use this property and print tree in format thatwe need at #14631 ?