Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support file-level ignore directive for specific rules #950

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions Documentation/IgnoringSource.md
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,22 @@ var a = foo+bar+baz
These ignore comments also apply to all children of the node, identical to the
behavior of the formatting ignore directive described above.

You can also disable specific source transforming rules for an entire file
by using the file-level ignore directive with a list of rule names. For example:

```swift
// swift-format-ignore-file: DoNotUseSemicolons, FullyIndirectEnum
import Zoo
import Arrays

struct Foo {
func foo() { bar();baz(); }
}
```
In this case, only the DoNotUseSemicolons and FullyIndirectEnum rules are disabled
throughout the file, while all other formatting rules (such as line breaking and
indentation) remain active.

## Understanding Nodes

`swift-format` parses Swift into an abstract syntax tree, where each element of
Expand Down
102 changes: 58 additions & 44 deletions Sources/SwiftFormat/Core/RuleMask.swift
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,16 @@ import SwiftSyntax
/// 2. | let a = 123
/// Ignores `RuleName` and `OtherRuleName` for line 2.
///
/// 1. | // swift-format-ignore-file: RuleName
/// 2. | let a = 123
/// 3. | class Foo { }
/// Ignores `RuleName` for the entire file (lines 2-3).
///
/// 1. | // swift-format-ignore-file: RuleName, OtherRuleName
/// 2. | let a = 123
/// 3. | class Foo { }
/// Ignores `RuleName` and `OtherRuleName` for the entire file (lines 2-3).
///
/// The rules themselves reference RuleMask to see if it is disabled for the line it is currently
/// examining.
@_spi(Testing)
Expand Down Expand Up @@ -85,6 +95,34 @@ extension SourceRange {
}
}

/// Represents the kind of ignore directive encountered in the source.
enum IgnoreDirective: CustomStringConvertible {
/// A node-level directive that disables rules for the following node and its children.
case node
/// A file-level directive that disables rules for the entire file.
case file

var description: String {
switch self {
case .node:
return "swift-format-ignore"
case .file:
return "swift-format-ignore-file"
}
}

/// Regex pattern to match an ignore comment. This pattern supports 0 or more comma delimited rule
/// names. The rule name(s), when present, are in capture group #3.
private var pattern: String {
return #"^\s*\/\/\s*"# + description + #"((:\s+(([A-z0-9]+[,\s]*)+))?$|\s+$)"#
}

/// Rule ignore regex object.
fileprivate var regex: NSRegularExpression {
return try! NSRegularExpression(pattern: pattern, options: [])
}
Comment on lines +116 to +123
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you are working on this already, I’ve got a few thoughts:

  • The regex restriction after the : seems a little restrictive to me. Eg. I imagine it would be fine to try and debug why a future rule that contains a _ cannot be ignored using a directive. Maybe we can just make the regex a prefix match. @allevato Do you remember why the regex is this specific?
  • This re-compiles the regex every time regex is accessed. Compiling regular expressions is fairly expensive, so I’d prefer to cache that.
  • Could we use Swift’s Regex instead of NSRegularExpression?
  • Or, alternatively, would it be easier to write a simple line matcher and avoid going through regex matching at all?

All except for avoiding to re-compile the regex don’t need to be in this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@allevato Do you remember why the regex is this specific?

Not off the top of my head. It may have just been to capture precisely what we expected rule names to look like at the time. I think it would be fine to be a little less prescriptive in the regex itself, if it simplifies things.

Could we use Swift’s Regex instead of NSRegularExpression?

We'd have to raise our minimum deployment version, which is macOS 12.0 right now, since Regex requires 13.0. If that aligns with other binaries in the Swift toolchain, I'd love to drop NSRegularExpression (holistically, in a separate PR).

}

