Skip to content

Commit

Permalink
Improve performance of excluded files filter
Browse files Browse the repository at this point in the history
The current algorithm is like "collect all included files and subtract all excluded files".
Collecting all included and all excluded files relies on the file system. This can become slow
when the patterns used to exclude files resolve to a large number of files.

The new approach only collects all lintable files and checks them against the exclude patterns.
This can be done by in-memory string-regex-match and does therefore not require file system accesses.
  • Loading branch information
SimplyDanny committed Aug 28, 2023
1 parent 1f65377 commit 4094558
Show file tree
Hide file tree
Showing 7 changed files with 84 additions and 78 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@
[keith](https://github.com/keith)
[5139](https://github.com/realm/SwiftLint/pull/5139)

* Improve performance when exclude patterns resolve to a large set of files.
[SimplyDanny](https://github.com/SimplyDanny)
[#5018](https://github.com/realm/SwiftLint/issues/5018)

#### Bug Fixes

* Fix false positive in `control_statement` rule that triggered on conditions
Expand Down
70 changes: 19 additions & 51 deletions Source/SwiftLintCore/Extensions/Configuration+LintableFiles.swift
Original file line number Diff line number Diff line change
@@ -1,24 +1,20 @@
import Foundation

extension Configuration {
public enum ExcludeBy {
case prefix
case paths(excludedPaths: [String])
}

// MARK: Lintable Paths

/// Returns the files that can be linted by SwiftLint in the specified parent path.
///
/// - parameter path: The parent path in which to search for lintable files. Can be a directory or a
/// file.
/// - parameter forceExclude: Whether or not excludes defined in this configuration should be applied even if
/// `path` is an exact match.
/// - parameter excludeByPrefix: Whether or not uses excluding by prefix algorithm.
/// - parameter excludeByPrefix: Whether or not it uses the exclude-by-prefix algorithm.
///
/// - returns: Files to lint.
public func lintableFiles(inPath path: String, forceExclude: Bool,
excludeBy: ExcludeBy) -> [SwiftLintFile] {
return lintablePaths(inPath: path, forceExclude: forceExclude, excludeBy: excludeBy)
excludeByPrefix: Bool) -> [SwiftLintFile] {
return lintablePaths(inPath: path, forceExclude: forceExclude, excludeByPrefix: excludeByPrefix)
.compactMap(SwiftLintFile.init(pathDeferringReading:))
}

Expand All @@ -28,24 +24,21 @@ extension Configuration {
/// file.
/// - parameter forceExclude: Whether or not excludes defined in this configuration should be applied even if
/// `path` is an exact match.
/// - parameter excludeByPrefix: Whether or not uses excluding by prefix algorithm.
/// - parameter excludeByPrefix: Whether or not it uses the exclude-by-prefix algorithm.
/// - parameter fileManager: The lintable file manager to use to search for lintable files.
///
/// - returns: Paths for files to lint.
internal func lintablePaths(
inPath path: String,
forceExclude: Bool,
excludeBy: ExcludeBy,
excludeByPrefix: Bool,
fileManager: LintableFileManager = FileManager.default
) -> [String] {
if fileManager.isFile(atPath: path) {
if forceExclude {
switch excludeBy {
case .prefix:
return filterExcludedPathsByPrefix(in: [path.absolutePathStandardized()])
case .paths(let excludedPaths):
return filterExcludedPaths(excludedPaths, in: [path.absolutePathStandardized()])
}
return excludeByPrefix
? filterExcludedPathsByPrefix(in: [path.absolutePathStandardized()])
: filterExcludedPaths(in: [path.absolutePathStandardized()])
}
// If path is a file and we're not forcing excludes, skip filtering with excluded/included paths
return [path]
Expand All @@ -56,35 +49,22 @@ extension Configuration {
.flatMap(Glob.resolveGlob)
.parallelFlatMap { fileManager.filesToLint(inPath: $0, rootDirectory: rootDirectory) }

switch excludeBy {
case .prefix:
return filterExcludedPathsByPrefix(in: pathsForPath, includedPaths)
case .paths(let excludedPaths):
return filterExcludedPaths(excludedPaths, in: pathsForPath, includedPaths)
}
return excludeByPrefix
? filterExcludedPathsByPrefix(in: pathsForPath, includedPaths)
: filterExcludedPaths(in: pathsForPath, includedPaths)
}

/// Returns an array of file paths after removing the excluded paths as defined by this configuration.
///
/// - parameter fileManager: The lintable file manager to use to expand the excluded paths into all matching paths.
/// - parameter paths: The input paths to filter.
/// - parameter paths: The input paths to filter.
///
/// - returns: The input paths after removing the excluded paths.
public func filterExcludedPaths(
_ excludedPaths: [String],
in paths: [String]...
) -> [String] {
let allPaths = paths.flatMap { $0 }
#if os(Linux)
let result = NSMutableOrderedSet(capacity: allPaths.count)
result.addObjects(from: allPaths)
#else
let result = NSMutableOrderedSet(array: allPaths)
#endif

result.minusSet(Set(excludedPaths))
// swiftlint:disable:next force_cast
return result.map { $0 as! String }
public func filterExcludedPaths(in paths: [String]...) -> [String] {
let exclusionPatterns = self.excludedPaths.compactMap { Glob.toRegex($0, rootPath: rootDirectory) }
let pathsWithoutExclusions = paths
.flatMap { $0 }
.filter { path in !exclusionPatterns.contains { $0.firstMatch(in: path, range: path.fullNSRange) != nil } }
return pathsWithoutExclusions.unique
}

/// Returns the file paths that are excluded by this configuration using filtering by absolute path prefix.
Expand All @@ -101,16 +81,4 @@ extension Configuration {
!excludedPaths.contains { path.hasPrefix($0) }
}
}

/// Returns the file paths that are excluded by this configuration after expanding them using the specified file
/// manager.
///
/// - parameter fileManager: The file manager to get child paths in a given parent location.
///
/// - returns: The expanded excluded file paths.
public func excludedPaths(fileManager: LintableFileManager = FileManager.default) -> [String] {
return excludedPaths
.flatMap(Glob.resolveGlob)
.parallelFlatMap { fileManager.filesToLint(inPath: $0, rootDirectory: rootDirectory) }
}
}
27 changes: 27 additions & 0 deletions Source/SwiftLintCore/Helpers/Glob.swift
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,33 @@ struct Glob {
.map { $0.absolutePathStandardized() }
}

static func toRegex(_ pattern: String, rootPath: String = "") -> NSRegularExpression? {
var regexPattern = pattern
.replacingOccurrences(of: "**/*", with: "\0")
.replacingOccurrences(of: "**/", with: "\0")
.replacingOccurrences(of: ".", with: "\\.")
.replacingOccurrences(of: "?", with: ".")
.replacingOccurrences(of: "**", with: "\0")
.replacingOccurrences(of: "*", with: "[^/]*")
.replacingOccurrences(of: "\0", with: ".*")
.replacingOccurrences(of: "(", with: "\\(")
.replacingOccurrences(of: ")", with: "\\)")
if !pattern.starts(with: rootPath) {
regexPattern = rootPath + "/" + regexPattern
}
if !regexPattern.hasSuffix("*") {
regexPattern.append(".*")
}
regexPattern = regexPattern.replacingOccurrences(of: "//", with: "/")
guard let regex = try? NSRegularExpression.cached(pattern: "^\(regexPattern)$") else {
Issue.genericWarning("""
Pattern '\(pattern)' cannot be converted to a regular expression and is therefore ignored
""").print()
return nil
}
return regex
}

// MARK: Private

private static func expandGlobstar(pattern: String) -> [String] {
Expand Down
7 changes: 3 additions & 4 deletions Source/swiftlint/Extensions/Configuration+CommandLine.swift
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ extension Configuration {
return filterExcludedPathsByPrefix(in: scriptInputPaths)
.map(SwiftLintFile.init(pathDeferringReading:))
} else {
return filterExcludedPaths(excludedPaths(), in: scriptInputPaths)
return filterExcludedPaths(in: scriptInputPaths)
.map(SwiftLintFile.init(pathDeferringReading:))
}
}
Expand All @@ -247,9 +247,8 @@ extension Configuration {
self.lintableFiles(
inPath: $0,
forceExclude: visitor.forceExclude,
excludeBy: visitor.useExcludingByPrefix
? .prefix
: .paths(excludedPaths: excludedPaths()))
excludeByPrefix: visitor.useExcludingByPrefix
)
}
}

Expand Down
4 changes: 2 additions & 2 deletions Tests/IntegrationTests/IntegrationTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ class IntegrationTests: SwiftLintTestCase {
let swiftFiles = config.lintableFiles(
inPath: "",
forceExclude: false,
excludeBy: .paths(excludedPaths: config.excludedPaths()))
excludeByPrefix: false)
XCTAssert(
swiftFiles.contains(where: { #file.bridge().absolutePathRepresentation() == $0.path }),
"current file should be included"
Expand All @@ -41,7 +41,7 @@ class IntegrationTests: SwiftLintTestCase {
let swiftFiles = config.lintableFiles(
inPath: "",
forceExclude: false,
excludeBy: .paths(excludedPaths: config.excludedPaths()))
excludeByPrefix: false)
let storage = RuleStorage()
let corrections = swiftFiles.parallelFlatMap {
Linter(file: $0, configuration: config).collect(into: storage).correct(using: storage)
Expand Down
35 changes: 14 additions & 21 deletions Tests/SwiftLintFrameworkTests/ConfigurationTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -256,21 +256,19 @@ class ConfigurationTests: SwiftLintTestCase {
excludedPaths: ["directory/excluded",
"directory/ExcludedFile.swift"])

let excludedPaths = configuration.excludedPaths(fileManager: fileManager)
let paths = configuration.lintablePaths(inPath: "",
forceExclude: false,
excludeBy: .paths(excludedPaths: excludedPaths),
excludeByPrefix: false,
fileManager: fileManager)
XCTAssertEqual(["directory/File1.swift", "directory/File2.swift"].absolutePathsStandardized(), paths)
}

func testForceExcludesFile() {
let fileManager = TestFileManager()
let configuration = Configuration(excludedPaths: ["directory/ExcludedFile.swift"])
let excludedPaths = configuration.excludedPaths(fileManager: fileManager)
let paths = configuration.lintablePaths(inPath: "directory/ExcludedFile.swift",
forceExclude: true,
excludeBy: .paths(excludedPaths: excludedPaths),
excludeByPrefix: false,
fileManager: fileManager)
XCTAssertEqual([], paths)
}
Expand All @@ -279,41 +277,37 @@ class ConfigurationTests: SwiftLintTestCase {
let fileManager = TestFileManager()
let configuration = Configuration(includedPaths: ["directory"],
excludedPaths: ["directory/ExcludedFile.swift", "directory/excluded"])
let excludedPaths = configuration.excludedPaths(fileManager: fileManager)
let paths = configuration.lintablePaths(inPath: "",
forceExclude: true,
excludeBy: .paths(excludedPaths: excludedPaths),
excludeByPrefix: false,
fileManager: fileManager)
XCTAssertEqual(["directory/File1.swift", "directory/File2.swift"].absolutePathsStandardized(), paths)
}

func testForceExcludesDirectory() {
let fileManager = TestFileManager()
let configuration = Configuration(excludedPaths: ["directory/excluded", "directory/ExcludedFile.swift"])
let excludedPaths = configuration.excludedPaths(fileManager: fileManager)
let paths = configuration.lintablePaths(inPath: "directory",
forceExclude: true,
excludeBy: .paths(excludedPaths: excludedPaths),
excludeByPrefix: false,
fileManager: fileManager)
XCTAssertEqual(["directory/File1.swift", "directory/File2.swift"].absolutePathsStandardized(), paths)
}

func testForceExcludesDirectoryThatIsNotInExcludedButHasChildrenThatAre() {
let fileManager = TestFileManager()
let configuration = Configuration(excludedPaths: ["directory/excluded", "directory/ExcludedFile.swift"])
let excludedPaths = configuration.excludedPaths(fileManager: fileManager)
let paths = configuration.lintablePaths(inPath: "directory",
forceExclude: true,
excludeBy: .paths(excludedPaths: excludedPaths),
excludeByPrefix: false,
fileManager: fileManager)
XCTAssertEqual(["directory/File1.swift", "directory/File2.swift"].absolutePathsStandardized(), paths)
}

func testLintablePaths() {
let excluded = Configuration.default.excludedPaths(fileManager: TestFileManager())
let paths = Configuration.default.lintablePaths(inPath: Mock.Dir.level0,
forceExclude: false,
excludeBy: .paths(excludedPaths: excluded))
excludeByPrefix: false)
let filenames = paths.map { $0.bridge().lastPathComponent }.sorted()
let expectedFilenames = [
"DirectoryLevel1.swift",
Expand All @@ -329,7 +323,7 @@ class ConfigurationTests: SwiftLintTestCase {
let configuration = Configuration(includedPaths: ["**/Level2"])
let paths = configuration.lintablePaths(inPath: Mock.Dir.level0,
forceExclude: true,
excludeBy: .paths(excludedPaths: configuration.excludedPaths))
excludeByPrefix: false)
let filenames = paths.map { $0.bridge().lastPathComponent }.sorted()
let expectedFilenames = ["Level2.swift", "Level3.swift"]

Expand All @@ -342,10 +336,9 @@ class ConfigurationTests: SwiftLintTestCase {
excludedPaths: [Mock.Dir.level3.stringByAppendingPathComponent("*.swift")]
)

let excludedPaths = configuration.excludedPaths()
let lintablePaths = configuration.lintablePaths(inPath: "",
forceExclude: false,
excludeBy: .paths(excludedPaths: excludedPaths))
excludeByPrefix: false)
XCTAssertEqual(lintablePaths, [])
}

Expand Down Expand Up @@ -432,7 +425,7 @@ extension ConfigurationTests {
"Level1/Level2/Level3"])
let paths = configuration.lintablePaths(inPath: Mock.Dir.level0,
forceExclude: false,
excludeBy: .prefix)
excludeByPrefix: true)
let filenames = paths.map { $0.bridge().lastPathComponent }
XCTAssertEqual(filenames, ["Level2.swift"])
}
Expand All @@ -442,7 +435,7 @@ extension ConfigurationTests {
let configuration = Configuration(excludedPaths: ["Level1/Level2/Level3/Level3.swift"])
let paths = configuration.lintablePaths(inPath: "Level1/Level2/Level3/Level3.swift",
forceExclude: true,
excludeBy: .prefix)
excludeByPrefix: true)
XCTAssertEqual([], paths)
}

Expand All @@ -452,7 +445,7 @@ extension ConfigurationTests {
excludedPaths: ["Level1/Level1.swift"])
let paths = configuration.lintablePaths(inPath: "Level1",
forceExclude: true,
excludeBy: .prefix)
excludeByPrefix: true)
let filenames = paths.map { $0.bridge().lastPathComponent }.sorted()
XCTAssertEqual(["Level2.swift", "Level3.swift"], filenames)
}
Expand All @@ -466,7 +459,7 @@ extension ConfigurationTests {
)
let paths = configuration.lintablePaths(inPath: ".",
forceExclude: true,
excludeBy: .prefix)
excludeByPrefix: true)
let filenames = paths.map { $0.bridge().lastPathComponent }.sorted()
XCTAssertEqual(["Level0.swift", "Level1.swift"], filenames)
}
Expand All @@ -480,7 +473,7 @@ extension ConfigurationTests {
)
let paths = configuration.lintablePaths(inPath: ".",
forceExclude: true,
excludeBy: .prefix)
excludeByPrefix: true)
let filenames = paths.map { $0.bridge().lastPathComponent }
XCTAssertEqual(["Level0.swift"], filenames)
}
Expand All @@ -492,7 +485,7 @@ extension ConfigurationTests {
excludedPaths: ["Level1/*/*.swift", "Level1/*/*/*.swift"])
let paths = configuration.lintablePaths(inPath: "Level1",
forceExclude: false,
excludeBy: .prefix)
excludeByPrefix: true)
let filenames = paths.map { $0.bridge().lastPathComponent }.sorted()
XCTAssertEqual(filenames, ["Level1.swift"])
}
Expand Down
15 changes: 15 additions & 0 deletions Tests/SwiftLintFrameworkTests/GlobTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -82,4 +82,19 @@ final class GlobTests: SwiftLintTestCase {
let files = Glob.resolveGlob(mockPath.stringByAppendingPathComponent("**/*.swift"))
XCTAssertEqual(files.sorted(), expectedFiles.sorted())
}

