Conditional ordering of rules

[paul-dingemans]

Description

For backgrounds see issue: #1229.

Description below is identical to the commit messages in the order they are applied. Only the last commits is new functionality. All preceding commits are preparations.

Extract VisitorProvider from Ktlint class

The visitor is created once for each file that has to be linted/formatted. It is now divided into two distinct phases.

The setup phase is executed whenever the VisitorProvider is created. This provider is created only once per invocation of the KtLint command. In the setup phase, the rules are analysed and the order in which they have to be executed is determined. This order is identical for all nodes. As the phase is executed only once, it can now containing logging about the order in which the rules are executed. Also it opens up possibilities for more complex ordering rules using annotations and reflection (for example executing a rule after a specific rule).

The visitor creation phase actually creates the visitor. In this phase all enabled rules will be executed. Note that disabling of rules depends on the '.editorconfig' which is active for a given rootNode.

Split test into multiple tests

Split DisabledRulesTest into multiple tests.

Move sorting on ruleSetId from RuleSetsLoader to VisitorProvider

The order in which the rule sets are discovered is not relevant for the loader. Ordering rules by rule set however is important within the scope of the VisitorProvider though.

Also add order that experimental rules takes precedence over custom rules. Finally add secondary sorting on ruleId so that the order of the rules as defined by the RuleSetProviders is not relevant anymore.

Restrict usage of Rule.Modifier to initialisation phase in which the order of the rules is determined

Rule.Modifier is no longer used during actual creation of the visitor. When creating the visitor the rules are already ordered. This opens up the new possibilities to add other rule ordering mechanisms.

Note that this introduces functional changes below with regard to the order of execution of the rules:

• Rules without Rule.Modifier and rules with Rule.Modifier.RestrictToRoot are now handled as one group instead of two distinct groups.
• Rules with Rule.Modifier.Last and rules with Rule.Modifier.RestrictToRootLast are now handled as one group instead of two distinct groups.

Add annotations RunAsLateAsPossible and RunOnRootNodeOnly as replacements for the Rule.Modifier interfaces

Rules in the standard and experimental rule sets already are converted to use the annotations instead of the Rule.Modifier interfaces. The Rule.Modifiers interface are kept to support a grace period in which maintainers of custom rule sets should migrate as well.

Add annotation RunAfterRule and alter the rule execution order (NEW FUNCTIONALITY)

Ensures that the rule annotated with RunAfterRule appears in the rule execution order after a specified other rule

Checklist

• tests are added
• CHANGELOG.md is updated

[shashachu]

thanks for the PR @paul-dingemans. It's an interesting idea! I feel that in theory rules should 'just work' no matter what order they're run - i.e. either they'll have work to do, or not, and if you have conflicting rules, that's sort of your problem. Similarly, you should not depend on some particular rule running before yours in order for your rule to work. But in practice maybe this ends up not being the case. I don't think I have a big problem with this PR, but I don't want to see us introducing a lot of strict ordering requirements in our standard or even experimental set of rules.

[paul-dingemans]

I do agree that dependencies between rules should be avoided when possible. But when I have too choose between a rule dependency versus implementing similar logic in multiple rules than I prefer the dependency.

For example the rule for wrapping arguments also does some indenting but it conflicts with indenting of the indent rule which is confusing when running lint without format. I think it would be better if indentation is solely reported by the the indent rule.

Within the standard and experimental rules we can be strict on using rule dependencies. With this functionality we do empower custom ruleset builders to make their own decisions. But with great power comes great responsibility 😉

[romtsn]

Yeah the wrapping rule should just do wrapping, and the indentation should be delegated to the indent rule. We'd need to think of a good way to have rule dependencies.

To me, the downside in this approach is that we have to introduce a ~3MB kotlin-reflect dependency, if possible it would be cool to avoid this.

[paul-dingemans]

To me, the downside in this approach is that we have to introduce a ~3MB kotlin-reflect dependency, if possible it would be cool to avoid this.

I didn't thought about this. I have replaced the annotations with a new sealed class VisitorModifier. Now the kotlin-reflect is no longer needed. I did like the annotation approach a little more, but I am also happy with the new approach.

[paul-dingemans]

@romtsn Could you have another look at this PR please? The unwanted kotlin-reflect dependency was removed a while ago. If you have no further remarks, I really would be pleased if this gets merged in the next ktlint release so that I can resolve my multiline string indent rule in a separate ruleset.