[core] Violation suppressors#2055
Conversation
adangel
left a comment
There was a problem hiding this comment.
Thanks for the work on this part of the API. I have a few questions:
- Is this PR for java-grammar or for pmd/7.0.x? I think, this is a language agnostic API change... so, you need to switch the branches for this PR.
- I was first thinking: Is Violation suppressors really a API, do we need it? I've seen now, that it is needed for language specific suppressions à la
@SuppressWarnings. So, I agree, it is public API. Then next question: Which part/division of the PMD API does this belong to? Not the one, the rule implementors use, but the one, that language implementors use. Currently the main interfaceViolationSuppressoris in the main pmd package. I think, it should be in some other, more specific sub-package. I don't know, whether we talked about this yet. But currently, too many things are in the main package and I'd like to clean this up eventually. But for this, I still need to build a global understanding of all PMD APIs (at least for me), to see, how everything would fit together. That's what I'm missing unfortunately atm (the "big picture"...). - For most languages, the
DefaultRuleViolationFactoryis good enough. That made me thinking: Do we really need to support customizing this core part of PMD for languages? I have seen now two reasons for language specific customization: (1) suppressors (2) language specific violation classes. The custom suppressors I could also see as part of the language module/language registration interface: We could have similar to LanguageVersionHandler#getXPathHandler a method "getAdditionalSuppressors". For (2), I don't know whether this really makes sense. It seems to be used by Java only with JavaRuleViolations. But pmd-core doesn't know about JavaRuleViolations, it knows mostly about ParametrizedRuleViolations and therefore only the informations from that class can be used in renderers. However, as this seems to have caused a problem, JavaRuleViolation doesn't add new fields, all Java-specific fields are actually in ParametrizedRuleViolations (classname, methodname, variablename, ...) which is a bigger problem... So, what I wanted to say: We should only open up stuff for customization if it is really required. Closing down e.g. the RuleViolationFactory would reduce the API surface that we need to maintain later on. - Adding the "noPmdComments" to the RootNodes is not ideal, but works for the moment. The information will only be available after the parsing is done. Maybe we change the parsing at some point to include all comments as individual nodes in the AST. Not sure.
- There is one feature, we should at least think about, how we could support it: We currently report about suppressed violations, but we don't report about "unused suppression markers" - that is a "//NOPMD" comment, that wasn't used to suppress a rule or a "@SuppressWarnings" that didn't suppress any violation. This seems to require an additional first run through the source to collect the markers and a consolidation phase at the end after the last rule has been executed.
btw.: the build is failing with some compile error:
[ERROR] /home/travis/build/pmd/pmd/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/impl/antlr4/AntlrRuleViolationFactory.java:[11,37] cannot find symbol
symbol: class AbstractRuleViolationFactory
location: package net.sourceforge.pmd.lang.rule
[ERROR] /home/travis/build/pmd/pmd/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/impl/antlr4/AntlrRuleViolationFactory.java:[15,54] cannot find symbol
symbol: class AbstractRuleViolationFactory
[ERROR] /home/travis/build/pmd/pmd/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/impl/antlr4/AntlrRuleViolationFactory.java:[16,57] incompatible types: net.sourceforge.pmd.lang.ast.impl.antlr4.AntlrRuleViolationFactory cannot be converted to net.sourceforge.pmd.lang.rule.RuleViolationFactory
[ERROR] /home/travis/build/pmd/pmd/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/impl/antlr4/AntlrRuleViolationFactory.java:[21,5] method does not override or implement a method from a supertype
[ERROR] /home/travis/build/pmd/pmd/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/impl/antlr4/AntlrRuleViolationFactory.java:[27,5] method does not override or implement a method from a supertype
You're right it is.. I need to rework the branch a bit because I built it up on top of #1927, though as seen in the Apex module it isn't really necessary Also I messed up the branch and this contains changes making parser support classes package-private (the branch started about that, and I forgot to branch away when shifting focus). So maybe:
I think that's true. In fact, I think we could package those special APIs into So, for a package
I think this packaging scheme would help keep root packages clean of implementation details and help navigate the API. This is what I've started doing when cleaning up the packaging of the antlr adapters in b51c02f (part of #1796), eg Wdyt?
I think that would be better yes. There is no point in having language-specific violation classes either. This is what I hinted at in the "future" section of the PR description. I think the data holder described on the wiki could be used to make rule violation extensible by any language module, and allow us to remove those language specific methods on the RuleViolation interface. Excerpt from the wiki because it's not in a section so I can't link:
There's much more to be said about how to improve our extension model but it's not the main topic of the PR
Good idea! This looks like some kind of "meta rule" actually. |
This reverts commit 111c0fd08ba479f2b4b7f0321071d1acce8df4fd.
55fea6a to
cd79d64
Compare
Sounds good.
Yes, this package scheme looks good. We need to define however, what
Yes, that could be possible. Although we create a "Meta-API" then between a language and a renderer...e.g. the keys need to be known by the renderer so that it can display the method name and so on. Since renderers are language independent, the DataKeys need to be as well. So we might end up "optional" common keys, and renderers can display sometimes more information from a rule violation and sometimes not (if the language doesn't provide the info).
exactly
I was thinking, whether this could be part of this ViolationSuppressor interface? Something like |
Generated by 🚫 Danger |
dd143c2 to
3d6f660
Compare
|
Maybe we should do the two deprecations tasks on master before merging this PR, otherwise we might forget them... otherwise I'm fine with merging it. |
30752a4 to
ae8c70b
Compare
See also #1962 (comment)
TODO on master
Future
I'm still not satisfied with the current amount of boilerplate. Violation suppressors are orthogonal from the violation factory (the thing creating the violation), it shouldn't be necessary for a language implementation to extend the class just to provide new suppressors. Similarly, you shouldn't have to extend RuleViolation to add new info to the violation (class name, method name, etc). Implementations don't provide any behavior, they just populate fields in the constructor. An interface ViolationDecorator + a map of properties could be used instead. All that is todo for the future and is not the topic of this PR. I'll describe that on the wiki instead.