Add a declarative DSL to write rules that use SwiftSyntax

[ryanaveo]

This PR proposes an abstraction over writing rules that depend on SwiftSyntax. The abstraction will be a SwiftUI-like declarative DSL to write rules. The DSL will be made up of various Matchers that will map to common SwiftSyntax nodes such as class declarations, function call expressions...etc. As a proof of concept, this PR creates the Matchers Class and DownCast which match class declarations and "as" expressions respectively. To see it in use, we modify the ForceCastRule to use this new DSL.

Proposal

A declarative DSL to write rules that search the AST produced by SwiftSyntax for certain nodes.

Motivation

At my company, we write a lot of lint rules that depend on the AST of Swift source code. A common lint rule is "prohibit the use of a deprecated function within classes that conform to a specific protocol". We've built a tool that does in the past called NEAL.

Adding SwiftSyntax unlocked powerful parsing and inspection of the Swift AST in SwiftLint. Currently, rules that use SwiftSyntax have a private SyntaxVisitor implementation that walks through the AST starting at the root and collects the positions of lint violations. The SyntaxVisitor is a powerful API, but it has some drawbacks.

Drawbacks of SyntaxVisitor

1. Readability. It can be hard to reason about how the visitor will traverse the AST just by looking at the code itself. To implement a visitor class, you must override various visit functions which correspond to the current syntax node being visited. This makes debugging and refactoring difficult.
2. Code Duplication. Rules that depend on SwiftSyntax would be repeating a lot of code to access information about SwiftSyntax nodes such as its identifier and modifiers. Getting this information from a node is non-trivial since SwiftSyntax nodes are opaque types and may require type erasing. Finally, things like trivia such as whitespace may accompany this information which could produce some false negatives in source code that contains whitespace around a node. It would be good to consolidate this information to prevent bugs in the future when these nodes are used in other rules.
3. Difficulty Creating Hierarchical Rules. Imagine a lint rule that you only want to raise within XCTestCases. To do that, you'll need to create two separate visitor classes and traverse the children of the class node with the second visitor. Depending on the rules' complexity, the number of nested classes could grow. Any modification would require changing a lot of relationships between the SyntaxVisitor classes.

Goals

1. Simplicity. We want to abstract the creation and usage of these complex visitor classes with a declarative syntax. By writing rules in a declarative way, it is easy to modify and reason about the traversal of the AST.
2. Flexibility. There are more complicated use cases in lint rules where we want to do more than look for a single node. Developers should be able to create rules with both the DSL matchers and their own custom SyntaxVisitor classes if they want.

Design

The resultBuilder language feature is the perfect tool to simplify working with SyntaxVisitors since resultBuilders target list and tree structures. We'll use a resultBuilder to build up our SyntaxVisitors.

Let's see a rule written with this new DSL. Our example rule will be: "For all tests, prohibit the use of a deprecated API deprecatedTestHelper()"

Our rule would look something like:

public struct DeprecatedTestHelperRule: SyntaxVisitorRule, OptInRule, ConfigurationProviderRule, AutomaticTestableRule {
    public var configuration = SeverityConfiguration(.error)

    public init() {}

    public static let description = RuleDescription(
        identifier: "deprecated_test_helper",
        name: "Deprecated Test Helper",
        description: "Please do not use deprecatedTestHelper(), use testHelper() instead.",
        kind: .lint,
        nonTriggeringExamples: [Example("class Test: XCTestCase { func testA() { testHelper() }}")],
        triggeringExamples: [Example("class Test: XCTestCase { func testA() { ↓deprecatedTestHelper() }}")]
    )

    @SyntaxVisitorRuleValidatorBuilder
    public var validator: SyntaxVisitorRuleValidating {
        Class {
            CallExpression().name("deprecatedTestHelper")
        }
        .inheritsFrom(["XCTestCase"])
    }
}

Here, the validator describes how to traverse the AST of the source file and collect violations. Class and CallExpression are Matchers that will look for the equivalent syntax nodes. The inheritsFrom function is a Lint Modifier that will look inside of classes that inherit from XCTestCase. The closure expression after Class denotes a child visitor. For all children of the matching class declaration node, look for all call expressions with the name "deprecatedTestHelper".

The actual lint violations will be raised by the nodes that do not have child visitors. In other words "leaf nodes" such as the CallExpression above will raise the lint violation for our rule, and its position in the Swift file will be collected by the visitor.

