Skip to content

Commit

Permalink
Revert "Improve performance of excluded files filter (#5157)"
Browse files Browse the repository at this point in the history
This reverts commit 152355e.

# Conflicts:
#	tools/oss-check
  • Loading branch information
SimplyDanny committed Jan 14, 2025
1 parent 9b22cda commit db71104
Show file tree
Hide file tree
Showing 13 changed files with 97 additions and 132 deletions.
1 change: 0 additions & 1 deletion BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,6 @@ swift_library(
":SourceKittenFramework.wrapper",
"@sourcekitten_com_github_jpsim_yams//:Yams",
"@swiftlint_com_github_scottrhoyt_swifty_text_table//:SwiftyTextTable",
"@com_github_ileitch_swift-filename-matcher//:FilenameMatcher"
] + select({
"@platforms//os:linux": ["@com_github_krzyzanowskim_cryptoswift//:CryptoSwift"],
"//conditions:default": [":DyldWarningWorkaround"],
Expand Down
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,12 @@
[SimplyDanny](https://github.com/SimplyDanny)
[#5954](https://github.com/realm/SwiftLint/issues/5954)

* Revert changes to improve performance when exclude patterns resolve to a large set of files. While resolving files
indeed got much faster in certain setups, it leads to missed exclusions for nested configurations and when the linted
folder is not the current folder.
[SimplyDanny](https://github.com/SimplyDanny)
[#5953](https://github.com/realm/SwiftLint/issues/5953)

#### Experimental

* None.
Expand Down
1 change: 0 additions & 1 deletion MODULE.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ bazel_dep(name = "sourcekitten", version = "0.36.0", repo_name = "com_github_jps
bazel_dep(name = "swift-syntax", version = "600.0.0", repo_name = "SwiftSyntax")
bazel_dep(name = "swift_argument_parser", version = "1.3.1.1", repo_name = "sourcekitten_com_github_apple_swift_argument_parser")
bazel_dep(name = "yams", version = "5.1.3", repo_name = "sourcekitten_com_github_jpsim_yams")
bazel_dep(name = "swift-filename-matcher", version = "2.0.0", repo_name = "com_github_ileitch_swift-filename-matcher")

swiftlint_repos = use_extension("//bazel:repos.bzl", "swiftlint_repos_bzlmod")
use_repo(
Expand Down
9 changes: 0 additions & 9 deletions Package.resolved
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,6 @@
"version" : "1.3.1"
}
},
{
"identity" : "swift-filename-matcher",
"kind" : "remoteSourceControl",
"location" : "https://github.com/ileitch/swift-filename-matcher",
"state" : {
"revision" : "516ff95f6a06c7a9eff8e944e989c7af076c5cdb",
"version" : "2.0.0"
}
},
{
"identity" : "swift-syntax",
"kind" : "remoteSourceControl",
Expand Down
2 changes: 0 additions & 2 deletions Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ let package = Package(
.package(url: "https://github.com/scottrhoyt/SwiftyTextTable.git", from: "0.9.0"),
.package(url: "https://github.com/JohnSundell/CollectionConcurrencyKit.git", from: "0.2.0"),
.package(url: "https://github.com/krzyzanowskim/CryptoSwift.git", .upToNextMinor(from: "1.8.4")),
.package(url: "https://github.com/ileitch/swift-filename-matcher", .upToNextMinor(from: "2.0.0")),
],
targets: [
.executableTarget(
Expand Down Expand Up @@ -92,7 +91,6 @@ let package = Package(
.product(name: "SwiftSyntaxBuilder", package: "swift-syntax"),
.product(name: "SwiftyTextTable", package: "SwiftyTextTable"),
.product(name: "Yams", package: "Yams"),
.product(name: "FilenameMatcher", package: "swift-filename-matcher"),
"SwiftLintCoreMacros",
],
swiftSettings: swiftFeatures + strictConcurrency
Expand Down
9 changes: 0 additions & 9 deletions Source/SwiftLintCore/Extensions/String+SwiftLint.swift
Original file line number Diff line number Diff line change
Expand Up @@ -68,20 +68,11 @@ public extension String {

/// Returns a new string, converting the path to a canonical absolute path.
///
/// > Important: This method might use an incorrect working directory internally. This can cause test failures
/// in Bazel builds but does not seem to cause trouble in production.
///
/// - returns: A new `String`.
func absolutePathStandardized() -> String {
bridge().absolutePathRepresentation().bridge().standardizingPath
}

/// Like ``absolutePathStandardized()`` but with the working directory that's used everywhere else.
var normalized: String {
let cwd = FileManager.default.currentDirectoryPath.bridge().standardizingPath
return bridge().absolutePathRepresentation(rootDirectory: cwd)
}

var isFile: Bool {
if self.isEmpty {
return false
Expand Down
23 changes: 14 additions & 9 deletions Source/SwiftLintFramework/Configuration+CommandLine.swift
Original file line number Diff line number Diff line change
Expand Up @@ -257,12 +257,15 @@ extension Configuration {
guard options.forceExclude else {
return files
}

let scriptInputPaths = files.compactMap(\.path)
return (
visitor.options.useExcludingByPrefix
? filterExcludedPathsByPrefix(in: scriptInputPaths)
: filterExcludedPaths(in: scriptInputPaths)
).map(SwiftLintFile.init(pathDeferringReading:))

if options.useExcludingByPrefix {
return filterExcludedPathsByPrefix(in: scriptInputPaths)
.map(SwiftLintFile.init(pathDeferringReading:))
}
return filterExcludedPaths(excludedPaths(), in: scriptInputPaths)
.map(SwiftLintFile.init(pathDeferringReading:))
}
if !options.quiet {
let filesInfo: String
Expand All @@ -274,12 +277,14 @@ extension Configuration {

queuedPrintError("\(options.capitalizedVerb) Swift files \(filesInfo)")
}
return visitor.options.paths.flatMap {
let excludeLintableFilesBy = options.useExcludingByPrefix
? Configuration.ExcludeBy.prefix
: .paths(excludedPaths: excludedPaths())
return options.paths.flatMap {
self.lintableFiles(
inPath: $0,
forceExclude: visitor.options.forceExclude,
excludeByPrefix: visitor.options.useExcludingByPrefix
)
forceExclude: options.forceExclude,
excludeBy: excludeLintableFilesBy)
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,21 +1,25 @@
import Foundation

extension Configuration {
// MARK: Lintable Paths
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 it uses the exclude-by-prefix algorithm.
/// - parameter excludeByPrefix: Whether or not uses excluding by prefix algorithm.
///
/// - returns: Files to lint.
public func lintableFiles(inPath path: String,
forceExclude: Bool,
excludeByPrefix: Bool) -> [SwiftLintFile] {
lintablePaths(inPath: path, forceExclude: forceExclude, excludeByPrefix: excludeByPrefix)
excludeBy: ExcludeBy) -> [SwiftLintFile] {
lintablePaths(inPath: path, forceExclude: forceExclude, excludeBy: excludeBy)
.compactMap(SwiftLintFile.init(pathDeferringReading:))
}

Expand All @@ -25,21 +29,24 @@ 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 it uses the exclude-by-prefix algorithm.
/// - parameter excludeByPrefix: Whether or not uses excluding 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,
excludeByPrefix: Bool,
excludeBy: ExcludeBy,
fileManager: some LintableFileManager = FileManager.default
) -> [String] {
if fileManager.isFile(atPath: path) {
if forceExclude {
return excludeByPrefix
? filterExcludedPathsByPrefix(in: [path.normalized])
: filterExcludedPaths(in: [path.normalized])
switch excludeBy {
case .prefix:
return filterExcludedPathsByPrefix(in: [path.absolutePathStandardized()])
case .paths(let excludedPaths):
return filterExcludedPaths(excludedPaths, in: [path.absolutePathStandardized()])
}
}
// If path is a file and we're not forcing excludes, skip filtering with excluded/included paths
return [path]
Expand All @@ -50,29 +57,35 @@ extension Configuration {
.flatMap(Glob.resolveGlob)
.parallelFlatMap { fileManager.filesToLint(inPath: $0, rootDirectory: rootDirectory) }

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

/// Returns an array of file paths after removing the excluded paths as defined by this configuration.
///
/// - parameter paths: The input paths to filter.
/// - parameter fileManager: The lintable file manager to use to expand the excluded paths into all matching paths.
/// - parameter paths: The input paths to filter.
///
/// - returns: The input paths after removing the excluded paths.
public func filterExcludedPaths(in paths: [String]) -> [String] {
public func filterExcludedPaths(
_ excludedPaths: [String],
in paths: [String]...
) -> [String] {
let allPaths = paths.flatMap { $0 }
#if os(Linux)
let result = NSMutableOrderedSet(capacity: paths.count)
result.addObjects(from: paths)
let result = NSMutableOrderedSet(capacity: allPaths.count)
result.addObjects(from: allPaths)
#else
let result = NSMutableOrderedSet(array: paths)
let result = NSMutableOrderedSet(array: allPaths)
#endif
let exclusionPatterns = self.excludedPaths.flatMap {
Glob.createFilenameMatchers(root: rootDirectory, pattern: $0)
}
return result.array
.parallelCompactMap { exclusionPatterns.anyMatch(filename: $0 as! String) ? nil : $0 as? String }
// swiftlint:disable:previous force_cast

result.minusSet(Set(excludedPaths))
// swiftlint:disable:next force_cast
return result.map { $0 as! String }
}

/// Returns the file paths that are excluded by this configuration using filtering by absolute path prefix.
Expand All @@ -81,12 +94,25 @@ extension Configuration {
/// algorithm `filterExcludedPaths`.
///
/// - returns: The input paths after removing the excluded paths.
public func filterExcludedPathsByPrefix(in paths: [String]) -> [String] {
public func filterExcludedPathsByPrefix(in paths: [String]...) -> [String] {
let allPaths = paths.flatMap { $0 }
let excludedPaths = self.excludedPaths
.parallelFlatMap { Glob.resolveGlob($0) }
.map(\.normalized)
return paths.filter { path in
.parallelFlatMap { @Sendable in Glob.resolveGlob($0) }
.map { $0.absolutePathStandardized() }
return allPaths.filter { path in
!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: some LintableFileManager = FileManager.default) -> [String] {
excludedPaths
.flatMap(Glob.resolveGlob)
.parallelFlatMap { fileManager.filesToLint(inPath: $0, rootDirectory: rootDirectory) }
}
}
23 changes: 0 additions & 23 deletions Source/SwiftLintFramework/Helpers/Glob.swift
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import FilenameMatcher
import Foundation

#if os(Linux)
Expand Down Expand Up @@ -37,28 +36,6 @@ struct Glob {
.map { $0.absolutePathStandardized() }
}

static func createFilenameMatchers(root: String, pattern: String) -> [FilenameMatcher] {
var absolutPathPattern = pattern
if !pattern.starts(with: root) {
// If the root is not already part of the pattern, prepend it.
absolutPathPattern = root + (root.hasSuffix("/") ? "" : "/") + absolutPathPattern
}
if pattern.hasSuffix(".swift") || pattern.hasSuffix("/**") {
// Suffix is already well defined.
return [FilenameMatcher(pattern: absolutPathPattern)]
}
if pattern.hasSuffix("/") {
// Matching all files in the folder.
return [FilenameMatcher(pattern: absolutPathPattern + "**")]
}
// The pattern could match files in the last folder in the path or all contained files if the last component
// represents folders.
return [
FilenameMatcher(pattern: absolutPathPattern),
FilenameMatcher(pattern: absolutPathPattern + "/**"),
]
}

// MARK: Private

private static func expandGlobstar(pattern: String) -> [String] {
Expand Down
Loading

0 comments on commit db71104

Please sign in to comment.