new Checker filter SuppressWithPlainTextCommentFilter as akin to Treewalker's SuppressionCommentFilter

[PhantomThief]

Same as #4884

cat Test.java

class Test {
  // CHECKSTYLE:OFF
  //	has tab here
}

cat config.xml

<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
    "-//Puppy Crawl//DTD Check Configuration 1.2//EN"
    "http://www.puppycrawl.com/dtds/configuration_1_2.dtd">

<module name="Checker">

    <module name="FileTabCharacter">
        <property name="eachLine" value="true"/>
    </module>

    <module name="TreeWalker">

        <module name="SuppressionCommentFilter"/>

        <module name="SuppressionCommentFilter">
            <property name="offCommentFormat" value="CSOFF\: ([\w\|]+)"/>
            <property name="onCommentFormat" value="CSON\: ([\w\|]+)"/>
            <property name="checkFormat" value="$1"/>
        </module>

    </module>

    <module name="SuppressWarningsFilter"/>

</module>

java -jar checkstyle-8.1-all.jar -c config.xml Test.java

Starting audit...
[ERROR] /***/Test.java:3:5: Line contains a tab character. [FileTabCharacter]
Audit done.
Checkstyle ends with 1 errors.

---

expected no errors because there is a suppression comment filter.

while it works on 8.0

[rnveach]

while it works on 8.0

I couldn't get it to work with 8.0 when everything is corrected but I can see why it won't work with 8.1 anyways.

This is because SuppressionCommentFilter was converted to only be suppressions for AbstractChecks and not an AbstractFileSet in Issue #4714. This was because the filters already needed access to comments which was only retrieved via Java parsing.

I'm not sure there is a decent way to bring this functionality back.
@romani What is your take?

[PhantomThief]

@rnveach
sorry, I forgot. on 8.0, the config should move SuppressionCommentFilter outside TreeWalker section, as there is a breaking change here between 8.0 and 8.1
on 8.0 config is:

<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
    "-//Puppy Crawl//DTD Check Configuration 1.2//EN"
    "http://www.puppycrawl.com/dtds/configuration_1_2.dtd">

<module name="Checker">

    <module name="FileTabCharacter">
        <property name="eachLine" value="true"/>
    </module>

    <module name="TreeWalker">
    </module>

    <module name="SuppressWarningsFilter"/>

    <module name="SuppressionCommentFilter"/>

    <module name="SuppressionCommentFilter">
        <property name="offCommentFormat" value="CSOFF\: ([\w\|]+)"/>
        <property name="onCommentFormat" value="CSON\: ([\w\|]+)"/>
        <property name="checkFormat" value="$1"/>
    </module>

</module>

[romani]

terrible regression ... we forgot about non-java grammar based Checks at all.

[romani]

@rnveach ,

we can not come back to Holders model to keep context between TreeWalker and Checker.
Checker own FileSetChecks - Checks that look at file as plain text (no grammar, no AST, complete freedom and responsibility of own Regular Expressions).
TreeWalker - is for Checks/Filters who care about java grammar.

So we need to introduce new Filter - SuppressWithPlainTextCommentFilter and SuppressWithNearbyPlainTextCommentFilter (if you see better name, please suggest)
This filter will NOT use AST to detect comments, it will use extra "parsing"(loading of file content before violation line and search by regexp of suppress comment). Yes, it will be performance hit in comparison to 7.X versions of checkstyle, but RegExp Checks is already user's desire to do smth close to smart without writing special Check.
Two Filters are required.
Filters might be used in non-java files, for example in property files. Property files do not support // or /* */ comments, but Filter should search for user defined text and should not care about what is located before matched text.

@PhantomThief , @Ricq , please share your ideas too, in case it might now satisfy your use cases.

[rnveach]

I am fine with proposal.

Filter should search for user defined text and should not care about what is located before matched text

If users care, they can add them as part of the text to match.

SuppressWithPlainTextCommentFilter and SuppressWithNearbyPlainTextCommentFilter
Filter should search for user defined text and should not care about what is located before matched text

Since we aren't verifying it is a comment like you specified, why don't we take Comment out of the names?