Implementation

Rules
To create a rule that uses this abstraction, conform to SyntaxVisitorRule. It requires you to implement a validator which is a collection of syntax visitors that will be used to traverse the AST of the Swift file and collect violations. It provides default functionality for taking those violation positions and turning them into StyleViolations.

Validator
The validator is built by using a resultBuilder named SyntaxVisitorRuleValidatorBuilder. This resultBuilder allows us to define our rules with our custom DSL. The DSL is made up of Matchers and ViolationSyntaxVisitors.

Matchers
Used to find a single Swift Syntax node. It can be filtered by applying Lint Modifiers which are methods which changes the attributes to look for. Matchers are responsible for creating ViolationSyntaxVisitors and its relationships to other matchers or visitors.

ViolationSyntaxVisitors
Traverses the AST and collects the positions of a node if it violates according to the attributes it was passed.

Future Considerations

In the future, I hope to add more Matchers for various syntax nodes. We could also consider adding more operators such as anyOf() or all() or even scoped matchers (look only at the next immediate child node) to the DSL.

Alternatives Considered

Define these rules in the config file
I considered defining these rules in the .swiftlint.yml file. Doing so would allow us to create rules without creating a new release of SwiftLint for each new rule. However, the maintenance of these config files would be expensive as we continue modifying these abstractions whenever we add more functionality or conform to updates from SwiftSyntax. The biggest downside is losing the debugging and testability capabilities of writing these rules in native Swift.

[SwiftLintBot]

	1 Warning
⚠️	Big PR

	12 Messages
📖	Linting Aerial with this PR took 0.99s vs 0.99s on master (0% slower)
📖	Linting Alamofire with this PR took 1.09s vs 1.1s on master (0% faster)
📖	Linting Firefox with this PR took 4.4s vs 4.47s on master (1% faster)
📖	Linting Kickstarter with this PR took 7.67s vs 7.61s on master (0% slower)
📖	Linting Moya with this PR took 0.51s vs 0.54s on master (5% faster)
📖	Linting Nimble with this PR took 0.44s vs 0.41s on master (7% slower)
📖	Linting Quick with this PR took 0.19s vs 0.19s on master (0% slower)
📖	Linting Realm with this PR took 10.85s vs 9.96s on master (8% slower)
📖	Linting SourceKitten with this PR took 0.34s vs 0.34s on master (0% slower)
📖	Linting Sourcery with this PR took 2.03s vs 2.04s on master (0% faster)
📖	Linting Swift with this PR took 3.45s vs 3.45s on master (0% slower)
📖	Linting WordPress with this PR took 8.15s vs 8.16s on master (0% faster)

Generated by 🚫 Danger

[jpsim]

Nice proof of concept! I'm supportive of something like this, but would like to see it in use before merging it.

We have a handful of SwiftSyntax-based rules, maybe one of those would be a good candidate to migrate to this DSL?

Thanks for the PR :)

[codecov-commenter]

Codecov Report

Merging #4023 (c27163e) into master (c95670e) will decrease coverage by 0.04%.
The diff coverage is 89.92%.

@@            Coverage Diff             @@
##           master    #4023      +/-   ##
==========================================
- Coverage   92.46%   92.41%   -0.05%     
==========================================
  Files         448      463      +15     
  Lines       22798    23160     +362     
==========================================
+ Hits        21080    21404     +324     
- Misses       1718     1756      +38     

Impacted Files	Coverage Δ
...ntFramework/Helpers/SwiftSyntaxParsingErrors.swift	0.00% <0.00%> (ø)
...tFrameworkTests/AutomaticRuleTests.generated.swift	100.00% <ø> (ø)
...axMatchers/InheritableSyntaxVisitorBuildable.swift	45.45% <45.45%> (ø)
...axMatchers/SyntaxVisitorRuleValidatorBuilder.swift	75.00% <75.00%> (ø)
...iftLintFramework/Protocols/SyntaxVisitorRule.swift	83.33% <83.33%> (ø)
...ts/SwiftLintFrameworkTests/ClassMatcherTests.swift	85.00% <85.00%> (ø)
...sts/SwiftLintFrameworkTests/DeclVisitorTests.swift	92.91% <92.91%> (ø)
...wiftLintFramework/SyntaxVisitors/DeclVisitor.swift	95.55% <95.55%> (ø)
...LintFramework/Extensions/SwiftLintFile+Cache.swift	79.74% <100.00%> (ø)
...work/Extensions/SwiftSyntax/DeclSyntaxTraits.swift	100.00% <100.00%> (ø)
... and 11 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[ryanaveo]