/// A syntax visitor that finds `SourceRange`s of nodes that have rule status modifying comment
/// directives. The changes requested in each comment is parsed and collected into a map to support
/// status lookup per rule name.
Expand All @@ -106,30 +144,13 @@ fileprivate class RuleStatusCollectionVisitor: SyntaxVisitor {
/// Computes source locations and ranges for syntax nodes in a source file.
private let sourceLocationConverter: SourceLocationConverter

/// Regex pattern to match an ignore comment. This pattern supports 0 or more comma delimited rule
/// names. The rule name(s), when present, are in capture group #3.
private let ignorePattern =
#"^\s*\/\/\s*swift-format-ignore((:\s+(([A-z0-9]+[,\s]*)+))?$|\s+$)"#

/// Rule ignore regex object.
private let ignoreRegex: NSRegularExpression

/// Regex pattern to match an ignore comment that applies to an entire file.
private let ignoreFilePattern = #"^\s*\/\/\s*swift-format-ignore-file$"#

/// Rule ignore regex object.
private let ignoreFileRegex: NSRegularExpression

/// Stores the source ranges in which all rules are ignored.
var allRulesIgnoredRanges: [SourceRange] = []

/// Map of rule names to list ranges in the source where the rule is ignored.
var ruleMap: [String: [SourceRange]] = [:]

init(sourceLocationConverter: SourceLocationConverter) {
ignoreRegex = try! NSRegularExpression(pattern: ignorePattern, options: [])
ignoreFileRegex = try! NSRegularExpression(pattern: ignoreFilePattern, options: [])

self.sourceLocationConverter = sourceLocationConverter
super.init(viewMode: .sourceAccurate)
}
Expand All @@ -140,40 +161,28 @@ fileprivate class RuleStatusCollectionVisitor: SyntaxVisitor {
guard let firstToken = node.firstToken(viewMode: .sourceAccurate) else {
return .visitChildren
}
let comments = loneLineComments(in: firstToken.leadingTrivia, isFirstToken: true)
var foundIgnoreFileComment = false
for comment in comments {
let range = NSRange(comment.startIndex..<comment.endIndex, in: comment)
if ignoreFileRegex.firstMatch(in: comment, options: [], range: range) != nil {
foundIgnoreFileComment = true
break
}
}
guard foundIgnoreFileComment else {
return .visitChildren
}

let sourceRange = node.sourceRange(
converter: sourceLocationConverter,
afterLeadingTrivia: false,
afterTrailingTrivia: true
)
allRulesIgnoredRanges.append(sourceRange)
return .skipChildren
return appendRuleStatus(from: firstToken, of: sourceRange, for: .file)
}

override func visit(_ node: CodeBlockItemSyntax) -> SyntaxVisitorContinueKind {
guard let firstToken = node.firstToken(viewMode: .sourceAccurate) else {
return .visitChildren
}
return appendRuleStatusDirectives(from: firstToken, of: Syntax(node))
let sourceRange = node.sourceRange(converter: sourceLocationConverter)
return appendRuleStatus(from: firstToken, of: sourceRange, for: .node)
}

override func visit(_ node: MemberBlockItemSyntax) -> SyntaxVisitorContinueKind {
guard let firstToken = node.firstToken(viewMode: .sourceAccurate) else {
return .visitChildren
}
return appendRuleStatusDirectives(from: firstToken, of: Syntax(node))
let sourceRange = node.sourceRange(converter: sourceLocationConverter)
return appendRuleStatus(from: firstToken, of: sourceRange, for: .node)
}

// MARK: - Helper Methods
Expand All @@ -183,17 +192,19 @@ fileprivate class RuleStatusCollectionVisitor: SyntaxVisitor {
///
/// - Parameters:
/// - token: A token that may have comments that modify the status of rules.
/// - node: The node to which the token belongs.
private func appendRuleStatusDirectives(
/// - sourceRange: The range covering the node to which `token` belongs. If an ignore directive
/// is found among the comments, this entire range is used to ignore the specified rules.
/// - directive: The type of ignore directive to look for.
private func appendRuleStatus(
from token: TokenSyntax,
of node: Syntax
of sourceRange: SourceRange,
for directive: IgnoreDirective
) -> SyntaxVisitorContinueKind {
let isFirstInFile = token.previousToken(viewMode: .sourceAccurate) == nil
let matches = loneLineComments(in: token.leadingTrivia, isFirstToken: isFirstInFile)
.compactMap(ruleStatusDirectiveMatch)
let sourceRange = node.sourceRange(converter: sourceLocationConverter)
for match in matches {
switch match {
let comments = loneLineComments(in: token.leadingTrivia, isFirstToken: isFirstInFile)
for comment in comments {
guard let matchResult = ruleStatusDirectiveMatch(in: comment, using: directive.regex) else { continue }
switch matchResult {
case .all:
allRulesIgnoredRanges.append(sourceRange)

Expand All @@ -210,9 +221,12 @@ fileprivate class RuleStatusCollectionVisitor: SyntaxVisitor {

/// Checks if a comment containing the given text matches a rule status directive. When it does
/// match, its contents (e.g. list of rule names) are returned.
private func ruleStatusDirectiveMatch(in text: String) -> RuleStatusDirectiveMatch? {
private func ruleStatusDirectiveMatch(
in text: String,
using regex: NSRegularExpression
) -> RuleStatusDirectiveMatch? {
let textRange = NSRange(text.startIndex..<text.endIndex, in: text)
guard let match = ignoreRegex.firstMatch(in: text, options: [], range: textRange) else {
guard let match = regex.firstMatch(in: text, options: [], range: textRange) else {
return nil
}
guard match.numberOfRanges == 5 else { return .all }
Expand Down
10 changes: 7 additions & 3 deletions Sources/SwiftFormat/Rules/OrderedImports.swift
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,11 @@ public final class OrderedImports: SyntaxFormatRule {
if atStartOfFile {
switch line.type {
case .comment:
commentBuffer.append(line)
if line.description.contains(IgnoreDirective.file.description) {
fileHeader.append(line)
} else {
commentBuffer.append(line)
}
continue

case .blankLine:
Expand Down Expand Up @@ -520,8 +524,8 @@ fileprivate class Line {
}
}

extension Line: CustomDebugStringConvertible {
var debugDescription: String {
extension Line: CustomStringConvertible {
var description: String {
var description = ""
if !leadingTrivia.isEmpty {
var newlinesCount = 0
Expand Down
171 changes: 171 additions & 0 deletions Tests/SwiftFormatTests/Core/RuleMaskTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -230,4 +230,175 @@ final class RuleMaskTests: XCTestCase {
XCTAssertEqual(mask.ruleState("rule1", at: location(ofLine: i)), .default)
}
}

func testSingleRuleWholeFileIgnore() {
let text =
"""
// This file has important contents.
// swift-format-ignore-file: rule1
// Everything in this file is ignored.

let a = 5
let b = 4

class Foo {
let member1 = 0
func foo() {
baz()
}
}

struct Baz {
}
"""

let mask = createMask(sourceText: text)

let lineCount = text.split(separator: "\n").count
for i in 0..<lineCount {
XCTAssertEqual(mask.ruleState("rule1", at: location(ofLine: i)), .disabled)
XCTAssertEqual(mask.ruleState("rule2", at: location(ofLine: i)), .default)
}
}

func testMultipleRulesWholeFileIgnore() {
let text =
"""
// This file has important contents.
// swift-format-ignore-file: rule1, rule2, rule3
// Everything in this file is ignored.

let a = 5
let b = 4

class Foo {
let member1 = 0
func foo() {
baz()
}
}

struct Baz {
}
"""

let mask = createMask(sourceText: text)

let lineCount = text.split(separator: "\n").count
for i in 0..<lineCount {
XCTAssertEqual(mask.ruleState("rule1", at: location(ofLine: i)), .disabled)
XCTAssertEqual(mask.ruleState("rule2", at: location(ofLine: i)), .disabled)
XCTAssertEqual(mask.ruleState("rule3", at: location(ofLine: i)), .disabled)
XCTAssertEqual(mask.ruleState("rule4", at: location(ofLine: i)), .default)
}
}

func testFileAndLineIgnoresMixed() {
let text =
"""
// This file has important contents.
// swift-format-ignore-file: rule1, rule2
// Everything in this file is ignored.

let a = 5
// swift-format-ignore: rule3
let b = 4

class Foo {
// swift-format-ignore: rule3, rule4
let member1 = 0

func foo() {
baz()
}
}

struct Baz {
}
"""

let mask = createMask(sourceText: text)

let lineCount = text.split(separator: "\n").count
for i in 0..<lineCount {
XCTAssertEqual(mask.ruleState("rule1", at: location(ofLine: i)), .disabled)
XCTAssertEqual(mask.ruleState("rule2", at: location(ofLine: i)), .disabled)
if i == 7 {
XCTAssertEqual(mask.ruleState("rule3", at: location(ofLine: i)), .disabled)
XCTAssertEqual(mask.ruleState("rule4", at: location(ofLine: i)), .default)
} else if i == 11 {
XCTAssertEqual(mask.ruleState("rule3", at: location(ofLine: i, column: 3)), .disabled)
XCTAssertEqual(mask.ruleState("rule4", at: location(ofLine: i, column: 3)), .disabled)
} else {
XCTAssertEqual(mask.ruleState("rule3", at: location(ofLine: i)), .default)
XCTAssertEqual(mask.ruleState("rule4", at: location(ofLine: i)), .default)
}
}
}

func testMultipleSubsetFileIgnoreDirectives() {
let text =
"""
// This file has important contents.
// swift-format-ignore-file: rule1
// swift-format-ignore-file: rule2
// Everything in this file is ignored.

let a = 5
let b = 4

class Foo {
let member1 = 0

func foo() {
baz()
}
}

struct Baz {
}
"""

let mask = createMask(sourceText: text)

let lineCount = text.split(separator: "\n").count
for i in 0..<lineCount {
XCTAssertEqual(mask.ruleState("rule1", at: location(ofLine: i)), .disabled)
XCTAssertEqual(mask.ruleState("rule2", at: location(ofLine: i)), .disabled)
XCTAssertEqual(mask.ruleState("rule3", at: location(ofLine: i)), .default)
}
}

func testSubsetAndAllFileIgnoreDirectives() {
let text =
"""
// This file has important contents.
// swift-format-ignore-file: rule1
// swift-format-ignore-file
// Everything in this file is ignored.

let a = 5
let b = 4

class Foo {
let member1 = 0

func foo() {
baz()
}
}

struct Baz {
}
"""

let mask = createMask(sourceText: text)

let lineCount = text.split(separator: "\n").count
for i in 0..<lineCount {
XCTAssertEqual(mask.ruleState("rule1", at: location(ofLine: i)), .disabled)
XCTAssertEqual(mask.ruleState("rule2", at: location(ofLine: i)), .disabled)
XCTAssertEqual(mask.ruleState("rule3", at: location(ofLine: i)), .disabled)
}
}
}
Loading