From 96db41c37913588fda9ae7cb541c02d9ef79ab40 Mon Sep 17 00:00:00 2001 From: Martin Redington Date: Wed, 1 May 2024 15:55:33 +0100 Subject: [PATCH] Add an experimental baseline feature (#5475) --- CHANGELOG.md | 8 +- Source/SwiftLintCore/Models/Baseline.swift | 182 +++++++++++++++ Source/SwiftLintCore/Models/Issue.swift | 9 +- .../SwiftLintCore/Models/StyleViolation.swift | 6 +- Source/swiftlint/Commands/Analyze.swift | 2 + Source/swiftlint/Commands/Baseline.swift | 88 +++++++ Source/swiftlint/Commands/Lint.swift | 2 + Source/swiftlint/Commands/SwiftLint.swift | 1 + .../Helpers/LintOrAnalyzeArguments.swift | 4 + .../Helpers/LintOrAnalyzeCommand.swift | 37 ++- .../BaselineTests.swift | 221 ++++++++++++++++++ 11 files changed, 551 insertions(+), 9 deletions(-) create mode 100644 Source/SwiftLintCore/Models/Baseline.swift create mode 100644 Source/swiftlint/Commands/Baseline.swift create mode 100644 Tests/SwiftLintFrameworkTests/BaselineTests.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index d9949e3025..ed5a4a1b3c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,7 +23,13 @@ #### Experimental -* None. +* Add two new options to the `lint` and `analyze` commands: `--write-baseline` + to save a baseline to disk, and `--baseline` to read a saved baseline and + use it to filter out detected pre-existing violations. A new `baseline` + command uses the reporters to print the violations in a baseline. + [Martin Redington](https://github.com/mildm8nnered) + [#5475](https://github.com/realm/SwiftLint/pull/5475) + [#3421](https://github.com/realm/SwiftLint/pull/3421) #### Enhancements diff --git a/Source/SwiftLintCore/Models/Baseline.swift b/Source/SwiftLintCore/Models/Baseline.swift new file mode 100644 index 0000000000..8e99891575 --- /dev/null +++ b/Source/SwiftLintCore/Models/Baseline.swift @@ -0,0 +1,182 @@ +import Foundation + +private typealias BaselineViolations = [BaselineViolation] +private typealias ViolationsPerFile = [String: BaselineViolations] +private typealias ViolationsPerRule = [String: BaselineViolations] + +private struct BaselineViolation: Codable, Hashable { + let violation: StyleViolation + let text: String + var key: String { text + violation.reason + violation.severity.rawValue } + + init(violation: StyleViolation, text: String) { + let location = violation.location + self.violation = violation.with(location: Location( + // Within the baseline, we use relative paths, so that + // comparisons are independent of the absolute path + file: location.relativeFile, + line: location.line, + character: location.character) + ) + self.text = text + } +} + +/// A set of violations that can be used to filter newly detected violations. +public struct Baseline: Equatable { + private let baseline: ViolationsPerFile + private var sortedBaselineViolations: BaselineViolations { + baseline.sorted(by: { $0.key < $1.key }).flatMap(\.value) + } + + /// The stored violations. + public var violations: [StyleViolation] { + sortedBaselineViolations.violationsWithAbsolutePaths + } + + /// Creates a `Baseline` from a saved file. + /// + /// - parameter fromPath: The path to read from. + public init(fromPath path: String) throws { + let data = try Data(contentsOf: URL(fileURLWithPath: path)) + baseline = try JSONDecoder().decode(BaselineViolations.self, from: data).groupedByFile() + } + + /// Creates a `Baseline` from a list of violations. + /// + /// - parameter violations: The violations for the baseline. + public init(violations: [StyleViolation]) { + baseline = BaselineViolations(violations).groupedByFile() + } + + /// Writes a `Baseline` to disk in JSON format. + /// + /// - parameter toPath: The path to write to. + public func write(toPath path: String) throws { + let data = try JSONEncoder().encode(sortedBaselineViolations) + try data.write(to: URL(fileURLWithPath: path)) + } + + /// Filters out violations that are present in the `Baseline`. + /// + /// Assumes that all violations are from the same file. + /// + /// - parameter violations: The violations to filter. + /// - Returns: The new violations. + public func filter(_ violations: [StyleViolation]) -> [StyleViolation] { + guard let firstViolation = violations.first, + let baselineViolations = baseline[firstViolation.location.relativeFile ?? ""], + baselineViolations.isNotEmpty else { + return violations + } + + let relativePathViolations = BaselineViolations(violations) + if relativePathViolations == baselineViolations { + return [] + } + + let violationsByRuleIdentifier = relativePathViolations.groupedByRuleIdentifier( + filteredBy: baselineViolations + ) + let baselineViolationsByRuleIdentifier = baselineViolations.groupedByRuleIdentifier( + filteredBy: relativePathViolations + ) + + var filteredViolations: Set = [] + + for (ruleIdentifier, ruleViolations) in violationsByRuleIdentifier { + guard + let baselineViolations = baselineViolationsByRuleIdentifier[ruleIdentifier], + baselineViolations.isNotEmpty else { + filteredViolations.formUnion(ruleViolations) + continue + } + + let groupedRuleViolations = Dictionary(grouping: ruleViolations, by: \.key) + let groupedBaselineViolations = Dictionary(grouping: baselineViolations, by: \.key) + + for (key, ruleViolations) in groupedRuleViolations { + guard let baselineViolations = groupedBaselineViolations[key] else { + filteredViolations.formUnion(ruleViolations) + continue + } + if ruleViolations.count > baselineViolations.count { + filteredViolations.formUnion(ruleViolations) + } + } + } + + let violationsWithAbsolutePaths = Set(filteredViolations.violationsWithAbsolutePaths) + return violations.filter { violationsWithAbsolutePaths.contains($0) } + } + + /// Returns the violations that are present in another `Baseline`, but not in this one. + /// + /// The violations are filtered using the same algorithm as the `filter` method above. + /// + /// - parameter otherBaseline: The other `Baseline`. + public func compare(_ otherBaseline: Baseline) -> [StyleViolation] { + otherBaseline.baseline.flatMap { + filter($1.violationsWithAbsolutePaths) + } + } +} + +private struct LineCache { + private var lines: [String: [String]] = [:] + + mutating func text(at location: Location) -> String { + let line = (location.line ?? 0) - 1 + if line > 0, let file = location.file, let content = cached(file: file), line < content.count { + return content[line] + } + return "" + } + + private mutating func cached(file: String) -> [String]? { + if let fileLines = lines[file] { + return fileLines + } + if let fileLines = SwiftLintFile(path: file)?.lines.map(\.content) { + lines[file] = fileLines + return fileLines + } + return nil + } +} + +private extension Sequence where Element == BaselineViolation { + init(_ violations: [StyleViolation]) where Self == BaselineViolations { + var lineCache = LineCache() + self = violations.map { $0.baselineViolation(text: lineCache.text(at: $0.location)) } + } + + var violationsWithAbsolutePaths: [StyleViolation] { + map { $0.violation.withAbsolutePath } + } + + func groupedByFile() -> ViolationsPerFile { + Dictionary(grouping: self) { $0.violation.location.relativeFile ?? "" } + } + + func groupedByRuleIdentifier(filteredBy existingViolations: [BaselineViolation] = []) -> ViolationsPerRule { + Dictionary(grouping: Set(self).subtracting(existingViolations), by: \.violation.ruleIdentifier) + } +} + +private extension StyleViolation { + var withAbsolutePath: StyleViolation { + let absolutePath: String? = + if let relativePath = location.file { + FileManager.default.currentDirectoryPath + "/" + relativePath + } else { + nil + } + let newLocation = Location(file: absolutePath, line: location.line, character: location.character) + return with(location: newLocation) + } + + func baselineViolation(text: String = "") -> BaselineViolation { + BaselineViolation(violation: self, text: text) + } +} diff --git a/Source/SwiftLintCore/Models/Issue.swift b/Source/SwiftLintCore/Models/Issue.swift index b7ee200f28..1e9cc6ef59 100644 --- a/Source/SwiftLintCore/Models/Issue.swift +++ b/Source/SwiftLintCore/Models/Issue.swift @@ -71,6 +71,9 @@ public enum Issue: LocalizedError, Equatable { /// An error that occurred when parsing YAML. case yamlParsing(String) + /// The baseline file at `path` is not readable or cannot be opened. + case baselineNotReadable(path: String) + /// Flag to enable warnings for deprecations being printed to the console. Printing is enabled by default. public static var printDeprecationWarnings = true @@ -172,6 +175,8 @@ public enum Issue: LocalizedError, Equatable { """ case let .initialFileNotFound(path): return "Could not read file at path '\(path)'." + case let .fileNotFound(path): + return "File at path '\(path)' not found." case let .fileNotReadable(path, id): return "Cannot open or read file at path '\(path ?? "...")' within '\(id)' rule." case let .fileNotWritable(path): @@ -187,8 +192,8 @@ public enum Issue: LocalizedError, Equatable { return "Cannot get cursor info from file at path '\(path ?? "...")' within '\(id)' rule." case let .yamlParsing(message): return "Cannot parse YAML file: \(message)" - case let .fileNotFound(path): - return "File at path '\(path)' not found." + case let .baselineNotReadable(path): + return "Cannot open or read the baseline file at path '\(path)'." } } } diff --git a/Source/SwiftLintCore/Models/StyleViolation.swift b/Source/SwiftLintCore/Models/StyleViolation.swift index c243e08d14..627f7460e9 100644 --- a/Source/SwiftLintCore/Models/StyleViolation.swift +++ b/Source/SwiftLintCore/Models/StyleViolation.swift @@ -1,5 +1,5 @@ /// A value describing an instance of Swift source code that is considered invalid by a SwiftLint rule. -public struct StyleViolation: CustomStringConvertible, Equatable, Codable { +public struct StyleViolation: CustomStringConvertible, Codable, Hashable { /// The identifier of the rule that generated this violation. public let ruleIdentifier: String @@ -66,4 +66,8 @@ public struct StyleViolation: CustomStringConvertible, Equatable, Codable { new.location = location return new } + + public func hash(into hasher: inout Hasher) { + hasher.combine(description) + } } diff --git a/Source/swiftlint/Commands/Analyze.swift b/Source/swiftlint/Commands/Analyze.swift index 9e41eea340..ac74e32f3f 100644 --- a/Source/swiftlint/Commands/Analyze.swift +++ b/Source/swiftlint/Commands/Analyze.swift @@ -43,6 +43,8 @@ extension SwiftLint { useScriptInputFiles: common.useScriptInputFiles, benchmark: common.benchmark, reporter: common.reporter, + baseline: common.baseline, + writeBaseline: common.writeBaseline, quiet: quiet, output: common.output, progress: common.progress, diff --git a/Source/swiftlint/Commands/Baseline.swift b/Source/swiftlint/Commands/Baseline.swift new file mode 100644 index 0000000000..a58b889c87 --- /dev/null +++ b/Source/swiftlint/Commands/Baseline.swift @@ -0,0 +1,88 @@ +import ArgumentParser +import Foundation +import SwiftLintFramework + +extension SwiftLint { + struct Baseline: ParsableCommand { + static let configuration = CommandConfiguration( + abstract: "Operations on existing baselines", + subcommands: [Report.self, Compare.self], + defaultSubcommand: Report.self + ) + } + + private struct BaselineOptions: ParsableArguments { + @Argument(help: "The path to the baseline file.") + var baseline: String + } + + private struct ReportingOptions: ParsableArguments { + @Option( + help: """ + The reporter used to report violations. The 'summary' reporter can be useful to \ + provide an overview. + """ + ) + var reporter: String? + @Option(help: "The file where violations should be saved. Prints to stdout by default.") + var output: String? + } + + private struct Report: ParsableCommand { + static let configuration = CommandConfiguration(abstract: "Reports the violations in a baseline.") + + @OptionGroup + var options: BaselineOptions + @OptionGroup + var reportingOptions: ReportingOptions + + func run() throws { + let savedBaseline = try SwiftLintCore.Baseline(fromPath: options.baseline) + try report(savedBaseline.violations, using: reportingOptions.reporter, to: reportingOptions.output) + ExitHelper.successfullyExit() + } + } + + private struct Compare: ParsableCommand { + static let configuration = CommandConfiguration( + abstract: "Reports the violations that are present in another baseline " + + "but not in the original baseline." + ) + + @OptionGroup + var options: BaselineOptions + @Option( + help: """ + The path to a second baseline to compare against the baseline. Violations in \ + the second baseline that are not present in the original baseline will be reported. + """ + ) + var otherBaseline: String + @OptionGroup + var reportingOptions: ReportingOptions + + func run() throws { + let baseline = try SwiftLintCore.Baseline(fromPath: options.baseline) + let otherBaseline = try SwiftLintCore.Baseline(fromPath: otherBaseline) + try report(baseline.compare(otherBaseline), using: reportingOptions.reporter, to: reportingOptions.output) + ExitHelper.successfullyExit() + } + } +} + +private func report(_ violations: [StyleViolation], using reporterIdentifier: String?, to output: String?) throws { + let reporter = reporterFrom(identifier: reporterIdentifier) + let report = reporter.generateReport(violations) + if report.isNotEmpty { + if let output { + let data = Data((report + "\n").utf8) + do { + try data.write(to: URL(fileURLWithPath: output)) + } catch { + Issue.fileNotWritable(path: output).print() + } + } else { + queuedPrint(report) + } + } +} diff --git a/Source/swiftlint/Commands/Lint.swift b/Source/swiftlint/Commands/Lint.swift index 7a26830b91..e3ec05f07a 100644 --- a/Source/swiftlint/Commands/Lint.swift +++ b/Source/swiftlint/Commands/Lint.swift @@ -59,6 +59,8 @@ extension SwiftLint { useScriptInputFiles: common.useScriptInputFiles, benchmark: common.benchmark, reporter: common.reporter, + baseline: common.baseline, + writeBaseline: common.writeBaseline, quiet: quiet, output: common.output, progress: common.progress, diff --git a/Source/swiftlint/Commands/SwiftLint.swift b/Source/swiftlint/Commands/SwiftLint.swift index dd86138e7e..8229530b18 100644 --- a/Source/swiftlint/Commands/SwiftLint.swift +++ b/Source/swiftlint/Commands/SwiftLint.swift @@ -24,6 +24,7 @@ struct SwiftLint: AsyncParsableCommand { Docs.self, GenerateDocs.self, Lint.self, + Baseline.self, Reporters.self, Rules.self, Version.self diff --git a/Source/swiftlint/Helpers/LintOrAnalyzeArguments.swift b/Source/swiftlint/Helpers/LintOrAnalyzeArguments.swift index e8e039d983..e7ea89cf05 100644 --- a/Source/swiftlint/Helpers/LintOrAnalyzeArguments.swift +++ b/Source/swiftlint/Helpers/LintOrAnalyzeArguments.swift @@ -38,6 +38,10 @@ struct LintOrAnalyzeArguments: ParsableArguments { var benchmark = false @Option(help: "The reporter used to log errors and warnings.") var reporter: String? + @Option(help: "The path to a baseline file, which will be used to filter out detected violations.") + var baseline: String? + @Option(help: "The path to save detected violations to as a new baseline.") + var writeBaseline: String? @Flag(help: "Use the in-process version of SourceKit.") var inProcessSourcekit = false @Option(help: "The file where violations should be saved. Prints to stdout by default.") diff --git a/Source/swiftlint/Helpers/LintOrAnalyzeCommand.swift b/Source/swiftlint/Helpers/LintOrAnalyzeCommand.swift index da717523c7..3c5e946b84 100644 --- a/Source/swiftlint/Helpers/LintOrAnalyzeCommand.swift +++ b/Source/swiftlint/Helpers/LintOrAnalyzeCommand.swift @@ -44,6 +44,9 @@ struct LintOrAnalyzeCommand { private static func lintOrAnalyze(_ options: LintOrAnalyzeOptions) async throws { let builder = LintOrAnalyzeResultBuilder(options) let files = try await collectViolations(builder: builder) + if let baselineOutputPath = options.writeBaseline { + try Baseline(violations: builder.unfilteredViolations).write(toPath: baselineOutputPath) + } try Signposts.record(name: "LintOrAnalyzeCommand.PostProcessViolations") { try postProcessViolations(files: files, builder: builder) } @@ -52,6 +55,7 @@ struct LintOrAnalyzeCommand { private static func collectViolations(builder: LintOrAnalyzeResultBuilder) async throws -> [SwiftLintFile] { let options = builder.options let visitorMutationQueue = DispatchQueue(label: "io.realm.swiftlint.lintVisitorMutation") + let baseline = try baseline(options) return try await builder.configuration.visitLintableFiles(options: options, cache: builder.cache, storage: builder.storage) { linter in let currentViolations: [StyleViolation] @@ -68,7 +72,6 @@ struct LintOrAnalyzeCommand { visitorMutationQueue.sync { builder.fileBenchmark.record(file: linter.file, from: start) currentRuleTimes.forEach { builder.ruleBenchmark.record(id: $0, time: $1) } - builder.violations += currentViolations } } else { currentViolations = applyLeniency( @@ -76,12 +79,15 @@ struct LintOrAnalyzeCommand { strict: builder.configuration.strict, violations: linter.styleViolations(using: builder.storage) ) - visitorMutationQueue.sync { - builder.violations += currentViolations - } } + let filteredViolations = baseline?.filter(currentViolations) ?? currentViolations + visitorMutationQueue.sync { + builder.unfilteredViolations += currentViolations + builder.violations += filteredViolations + } + linter.file.invalidateCache() - builder.report(violations: currentViolations, realtimeCondition: true) + builder.report(violations: filteredViolations, realtimeCondition: true) } } @@ -115,6 +121,22 @@ struct LintOrAnalyzeCommand { guard numberOfSeriousViolations == 0 else { exit(2) } } + private static func baseline(_ options: LintOrAnalyzeOptions) throws -> Baseline? { + if let baselinePath = options.baseline { + do { + return try Baseline(fromPath: baselinePath) + } catch { + Issue.baselineNotReadable(path: baselinePath).print() + if + (error as? CocoaError)?.code != CocoaError.fileReadNoSuchFile || + options.writeBaseline != options.baseline { + throw error + } + } + } + return nil + } + private static func printStatus(violations: [StyleViolation], files: [SwiftLintFile], serious: Int, verb: String) { let pluralSuffix = { (collection: [Any]) -> String in return collection.count != 1 ? "s" : "" @@ -237,6 +259,8 @@ struct LintOrAnalyzeOptions { let useScriptInputFiles: Bool let benchmark: Bool let reporter: String? + let baseline: String? + let writeBaseline: String? let quiet: Bool let output: String? let progress: Bool @@ -260,6 +284,9 @@ struct LintOrAnalyzeOptions { private class LintOrAnalyzeResultBuilder { var fileBenchmark = Benchmark(name: "files") var ruleBenchmark = Benchmark(name: "rules") + /// All detected violations, unfiltered by the baseline, if any. + var unfilteredViolations = [StyleViolation]() + /// The violations to be reported, possibly filtered by a baseline, plus any threshold violations. var violations = [StyleViolation]() let storage = RuleStorage() let configuration: Configuration diff --git a/Tests/SwiftLintFrameworkTests/BaselineTests.swift b/Tests/SwiftLintFrameworkTests/BaselineTests.swift new file mode 100644 index 0000000000..8d38050308 --- /dev/null +++ b/Tests/SwiftLintFrameworkTests/BaselineTests.swift @@ -0,0 +1,221 @@ +@testable import SwiftLintBuiltInRules +@testable import SwiftLintCore +import XCTest + +private var temporaryDirectoryPath: String { + let result = URL( + fileURLWithPath: NSTemporaryDirectory(), + isDirectory: true + ).path + +#if os(macOS) + return "/private" + result +#else + return result +#endif +} + +final class BaselineTests: XCTestCase { + private static let example = """ + import Foundation + import SwiftLintFramework + + class Example: NSObject { + private var foo: Int + private var bar: String + + init(foo: Int, bar: String) { + self.foo = foo + self.bar = bar + } // init + func someFunction() -> Int { + foo * 10 + } // someFunction + func someOtherFunction() -> String { + bar + } // someOtherFunction + func yetAnotherFunction() -> (Int, String) { + (foo, bar) + } // yetAnotherFunction + } + """ + + private static let ruleDescriptions = [ + ArrayInitRule.description, + BlockBasedKVORule.description, + ClosingBraceRule.description, + DirectReturnRule.description + ] + + private static var currentDirectoryPath: String? + + private static func violations(for filePath: String?) -> [StyleViolation] { + ruleDescriptions.violations(for: filePath) + } + + private static func baseline(for filePath: String) -> Baseline { + Baseline(violations: ruleDescriptions.violations(for: filePath)) + } + + override static func setUp() { + super.setUp() + currentDirectoryPath = FileManager.default.currentDirectoryPath + XCTAssertTrue(FileManager.default.changeCurrentDirectoryPath(temporaryDirectoryPath)) + } + + override static func tearDown() { + if let currentDirectoryPath { + XCTAssertTrue(FileManager.default.changeCurrentDirectoryPath(currentDirectoryPath)) + self.currentDirectoryPath = nil + } + super.tearDown() + } + + func testWritingAndReading() throws { + try withExampleFileCreated { sourceFilePath in + let baselinePath = temporaryDirectoryPath.stringByAppendingPathComponent(UUID().uuidString) + try Baseline(violations: Self.violations(for: sourceFilePath)).write(toPath: baselinePath) + defer { + try? FileManager.default.removeItem(atPath: baselinePath) + } + let newBaseline = try Baseline(fromPath: baselinePath) + XCTAssertEqual(newBaseline, Self.baseline(for: sourceFilePath)) + } + } + + func testUnchangedViolations() throws { + try withExampleFileCreated { sourceFilePath in + XCTAssertEqual(Self.baseline(for: sourceFilePath).filter(Self.violations(for: sourceFilePath)), []) + } + } + + func testShiftedViolations() throws { + try withExampleFileCreated { sourceFilePath in + let baseline = Self.baseline(for: sourceFilePath) + let violations = try Self.violations(for: sourceFilePath).lineShifted(by: 2, path: sourceFilePath) + XCTAssertEqual(baseline.filter(violations), []) + } + } + + func testNewViolation() throws { + try testViolationDetection( + violationRuleDescriptions: Self.ruleDescriptions, + newViolationRuleDescription: EmptyCollectionLiteralRule.description, + insertionIndex: 2 + ) + } + + func testViolationDetection() throws { + let violationRuleDescriptions = [ + ArrayInitRule.description, + BlockBasedKVORule.description, + ArrayInitRule.description, + ClosingBraceRule.description, + ClosingBraceRule.description, + ClosingBraceRule.description, + BlockBasedKVORule.description, + DirectReturnRule.description, + ArrayInitRule.description, + ClosingBraceRule.description + ] + + let ruleDescriptions = [ + ArrayInitRule.description, + BlockBasedKVORule.description, + ClosingBraceRule.description, + DirectReturnRule.description + ] + + for ruleDescription in ruleDescriptions { + for insertionIndex in 0.. Void) throws { + let sourceFilePath = temporaryDirectoryPath.stringByAppendingPathComponent("\(UUID().uuidString).swift") + guard let data = Self.example.data(using: .utf8) else { + XCTFail("Could not convert example code to data using UTF-8 encoding") + return + } + try data.write(to: URL(fileURLWithPath: sourceFilePath)) + defer { + try? FileManager.default.removeItem(atPath: sourceFilePath) + } + try block(sourceFilePath) + } +} + +private extension [StyleViolation] { + func lineShifted(by shift: Int, path: String) throws -> [StyleViolation] { + guard shift > 0 else { + XCTFail("Shift must be positive") + return self + } + var lines = SwiftLintFile(path: path)?.lines.map({ $0.content }) ?? [] + lines = [String](repeating: "", count: shift) + lines + if let data = lines.joined(separator: "\n").data(using: .utf8) { + try data.write(to: URL(fileURLWithPath: path)) + } + return map { + let shiftedLocation = Location( + file: path, + line: $0.location.line != nil ? $0.location.line! + shift : nil, + character: $0.location.character + ) + return $0.with(location: shiftedLocation) + } + } +} + +private extension Sequence where Element == RuleDescription { + func violations(for filePath: String?) -> [StyleViolation] { + enumerated().map { index, ruleDescription in + StyleViolation( + ruleDescription: ruleDescription, + location: Location(file: filePath, line: (index + 1) * 2, character: 1) + ) + } + } +}