Contributing - How does suppressions work

[blundell]

I have an idea for a new feature and wanted to help. Can someone point out where to start

Feature idea:

When someone uses @SuppressWarnings("PMD.GodClass") (or other suppression) it will ignore the warning of god class. However I wanted to add a new PMD warning that will warn you if you are suppressing a non existant warning! Therefore if the class is no longer a GodClass, this new PMD warnng would tell you "you are supressing GodClass but this class/method/line of code does not cause a GodClass violation."

i.e. if someone does @SuppressWarning("PMD") but the class doesn't have any PMD warnings any way - the rule will fail. (or maybe its not a rule, but at least show this in logs or whatever).

Any pointers?

[blundell]

It can be called UnnecessarySupressionRule

[blundell]

I went for something like this:

public class UnnecessarySuppressionRule extends AbstractJavaRule  {

    @Override
    protected void visitAll(List<? extends Node> nodes, RuleContext ctx) {
        for (Node node : nodes) {
           // CHECK IF THE NODE HAS A SUPPRESSION
            if (!isSuppressed(node)) {
                continue;
            }

            // IF IT HAS A SUPRESSION - CHECK IF ANY RULES HAVE BEEN VIOLATED
            Report report = ctx.getReport();
            List<Report.SuppressedViolation> suppressedRuleViolations = report.getSuppressedRuleViolations();
            for (Report.SuppressedViolation suppressedViolation : suppressedRuleViolations) {
                boolean violatedRule = isRuleViolated(node, suppressedViolation);
                if(violatedRule) {
                    return;
                }
            }

            // IF NO RULES HAVE BEEN VIOLATED THEN IT IS AN UNNECCESSARY SUPPRESSION
            String ruleName = "TODO";
            String message = "You are suppressing " + ruleName
                    + " in " + node.getImage() + ", line:" + node.getBeginLine()
                    + " but this class/method/line of code does not cause a "
                    + ruleName + " violation. You should remove the suppression!";
            addViolationWithMessage(ctx, node, message);
        }
    }

    private boolean isRuleViolated(Node node, Report.SuppressedViolation suppressedViolation) {
        RuleViolation ruleViolation = suppressedViolation.getRuleViolation();
        String maybeNodeFilename = node.getImage();
        String ruleFilename = ruleViolation.getFilename();
        int ruleBeginLine = ruleViolation.getBeginLine();
        int ruleBeginColumn = ruleViolation.getBeginColumn();
        int ruleEndColumn = ruleViolation.getEndColumn();
        int ruleEndLine = ruleViolation.getEndLine();

        int nodeBeginLine = node.getBeginLine();
        int nodeBeginColumn = node.getBeginColumn();
        int nodeEndColumn = node.getEndColumn();
        int nodeEndLine = node.getEndLine();

        return ruleBeginLine == nodeBeginLine
                && ruleBeginColumn == nodeBeginColumn
                && ruleEndColumn == nodeEndColumn
                && ruleEndLine == nodeEndLine;
    }
}

It doesn't work.
Because I don't think I understand how to check for rule violations.
Two things:
1) How can I check if the node I am visiting has a suppression on it?
2) How can I then check if any of the other rules have already failed for this node?

[Andreas Dangel]

That's a great idea for a feature! I guess, it would be very usefull.

Some comments on the suggested implementation:

• visitAll - this is called once per source file. There is always only one node in the nodes list - it is the ASTCompilationUnit root node. That's why you probably don't catch all suppressions here.
• ctx.getReport() - you won't see the violations/suppressed violations from other rules/files. You might see the current state (some violations) but not the end state. This depends on when the other rules are executed and whether your rule is executed in a different thread or not.

Because of the second point I don't think, that the problem can be solved solely with a rule.

What we need is a complete overview of the suppressions in all analyzed source files and then a post-processing step the removes the matching suppressed violations. All the remaining suppressions should be the ones, that are not needed anymore.
This post processing sounds like, it could be integrated somehow into a report? Currently, a report collects not only the violations but also the suppressed violations. If it would have the information about all the existing suppressions, it could calculate the useless suppressions.

So, we could create a rule, which visits all nodes and collects the node in case it is suppressed - similary like a call to "addViolation" - but I would call it differently - something like "addSuppression" or so. This means, that the Report class needs to be extended to collect this (similarly to violations, errors, metrics, etc.).

At this point, a report formatter/renderer could use the information to calculate the useless suppressions.

I'm not sure yet, where to place this logic. It would be nice, to have a real violation reported. Each formatter/renderer could add a "fake" violation if some useless suppressions are left - to help staying compatible with upstream tools. But it would be nicer to have a definitive step in the processing for this. There is a start/stop hook (How to write a rule) but I don't know of a hook, that would be executed after all files have been analyzed and just before the result would be rendered. Looks like something that needs to be added.

[blundell]

Great thanks for the input!

I'm working on this when I find time.

I've managed to visit all nodes and collect those that are suppressed.
I've added these to the report.

Next step like you say is to get this to be rendered. I'm currently exploring what it means to be rendered / trying to work out how the report is used to show violations.

The code is on this branch here, just check the commit history: https://github.com/novoda/pmd/tree/unneccessary_suppression

btw anyone reading this feel free to contribute if you want :-)