[java] Fix #2996 - CommentSize suppression

[oowekyala]

Describe the PR

Suppressing violations of comment related rules doesn't work well right now, because they are reported against the compilation unit node. This adds some utilities to be able to report violations on a specific token. The RuleContext finds the closest node to the token and uses that to look for suppression annotations.

See also #2996 (comment)

Draft because probably other comment rules have the same problem.

Related issues

• Fixes [java] CommentSize rule violation is not suppressed at method level #2996

Ready?

• Added unit tests for fixed bug/feature
• Passing all unit tests
• Complete build ./mvnw clean verify passes (checked automatically by github actions)
• Added (in-code) documentation (if needed)

[pmd-test]

	1 Message
📖	Compared to master: This changeset changes 0 violations, introduces 0 new violations, 0 new errors and 0 new configuration errors, removes 0 violations, 0 errors and 0 configuration errors. Download full report as build artifact

Generated by 🚫 Danger

[oowekyala]

One problem I have with this is the unreadability of these overloads... Maybe this API could have a makeover? Maybe we should write it like that:

ctx.report(at(node), fmt("msg", a, b))
ctx.report(at(token, astInfo, loc), msg, a, b)

// or

ctx.at(node).warnWithDefaultMsg();
ctx.at(node).warnWithDefaultMsg(arg1, arg2);
ctx.at(token, astInfo, loc).warnWithDefaultMsg();
ctx.at(token, astInfo, loc).warnWithDefaultMsg();
ctx.at(token, astInfo, loc).warn("Custom msg {0}", arg);

We could also probably dispense with the token and astInfo parameters with some magic (store the AstInfo in the RuleContext) and get

ctx.at(loc).warn(...);

But apparently changing the construction of the RuleContext would require changing a bunch of test code... Just a small hurdle though if this is what we want.

@jsotuyod @adangel any thoughts?

[adangel]

Maybe we could implement it with a builder? Which could look like (similar to your suggestions):

ctx.newViolation(message).withArgs(arg1, arg2).at(node);
ctx.newViolation(message).withArgs(arg1, arg2).at(token, astInfo);

In this example, at(Reportable) would actually add the ruleviolation to the rule context. We could also have a separate, explicit build method, which would allow to refine the location:

ctx.newViolation(message).withArgs(arg1, arg2).at(node).atBeginLine(4).build();

This could be an additional API first, as an alternative to the overloads addViolation...

We could do it slightly different:

new ViolationMessageBuilder().with(message).withArgs(arg).at(node).build(ctx);

Which would be then more separated from RuleContext, but honestly, I like it that the entry point for reporting violations is in rulecontext.

[jsotuyod]

Late to the party, but here are some thoughts:

1. I support the fluent API, I think it's a neat solution. Moreover, it's very flexible looking forward for adding things like [core] Reduce priority levels and add confidence value #4327)
2. I feel shaping up that in this PR would be muddying the waters / delay getting this fixed, I'd recommend a separate PR.
3. Any fluent API needs to be flagged @Experimental, I'd not commit our first iteration to a stable public API.
4. Starting at RuleContext is the cleanest approach, I like it. This is irrespective of wether that is passed down directly or still obtained through asCtx(data) (again, separate issue / PR eventually).
5. I'd have the finalizer method be a verb (not at!), descriptive (ie: warn() or report()) and return void to avoid any ambiguity (I've this ViolationMessage object, how do I report it? what do I do with it now?
6. SLF4J v2 introduced it's own fluent API which may work as reference / inspiration: https://slf4j.org/manual.html#fluent