[romani]

Since we aren't verifying it is a comment like you specified, why don't we take Comment out of the names?

SuppressWithPlainTextFilter and SuppressWithNearbyPlainTextFilter.
Looks a bit strange by names. As we validate some source file, where each symbol mean smth, any extra "CHECKSTYLE OFF" should be in comment of some sort , to be ignored by compiler/loader/interpretor.
So strings like "CHECKSTYLE OFF" will be in comments for sure, so placing "Comment" in name is ok.

@MEZk , please share your ideas on what name do you like more.

[MEZk]

SuppressWithPlainTextCommentFilter and SuppressWithNearbyPlainTextCommentFilter

I'm fine with the names.

@rnveach

any extra "CHECKSTYLE OFF" should be in comment of some sort , to be ignored by compiler/loader/interpretor.

Sounds reasonable. If the filter is for property file, we will need '#' to mark the text as a comment, but still it will be a comment.

@romani @rnveach
SO, the basic idea is to create two filters which implement Filter interface. Both filters must not know anything about AST, javadoc, grammar, etc. Filters should work only with plain source file. Do I understand the issue correctly? If yes, I can provide the implementation.

terrible regression ... we forgot about non-java grammar based Checks at all.

Yes, but now we have two filters in Checkstyle architecture with separate responsibility.

[romani]

If yes, I can provide the implementation.

All right, please send PR.

[Ricq]

The implementation will resolve my issue with using the suppression in combination with RegexpMultiline in Java files, and supports another use case I have, which is to run simple regular expression checks (for headers, etc.) on JavaScript as well.

So, +1 on the fix.

One remark: it might become hard for users to distinguish which suppression filter to use. As the original ones now only apply to Java-specific TreeWalker checks, and the new ones will apply to regular Checker checks such as the RegexpSingleline and RegexpMultiline checks. I suggest creating some clear documentation with cross-references between the two sets to prevent confusion.

[romani]

I suggest creating some clear documentation with cross-references between the two sets to prevent confusion.

documentation will be provided, but as usually nobody will read it. Checkstyle execution will fail if wrong filter is used.

[MEZk]

@romani @rnveach
Questions:

1. Should SuppressWithPlainTextCommentFilter parse comments itself?
2. Should Checker pass FileText to the filter or should the filter receive fileName from the AuditEvent and create FileText itself?
3. I think that

private WeakReference<FileContents> fileContentsReference = new WeakReference<>(null);

should be removed for sure.
4) Should the filter work only with singleline comments:

// CHECKSTYLE:OFF
// CHECKSTYLE:ON

[romani]

Should SuppressWithPlainTextCommentFilter parse comments itself?

It is hard to make it for sure, but for now I think it should not parse comment at all. Just match comment line by regexp.

Should Checker pass FileText to the filter or should the filter receive fileName from the AuditEvent and create FileText itself?