func testToRegex() {
XCTAssertEqual(Glob.toRegex("*?.(**)")?.pattern, "^[^/]*.\\.\\(.*\\).*$")
XCTAssertEqual(Glob.toRegex("a/b/c")?.pattern, "^a/b/c.*$")
XCTAssertEqual(Glob.toRegex("a//b")?.pattern, "^a/b.*$")
XCTAssertEqual(Glob.toRegex("/a/b/*")?.pattern, "^/a/b/[^/]*$")
XCTAssertEqual(Glob.toRegex("a/*.swift")?.pattern, "^a/[^/]*\\.swift.*$")
XCTAssertEqual(Glob.toRegex("**/a/*.swift")?.pattern, "^.*a/[^/]*\\.swift.*$")
XCTAssertEqual(Glob.toRegex("**/*.swift")?.pattern, "^.*\\.swift.*$")

XCTAssertEqual(Glob.toRegex("a/b", rootPath: "/tmp/")?.pattern, "^/tmp/a/b.*$")
XCTAssertEqual(Glob.toRegex("/tmp/a/b", rootPath: "/tmp/")?.pattern, "^/tmp/a/b.*$")
XCTAssertEqual(Glob.toRegex("/a/b", rootPath: "/tmp")?.pattern, "^/tmp/a/b.*$")
XCTAssertEqual(Glob.toRegex("**/*.swift", rootPath: "/tmp")?.pattern, "^/tmp/.*\\.swift.*$")
}
}

0 comments on commit 4094558

Please sign in to comment.