[core] Fix #5039: Refactor RuleContext to use builder pattern#6260
[core] Fix #5039: Refactor RuleContext to use builder pattern#6260oowekyala wants to merge 28 commits into
Conversation
This will replace the many `addViolation*` methods with a simpler set of orthogonal methods. - First specify the location. - Then specify the message/format arguments. Fix pmd#5039
This has been a TODO for a long time. Compared to automatically finding the suppression node, this saves time, as we don't have to explore the whole tree when reporting something.
parse default message ahead of time
|
Compared to main: (comment created at 2026-03-04 13:34:46+00:00 for 621cb8b) |
Header comments are not considered part of the root node...
adangel
left a comment
There was a problem hiding this comment.
Thanks, I like the new API (almost 😄 )
I would really name the verb report() instead of warn().
If we introduce the new API with the violation builder without marking it Experimental (like it is right now in this PR), then I'd deprecate all the old methods addViolation... right now. No need to wait, because we have then already a stable alternative API.
I've created an "API Changes" section in the description of this PR to keep track of the changes for the release notes so far.
We also need to update the documentation then on how to report a violation: https://docs.pmd-code.org/latest/pmd_userdocs_extending_writing_java_rules.html#reporting-violations
|
|
||
| /** Marker annotation for Intellij inspection to warn on unused return value. */ | ||
| @Documented | ||
| @interface CheckReturnValue { |
There was a problem hiding this comment.
We probably should move this annotation to net.sourceforge.pmd.annotation.
There was a problem hiding this comment.
Or did you mean to use com.google.errorprone.annotations.CheckReturnValue? We get it currently transitively via com.google.code.gson:gson, but if we use it ourselves, we should at a explicit dependency.
There was a problem hiding this comment.
The intellij inspection handles any annotation called CheckReturnValue. It doesn't even need to be public (which is why I kept this annotation package private). If we really want to use a well-known annotation then I would suggest using jetbrains annotations instead, because that package is small and would give us @NotNull/@Nullable (#6134).
Are you ok with doing this in a companion PR? I would prefer this PR to be only about the new API and the other being just mechanical translation. |
Describe the PR
Add new API to RuleContext to report violations. The goal is to replace the set of
addViolation*methods with a more flexible 2-stage builder pattern. This allows specifying the violation location and the message parameters independently and increases the readability and usability of this API.I removed the more complex overloads already in this PR (those that were marked
@Experimental) and replace their usages. Should we deprecate the simpler methods now, later, never? I think we should do so, but maybe in a separate PR.API Changes
New Experimental APIs
Removed Experimental APIs
Deprecated APIs
Related issues
Ready?
./mvnw clean verifypasses (checked automatically by github actions)