Merge pull request swiftlang#16 from allevato/pipeline

Lint pipeline improvements; fix enabling/disabling of lint rules.

Author: allevato

Sources/SwiftFormat/LintPipeline.swift

@@ -25,4 +25,42 @@ struct LintPipeline: SyntaxVisitor {
   init(context: Context) {
     self.context = context
   }
+
+  /// Calls the `visit` method of a rule for the given node if that rule is enabled for the node.
+  ///
+  /// - Parameters:
+  ///   - visitor: A reference to the `visit` method on the *type* of a `SyntaxLintRule` subclass.
+  ///     The type of the rule in question is inferred from the signature of the method reference.
+  ///   - context: The formatter context that contains information about which rules are enabled or
+  ///     disabled.
+  ///   - node: The syntax node on which the rule will be applied. This lets us check whether the
+  ///     rule is enabled for the particular source range where the node occurs.
+  func visitIfEnabled<Rule: SyntaxLintRule, Node: Syntax>(
+    _ visitor: (Rule) -> (Node) -> SyntaxVisitorContinueKind, in context: Context, for node: Node
+  ) {
+    guard context.isRuleEnabled(Rule.self.ruleName, node: node) else { return }
+    let rule = Rule(context: context)
+    _ = visitor(rule)(node)
+  }
+
+  /// Calls the `visit` method of a rule for the given node if that rule is enabled for the node.
+  ///
+  /// - Parameters:
+  ///   - visitor: A reference to the `visit` method on the *type* of a `SyntaxFormatRule` subclass.
+  ///     The type of the rule in question is inferred from the signature of the method reference.
+  ///   - context: The formatter context that contains information about which rules are enabled or
+  ///     disabled.
+  ///   - node: The syntax node on which the rule will be applied. This lets us check whether the
+  ///     rule is enabled for the particular source range where the node occurs.
+  func visitIfEnabled<Rule: SyntaxFormatRule, Node: Syntax>(
+    _ visitor: (Rule) -> (Node) -> Any, in context: Context, for node: Node
+  ) {
+    // Note that visitor function type is expressed as `Any` because we ignore the return value, but
+    // more importantly because the `visit` methods return protocol refinements of `Syntax` that
+    // cannot currently be expressed as constraints without duplicating this function for each of
+    // them individually.
+    guard context.isRuleEnabled(Rule.self.ruleName, node: node) else { return }
+    let rule = Rule(context: context)
+    _ = visitor(rule)(node)
+  }
 }

Sources/SwiftFormat/Pipelines+Generated.swift