Right now I see that how Checker work with FileSetModules:

            final FileText theText = new FileText(file.getAbsoluteFile(), charset);
            for (final FileSetCheck fsc : fileSetChecks) {
                fileMessages.addAll(fsc.process(file, theText));

so it put FileText in modules, so lets put FileText in Filter too.
But for filters we keep Filter interface as simple as accept only. So all context should go by AuditEvent.
in Treewalker we do this like new TreeWalkerAuditEvent(fileContents, fileName, element); so give filename and object that contain content of file. I think that we should do same and put in AuditEvent FileText object (event it is an API change). Global holders should not be used any more.

should be removed for sure.

ok

Should the filter work only with singleline comments

it should work how it worked before. It had support for multi-line comments. If that is over complication, we can postpone support of multiline-comments for future.

[MEZk]

@romani

it should work how it worked before. It had support for multi-line comments. If that is over complication, we can postpone support of multiline-comments for future.

FileContents has all required logic to get c lang and cpp comments, block comments, etc. All we need to do is to invoke reportSingleLineComment, or reportBlockComment just as GeneratedJavaLexer does. No need to reinvent the wheel and implement logic to match comments (it will be the same as in FIleContents anyway), no need to change the logic (we can copypaste SuppressionCommentFIlter logic). All we need to do is to make the new filters children of Checker and remove all global context. Thus, the filters will get FileText from the AuditEvent, loop over the each line of the file, report comments to FileContents.

@rnveach @romani
What is your opinion on this approach?

[rnveach]

All we need to do is to invoke reportSingleLineComment, or reportBlockComment just as GeneratedJavaLexer does.

But we can't assume file is Java as romani's example at #4841 (comment) was for properties, which don't have C/C++ styled comments.
Also, if we invoke Java grammar a 2nd time it will cause performance problems, and we can't assume TreeWalker will execute if user doesn't add an AbstractCheck module.

[MEZk]

@rnveach

Also, if we invoke Java grammar a 2nd time it will cause performance problems, and we can't assume TreeWalker will execute if user doesn't add an AbstractCheck module.

We do not need TreeWalker execution. Look at these methods:

1. https://github.com/MEZk/checkstyle/blob/66cc9d43c9620678078d2716308ee61916227fcb/src/main/java/com/puppycrawl/tools/checkstyle/api/FileContents.java#L104
2. https://github.com/MEZk/checkstyle/blob/66cc9d43c9620678078d2716308ee61916227fcb/src/main/java/com/puppycrawl/tools/checkstyle/api/FileContents.java#L125

But we can't assume file is Java as romani's example at #4841 (comment) was for properties, which don't have C/C++ styled comments.

Yes, this is the only problem as for me. However, we can implement additional method: reportPropertiesComment which will be invoked when we the line contains #.*.

My main point is to avoid copy-paste of the same logic from FileContents to the filters.

[rnveach]

@MEZk

Look at these methods:

How do you think those methods get called? In the Java grammar...

checkstyle/src/main/resources/com/puppycrawl/tools/checkstyle/grammars/java.g

Line 1747 in 25a37e5

{ mCommentListener.reportSingleLineComment("//", getLine(),

checkstyle/src/main/resources/com/puppycrawl/tools/checkstyle/grammars/java.g

Line 1769 in 25a37e5

mCommentListener.reportBlockComment("/*", startLine, startCol,

reportPropertiesComment

What about other text base files? XML, json, etc...

[MEZk]

@romani
After discussion with @rnveach we decided that

1. User should specify comment regexp pattern. This will allow us to process all types of comments in java, xml, properties files.
2. We should definitely process single line comments in the first implementation in case we have problems with multi-line ones. We need to fix regression ASAP.
3. The filter should not invoke any methods from FileContents. It has to search for the suppression comments on his own line by line. If the line matches comment regexp the filter should pick it and process it as a suppression.

If you are OK with the proposal, let me know.

[romani]

@MEZk,

User should specify comment regexp pattern

User just need to specify String to match, user has to put comment symbols himself to regexp. All we will do is match some string above violation by user defined regexp. We should forget about term "comment" at all.

We should definitely process single line comments in the first implementation in case we have problems with multi-line ones

I think drop of support of multiline comments will be ok.
As we will suppress by some line match, we will not know is it comment of some type or not.

The filter should not invoke any methods from FileContents

Yes

If you are OK with the proposal, let me know.

Please be welcome with PR.

[Starwind0]

Can someone link the documents for this feature mentioned (some of us do in fact read them). This is assuming that this fix was in fact implemented.

[rnveach]

assuming that this fix was in fact implemented.

This fix has not been implemented hence why it is still open.

[Starwind0]

Hmm okay. I only pressed as after I updated android studio to 3.0 b7 and the intellij / android studio check style plugin to 5.10.2 I am no longer able to import my rules citing the error that appears in the newest checkstyles. That is instead of the current selected version of checkstyles. Which had me hoping that this had been at last fixed. So I will just downgrade the plugins instead.

[romani]

@MEZk , ping.

[MEZk]

@romani
Sorry for delay. Working on the problem.

Created SuppressWithPlainTextCommentFilter implementation, working on the documentation.

[MEZk]

@romani
PR which adds SuppressWithPlainTextComment was sent, but Travis CI failed the build due to the problem with localized tests.

[rnveach]

Fix was merged.