[core] Simplify the rulechain #2490
Conversation
Change behavior of LoosePackageCoupling It stores state between visits, should not use the rulechain
Generated by 🚫 Danger |
There was a problem hiding this comment.
Thanks for this, looks like a lot of work! Please don't take my comments personal, I'm just trying to understand what you did... (you have definitively lost me with the LatticeRelation ...).
I'll try to merge this now...
Rulesets. This is an implementation detail
I've done the deprecations, see 91868f3
We probably need to look at RuleSetFactory as well, there are a couple of usages of RuleSets... But that's a separate task.
Port remaining usages of addRuleChainVisit
That's a follow up PR.
Benchmark API (internal)
What do you mean with that? Benchmark is currently possible to be called from CLI (-benchmark), so we need to define, what exactly the API is...
But I guess, this can be done later as well, as I didn't see, that you modified the benchmark classes here.
.../src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/AvoidLogicInTriggerRule.java
Show resolved
Hide resolved
| * @param nodeClass | ||
| * the AST node to add to the RuleChain visit list | ||
| */ | ||
| void addRuleChainVisit(Class<? extends Node> nodeClass); |
There was a problem hiding this comment.
All this can't be deprecated on master, since there is no way to migrate before. I've added a point to #1139 for the migration documentation.
When we have finished the changes on PMD 7, we can go back and look, whether we introduce e.g. RuleTargetSelector already in PMD 6.x and providing a compatibility API - that would allow us to deprecate this method in PMD 6.x. But - if we do that - only as a last step and maybe only as a small minor release for PMD 6.x after PMD 7 is out (for helping migration).
| import net.sourceforge.pmd.lang.ast.Node; | ||
| import net.sourceforge.pmd.lang.rule.internal.RuleApplicator; | ||
|
|
||
| /** | ||
| * Grouping of Rules per Language in a RuleSet. |
There was a problem hiding this comment.
That's btw something we need to think about: The comment probably was the intention, but was never implemented this way - all the rules are put together into the same list regardless of their language....
So, how do we deal with a ruleset that contains rules of multiple languages, etc.... (in terms of internal processing and expected unsurprising behavior of PMD).
| import net.sourceforge.pmd.lang.ast.Node; | ||
| import net.sourceforge.pmd.lang.rule.internal.RuleApplicator; | ||
|
|
||
| /** | ||
| * Grouping of Rules per Language in a RuleSet. | ||
| * | ||
| * @author pieter_van_raemdonck - Application Engineers NV/SA - www.ae.be | ||
| */ | ||
| public class RuleSets { |
There was a problem hiding this comment.
I'll @Deprecate and @InternalApi it on master....
Note: there are quite some usages in the PMD Eclipse plugin. This is due to the unclear API on how to call PMD programmatically, e.g. how to provide programmatically the rulesets to be used for analyzing....
| * | ||
| * @see Rule#getTargetSelector() | ||
| */ | ||
| public abstract class RuleTargetSelector extends TargetSelectorInternal { |
There was a problem hiding this comment.
This looks a bit weird... RuleTargetSelector is API, but the supertype is internal.... effectively, this makes the internal supertype API as well
Ok, the class is abstract and the constructor is hidden - so no one can implement their own RuleTargetSelector...
Right now, RuleTargetSelector serves as a utility class, that provides defined (hidden) implementations of itself...
Not sure yet, what to do here, but it still looks weird to me....
There was a problem hiding this comment.
Yes this is super weird. I wanted to make RuleTargetSelector published because it's somewhere on the Rule interface, but still keep everything about TreeIndex internal... So no one can implement their RuleTargetSelector because that would require us to expose TreeIndex.
I'm truly not sure this is the best way to do it, if you have a better idea go ahead
| import net.sourceforge.pmd.lang.rule.internal.GraphUtils.DotColor; | ||
|
|
||
| /** | ||
| * Indexes data of type {@code <V>} with keys of type {@code <K>}, where |
There was a problem hiding this comment.
Just a question: Do we really need to be that generic? We are dealing with AST Nodes, don't we?
That's btw one example for #2611 - it will be very difficult to understand this class without looking at many more classes in parallel to understand, what's going on....
Maybe the question in a different way: Do we really implement it that abstract? We only need it for PMD purposes, not for general purpose....
There was a problem hiding this comment.
Ok, as far as I understand, that is your solution for dealing with Abstract node types.... right?
There was a problem hiding this comment.
Ok, as far as I understand, that is your solution for dealing with Abstract node types.... right?
Yes that's it.
Honestly making it generic is easy and makes tests much more comprehensive and easier to write. This data structure is a kind of multimap where values are added at once to several keys, doing minimal work. So we can add Node values to several Class keys at once, just by knowing node.getClass(), without having to traverse all the supertypes for every instance. We also don't accumulate values for keys that won't be queried (those Class instances for which no rulechain rule has added a visit request).
The API is really simple I think, you just use put and get as if it was a regular map.
| * Map each element of the given iterable with the given function, | ||
| * and accumulates it into the collector. | ||
| */ | ||
| public static <T, U, A, C> C map(Collector<? super U, A, ? extends C> collector, |
There was a problem hiding this comment.
Another example of overly used generics... What is T, U, A ,C?... Maybe I'm just not that into streams...
There was a problem hiding this comment.
I think this is just "used generics" rather than overused... It's not my fault that the Collector API uses 3 type parameters...
U, A, C in that order correspond to the T, A, R type parameters of Collector:
T - the type of input elements to the reduction operation
A - the mutable accumulation type of the reduction operation (often hidden as an implementation detail)
R - the result type of the reduction operation
On this method we have a source of T elements, a mapper from T to U, and a collector that accumulates a bunch of Us to produce a C, which is generally a collection of U.
The A is an implementation detail, when you make methods that return collectors, this is generally some wildcard to not expose it to clients. But when you use the collector to perform the collection operation, then you need to declare A to capture it. You don't need to know that to use methods that use collectors, but the implementation of the method has to capture it.
It takes some time to get used to the collector API but once you get it there's really nothing voodoo happening.
Consider that this method is super reusable, you can do map(toSet(), listOfT, t -> t.getU()) and end up with a Set<U>. You can use any of the standard collectors in Collectors. It's much better than writing one specialized map function for each kind of result type (mapToSet, mapToList, etc).
| @@ -18,32 +19,25 @@ | |||
| */ | |||
| public abstract class AbstractJavaRulechainRule extends AbstractJavaRule { | |||
There was a problem hiding this comment.
Maybe we can get rid of this class eventually?
There was a problem hiding this comment.
Probably yes. But the fact that rules also implement the visitor interface is a problem for that, as the default visit method traverses the tree, which we want to avoid.
Merge branch 'pr-2490' into pmd/7.0.x Fixes #1785
|
Thanks for merging!
Yeah I don't remember why I mentioned the benchmark api here. What I meant, is that the implementation classes are probably internal API (and the |
Describe the PR
Simplify the way the rulechain is implemented, clarify the contract of Rule::apply, remove some needless API about the rulechain. In effect, this makes applying rules on a tree index the only way to apply rules. This subsumes the two previous ways to apply rules (rulechain, which uses an index, and Rule.apply, which doesn't).
Current problems
pmd/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/AbstractJavaRule.java
Lines 42 to 54 in 1c09a4f
pmd/pmd-jsp/src/main/java/net/sourceforge/pmd/lang/jsp/rule/AbstractJspRule.java
Lines 29 to 34 in 1c09a4f
visitAllmethod. This multiplies the extension points (and hence the failure points) for no benefit at all.This PR irons out this part of the API, by removing the divide between rulechain rules and other ones, and simplifying those extension points.
Changes
The basic API change to Rule is the following:
The selectors select nodes from an index of the tree, similar to the one previously built by RulechainVisitors. This is now used by all rules though. The same implementation is used by all languages.
Other API changes include:
applyis only called with nodes selected by the target selector, it's safe to rely on this and throw an exception if the node is not of the correct type. So implementations ofapplyalways cast without checking now.In the non-API changes, there is now a way to index and visit abstract types efficiently. This simplifies some rulechain rules (though eg for the java tree, many abstractions only exist in the java-grammar branch). This represents most of the code additions of this PR mind you.
Related issues
Ready?
Port remaining usages of addRuleChainVisit
Added unit tests for fixed bug/feature
Passing all unit tests
Complete build
./mvnw clean verifypasses (checked automatically by travis)Added (in-code) documentation (if needed)
Deprecations due on master
Future work
As part of the broader API changes around eg RuleContext and SourceCodeProcessor, I think removing RuleSets::apply would be appropriate. Possibly removing RuleSets altogether would be beneficial.