@jpsim thank you for the feedback! I've modified ForceCastRule to use the DSL. Please let me know if there any additional changes I can make.

[jpsim]

Apologies for the delay in getting back to you. If the main goal of introducing this DSL is to make it easier to build SwiftSyntax-based rules by removing ceremony I feel like this accomplishes the opposite, where you need to first write a SyntaxVisitorBuildable component for what you want to match on before you can write your rule.

Contrast this with how concise SwiftSyntax rules are today and I don't think this is an improvement.

That being said, I do think there's an opportunity to remove some boilerplate involved with SwiftLint rules that use SwiftSyntax, but I don't think this is it.

If I'm missing something, maybe we can find a way to have a call where you can sell me on the advantages here that I'm missing.

[jpsim]

As an exploration, I've pushed a prototype of what some SwiftSyntax/SwiftLint convenience helpers could look like in #4126

It allows rules to provide visitors/rewriters and the framework takes care of the rest:

public struct MyCustomRule: SwiftSyntaxCorrectableRule, ConfigurationProviderRule {
  public var configuration = SeverityConfiguration(.warning)

  public init() {}

  public static let description = RuleDescription(...)

  public func makeVisitor(file: SwiftLintFile) -> ViolationsSyntaxVisitor? {
    file.locationConverter.map(MyCustomRuleVisitor.init)
  }

  public func makeRewriter(file: SwiftLintFile) -> ViolationsSyntaxRewriter? {
    file.locationConverter.map { locationConverter in
      MyCustomRuleRewriter(
        locationConverter: locationConverter,
        disabledRegions: disabledRegions(file: file)
      )
    }
  }
}

That's a pretty declarative way to define rules, removing a lot of boilerplate.

[ryanaveo]

Hi JP, sorry for the late reply!

I took a look at your PR and it looks great. I like how it standardizes how the SwiftSyntax rules validates a file and removes a lot of the ceremony needed. It also accounts for correctable rules which my PR does not.

Regarding the goals of the DSL I proposed, I was hoping it would solve the use cases when we have a lint violation that is nested in nature. The biggest use case for these nested AST rules are for enforcing best practices for libraries and frameworks. As an example:

Raise a lint violation for all call expressions of `expensiveFunction()` only inside of classes that inherit from `Component`

To write a visitor for this rule, we could write something like:

class ClassVisitor: ViolationsSyntaxVisitor {
   override func visitPost(_ node: ClassDeclSyntax) {
      if nodeInheritsFromComponent {
         let childVisitor = CallExpressionVisitor()
         violationsPositions += childVisitor.walk()
      }
   }
}

class FunctionCallExpressionVisitor: ViolationsSyntaxVisitor {
   override func visitPost(_ node: FunctionCallExprSyntax) {
      if nodeHasExpensiveFunctionIdentifier {
         violationsPositions.append(node.position)
      }
   }
}

The problems with the above solution are:

1. It is difficult to read how the AST is being traversed from the code alone.
2. The bigger problem is that if we want to add a childVisitor or even modify a childVisitor, it is difficult to do so. For example, if instead we wanted to look for type identifiers instead of call expressions, we'll need to first understand the structure of the visitors and know which class to swap out/change.
3. There is no standardization on getting information from common nodes. This could lead to duplicate work/expensive ways of retrieving information from a node.

With a DSL, we could change the above code to something like:

Class {
   CallExpression().name("expensiveFunction")
}
.inheritsFrom("Component")

The above code solves problems 1 and 2 because at a glance we can see what we want the visitor to do: for all classes that inherit from component, look for call expressions with the name "expensiveFunction". It solves number 3 by adding lint modifiers such as inheritsFrom and name where we can abstract how we're getting this information in case swift syntax's API changes and it can be changed and tested all in one place.

This is a simple example, but the complexity can grow quite large especially when we want more fine grained lint rules.

I agree that the DSL introduces more boilerplate by creating a mapping from a DSL matcher to its respective visitor classes. Maybe we can leverage some code generation in order to reduce this boilerplate. I think this duplication is worth the benefits gained from the DSL as it would help in migrating current rules to SwiftSyntax, protect our rules from any changes to the SwiftSyntax API, and open the doors for custom native rules to be more powerful.