@@ -73,9 +73,16 @@ public class Context {
 
   /// Given a rule's name and the node it is examining, determine if the rule is disabled at this
   /// location or not.
-  public func isRuleDisabled(_ ruleName: String, node: Syntax) -> Bool {
+  public func isRuleEnabled(_ ruleName: String, node: Syntax) -> Bool {
     let loc = node.startLocation(converter: self.sourceLocationConverter)
     guard let line = loc.line else { return false }
-    return self.ruleMask.isDisabled(ruleName, line: line)
+    switch ruleMask.ruleState(ruleName, atLine: line) {
+    case .default:
+      return configuration.rules[ruleName] ?? false
+    case .disabled:
+      return false
+    case .enabled:
+      return true
+    }
   }
 }

Sources/SwiftFormatCore/Context.swift

@@ -17,6 +17,6 @@ extension Diagnostic.Message {
   /// - parameter rule: The rule whose name will be prepended to the diagnostic.
   /// - returns: A new `Diagnostic.Message` with the name of the provided rule prepended.
   public func withRule(_ rule: Rule) -> Diagnostic.Message {
-    return .init(severity, "[\(rule.ruleName)]: \(text)")
+    return .init(severity, "[\(type(of: rule).ruleName)]: \(text)")
   }
 }

Sources/SwiftFormatCore/Diagnostic+Rule.swift

@@ -16,7 +16,7 @@ public protocol Rule {
   var context: Context { get }
 
   /// The human-readable name of the rule. This defaults to the class name.
-  var ruleName: String { get }
+  static var ruleName: String { get }
 
   /// Creates a new Rule in a given context.
   init(context: Context)
@@ -26,12 +26,10 @@ private var nameCache = [ObjectIdentifier: String]()
 
 extension Rule {
   /// By default, the `ruleName` is just the name of the implementing rule class.
-  public var ruleName: String {
-    let myType = type(of: self)
-    // TODO(abl): Test and potentially replace with static initialization.
+  public static var ruleName: String {
     return nameCache[
-      ObjectIdentifier(myType),
-      default: String("\(myType)".split(separator: ".").last!)
+      ObjectIdentifier(self),
+      default: String("\(self)".split(separator: ".").last!)
     ]
   }
 }

Sources/SwiftFormatCore/Rule.swift

@@ -24,8 +24,19 @@ import SwiftSyntax
 /// RuleMask to see if it is disabled for the line it is currently examining.
 public class RuleMask {
 
-  /// Each rule has a list of ranges for which it is disabled.
-  private var ruleMap: [String: [Range<Int>]] = [:]
+  /// Information about whether a particular lint/format rule is enabled or disabled for a range of
+  /// lines in the source file.
+  private struct LineRangeState {
+
+    /// The line range where a rule is either disabled or enabled.
+    var range: Range<Int>
+
+    /// Indicates whether the rule is enabled in this range.
+    var isEnabled: Bool
+  }
+
+  /// Each rule has a list of ranges for which it is explicitly enabled or disabled.
+  private var ruleMap: [String: [LineRangeState]] = [:]
 
   /// Regex to match the enable comments; rule name is in the first capture group.
   private let enablePattern = #"^\s*//\s*swift-format-enable:\s+(\S+)"#
@@ -58,8 +69,9 @@ public class RuleMask {
     return loc.line
   }
 
-  /// Check if a comment matches a disable/enable flag.
+  /// Check if a comment matches a disable/enable flag, and if so, returns the name of the rule.
   private func getRule(regex: NSRegularExpression, text: String) -> String? {
+    // TODO: Support multiple rules in the same comment; e.g., a comma-delimited list.
     let nsrange = NSRange(text.startIndex..<text.endIndex, in: text)
     if let match = regex.firstMatch(in: text, options: [], range: nsrange) {
       let matchRange = match.range(at: 1)
@@ -73,47 +85,73 @@ public class RuleMask {
   /// Generate the dictionary (ruleMap) by walking the syntax tokens.
   private func generateDictionary(_ node: Syntax) {
     var disableStart: [String: Int] = [:]
+    var enableStart: [String: Int] = [:]
+
     for token in node.tokens {
-      guard let leadingtrivia = token.leadingTrivia else { continue }
+      guard let leadingTrivia = token.leadingTrivia else { continue }
 
       // Flags must be on lines by themselves: not at the end of an existing line.
       var firstPiece = true
 
-      for piece in leadingtrivia {
+      for piece in leadingTrivia {
         guard case .lineComment(let text) = piece else {
           firstPiece = false
           continue
         }
         guard !firstPiece else { continue }
 
-        if let disableRule = getRule(regex: disableRegex, text: text) {
-          guard !disableStart.keys.contains(disableRule) else { continue }
-          guard let startLine = getLine(token) else { continue }
-          disableStart[disableRule] = startLine
-        }
+        if let ruleName = getRule(regex: disableRegex, text: text) {
+          guard !disableStart.keys.contains(ruleName) else { continue }
+          guard let disableStartLine = getLine(token) else { continue }
 
-        if let enableRule = getRule(regex: enableRegex, text: text) {
-          guard let startLine = disableStart.removeValue(forKey: enableRule) else { continue }
-          guard let endLine = getLine(token) else { continue }
-          let exclusionRange = startLine..<endLine
+          if let enableStartLine = enableStart[ruleName] {
+            // If we're processing an enable block for the rule, finalize it.
+            ruleMap[ruleName, default: []].append(
+              LineRangeState(range: enableStartLine..<disableStartLine, isEnabled: true))
+            enableStart[ruleName] = nil
+          }
 
-          if ruleMap.keys.contains(enableRule) {
-            ruleMap[enableRule]?.append(exclusionRange)
-          } else {
-            ruleMap[enableRule] = [exclusionRange]
+          disableStart[ruleName] = disableStartLine
+        }
+        else if let ruleName = getRule(regex: enableRegex, text: text) {
+          guard !enableStart.keys.contains(ruleName) else { continue }
+          guard let enableStartLine = getLine(token) else { continue }
+
+          if let disableStartLine = disableStart[ruleName] {
+            // If we're processing a disable block for the rule, finalize it.
+            ruleMap[ruleName, default: []].append(
+              LineRangeState(range: disableStartLine..<enableStartLine, isEnabled: false))
+            disableStart[ruleName] = nil
           }
+
+          enableStart[ruleName] = enableStartLine
         }
+
         firstPiece = false
       }
     }
+
+    // Finalize any remaining blocks by closing them off at the last line number in the file.
+    guard let lastToken = node.lastToken, let lastLine = getLine(lastToken) else { return }
+
+    for (ruleName, disableStartLine) in disableStart {
+      ruleMap[ruleName, default: []].append(
+        LineRangeState(range: disableStartLine..<lastLine + 1, isEnabled: false))
+    }
+    for (ruleName, enableStartLine) in enableStart {
+      ruleMap[ruleName, default: []].append(
+        LineRangeState(range: enableStartLine..<lastLine + 1, isEnabled: true))
+    }
   }
 
   /// Return if the given rule is disabled on the provided line.
-  public func isDisabled(_ rule: String, line: Int) -> Bool {
-    guard let ranges = ruleMap[rule] else { return false }
-    for range in ranges {
-      if range.contains(line) { return true }
+  public func ruleState(_ rule: String, atLine line: Int) -> RuleState {
+    guard let rangeStates = ruleMap[rule] else { return .default }
+    for rangeState in rangeStates {
+      if rangeState.range.contains(line) {
+        return rangeState.isEnabled ? .enabled : .disabled
+      }
     }
-    return false
+    return .default
   }
 }

Sources/SwiftFormatCore/RuleMask.swift

@@ -0,0 +1,26 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2019 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+
+/// The enablement of a lint/format rule based on the presence or absence of comment directives in
+/// the source file.
+public enum RuleState {
+
+  /// There is no explicit information in the source file about whether the rule should be enabled
+  /// or disabled at the requested location, so the configuration default should be used.
+  case `default`
+
+  /// The rule is explicitly enabled at the requested location.
+  case enabled
+
+  /// The rule is explicitly disabled at the requested location.
+  case disabled
+}

Sources/SwiftFormatCore/RuleState.swift

@@ -14,7 +14,16 @@ import Foundation
 import SwiftSyntax
 
 /// A rule that lints a given file.
-public protocol SyntaxLintRule: SyntaxVisitor, Rule {}
+open class SyntaxLintRule: SyntaxVisitorBase, Rule {
+
+  /// The context in which the rule is executed.
+  public let context: Context
+
+  /// Creates a new rule in a given context.
+  public required init(context: Context) {
+    self.context = context
+  }
+}
 
 extension Rule {
   /// Emits the provided diagnostic to the diagnostic engine.

Sources/SwiftFormatCore/SyntaxLintRule.swift

@@ -19,55 +19,50 @@ import SwiftSyntax
 /// Lint: If a public declaration is missing a documentation comment, a lint error is raised.
 ///
 /// - SeeAlso: https://google.github.io/swift#where-to-document
-public struct AllPublicDeclarationsHaveDocumentation: SyntaxLintRule {
-  public let context: Context
+public final class AllPublicDeclarationsHaveDocumentation: SyntaxLintRule {
 
-  public init(context: Context) {
-    self.context = context
-  }
-
-  public func visit(_ node: FunctionDeclSyntax) -> SyntaxVisitorContinueKind {
+  public override func visit(_ node: FunctionDeclSyntax) -> SyntaxVisitorContinueKind {
     diagnoseMissingDocComment(node, name: node.fullDeclName, modifiers: node.modifiers)
     return .skipChildren
   }
 
-  public func visit(_ node: InitializerDeclSyntax) -> SyntaxVisitorContinueKind {
+  public override func visit(_ node: InitializerDeclSyntax) -> SyntaxVisitorContinueKind {
     diagnoseMissingDocComment(node, name: "init", modifiers: node.modifiers)
     return .skipChildren
   }
 
-  public func visit(_ node: DeinitializerDeclSyntax) -> SyntaxVisitorContinueKind {
+  public override func visit(_ node: DeinitializerDeclSyntax) -> SyntaxVisitorContinueKind {
     diagnoseMissingDocComment(node, name: "deinit", modifiers: node.modifiers)
     return .skipChildren
   }
 
-  public func visit(_ node: SubscriptDeclSyntax) -> SyntaxVisitorContinueKind {
+  public override func visit(_ node: SubscriptDeclSyntax) -> SyntaxVisitorContinueKind {
     diagnoseMissingDocComment(node, name: "subscript", modifiers: node.modifiers)
     return .skipChildren
   }
 
-  public func visit(_ node: ClassDeclSyntax) -> SyntaxVisitorContinueKind {
+  public override func visit(_ node: ClassDeclSyntax) -> SyntaxVisitorContinueKind {
     diagnoseMissingDocComment(node, name: node.identifier.text, modifiers: node.modifiers)
     return .skipChildren
   }
 
-  public func visit(_ node: VariableDeclSyntax) -> SyntaxVisitorContinueKind {
+  public override func visit(_ node: VariableDeclSyntax) -> SyntaxVisitorContinueKind {
     guard let mainBinding = node.bindings.firstAndOnly else { return .skipChildren }
     diagnoseMissingDocComment(node, name: "\(mainBinding.pattern)", modifiers: node.modifiers)
     return .skipChildren
   }
 
-  public func visit(_ node: StructDeclSyntax) -> SyntaxVisitorContinueKind {
+  public override func visit(_ node: StructDeclSyntax) -> SyntaxVisitorContinueKind {
     diagnoseMissingDocComment(node, name: node.identifier.text, modifiers: node.modifiers)
     return .skipChildren
   }
 
-  public func visit(_ node: ProtocolDeclSyntax) -> SyntaxVisitorContinueKind {
+  public override func visit(_ node: ProtocolDeclSyntax) -> SyntaxVisitorContinueKind {
     diagnoseMissingDocComment(node, name: node.identifier.text, modifiers: node.modifiers)
     return .skipChildren
   }
 
-  public func visit(_ node: TypealiasDeclSyntax) -> SyntaxVisitorContinueKind {
+  public override func visit(_ node: TypealiasDeclSyntax) -> SyntaxVisitorContinueKind {
     diagnoseMissingDocComment(node, name: node.identifier.text, modifiers: node.modifiers)
     return .skipChildren
   }

Sources/SwiftFormatRules/AllPublicDeclarationsHaveDocumentation.swift

@@ -21,14 +21,9 @@ import SwiftSyntax
 ///       raised.
 ///
 /// - SeeAlso: https://google.github.io/swift#identifiers
-public struct AlwaysUseLowerCamelCase: SyntaxLintRule {
-  public let context: Context
+public final class AlwaysUseLowerCamelCase: SyntaxLintRule {
 
-  public init(context: Context) {
-    self.context = context
-  }
-
-  public func visit(_ node: VariableDeclSyntax) -> SyntaxVisitorContinueKind {
+  public override func visit(_ node: VariableDeclSyntax) -> SyntaxVisitorContinueKind {
     for binding in node.bindings {
       guard let pat = binding.pattern as? IdentifierPatternSyntax else {
         continue
@@ -38,12 +33,12 @@ public struct AlwaysUseLowerCamelCase: SyntaxLintRule {
     return .skipChildren
   }
 
-  public func visit(_ node: FunctionDeclSyntax) -> SyntaxVisitorContinueKind {
+  public override func visit(_ node: FunctionDeclSyntax) -> SyntaxVisitorContinueKind {
     diagnoseLowerCamelCaseViolations(node.identifier)
     return .skipChildren
   }
 
-  public func visit(_ node: EnumCaseElementSyntax) -> SyntaxVisitorContinueKind {
+  public override func visit(_ node: EnumCaseElementSyntax) -> SyntaxVisitorContinueKind {
     diagnoseLowerCamelCaseViolations(node.identifier)
     return .skipChildren
   }