-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Cache implementation #1073
Cache implementation #1073
Changes from 20 commits
c698d02
a92a24c
f1e3376
b1f62e2
9d42a73
a47385a
77e4249
34af12a
3d37624
c7461e0
b006a28
1ef461b
a2ea9ca
b3cd500
a56dba7
b9eb54a
119c214
e8ead88
8af6552
caee985
0cf4338
903f43b
b1b9027
f89068e
20620d5
1d4d04d
05d0a31
5e30935
e1e2369
94f48f4
b7b905c
c28e308
66d0bb2
2a97a1c
0530a23
4a9806f
86214a7
41251d8
b189da0
5702ef3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,7 @@ private enum ConfigurationKey: String { | |
case useNestedConfigs = "use_nested_configs" // deprecated | ||
case whitelistRules = "whitelist_rules" | ||
case warningThreshold = "warning_threshold" | ||
case cachePath = "cache_path" | ||
} | ||
|
||
public struct Configuration: Equatable { | ||
|
@@ -32,6 +33,8 @@ public struct Configuration: Equatable { | |
public let rules: [Rule] | ||
public var rootPath: String? // the root path to search for nested configurations | ||
public var configurationPath: String? // if successfully loaded from a path | ||
public var hash: Int? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not make Would require naming this variable as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mainly because I thought it'd be too much work to make There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about its configuration? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, ok thinking more about it, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, to make sure we're in the same page, do you still think we should make There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. |
||
public let cachePath: String? | ||
|
||
public init?(disabledRules: [String] = [], | ||
optInRules: [String] = [], | ||
|
@@ -41,11 +44,12 @@ public struct Configuration: Equatable { | |
warningThreshold: Int? = nil, | ||
reporter: String = XcodeReporter.identifier, | ||
ruleList: RuleList = masterRuleList, | ||
configuredRules: [Rule]? = nil) { | ||
|
||
configuredRules: [Rule]? = nil, | ||
cachePath: String? = nil) { | ||
self.included = included | ||
self.excluded = excluded | ||
self.reporter = reporter | ||
self.cachePath = cachePath | ||
|
||
let configuredRules = configuredRules | ||
?? (try? ruleList.configuredRules(with: [:])) | ||
|
@@ -134,7 +138,8 @@ public struct Configuration: Equatable { | |
reporter: dict[ConfigurationKey.reporter.rawValue] as? String ?? | ||
XcodeReporter.identifier, | ||
ruleList: ruleList, | ||
configuredRules: configuredRules) | ||
configuredRules: configuredRules, | ||
cachePath: dict[ConfigurationKey.cachePath.rawValue] as? String) | ||
} | ||
|
||
public init(path: String = Configuration.fileName, rootPath: String? = nil, | ||
|
@@ -157,6 +162,7 @@ public struct Configuration: Equatable { | |
} | ||
self.init(dict: dict)! | ||
configurationPath = fullPath | ||
hash = yamlContents.hash | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. instead of hashing the yaml contents, we could hash the properties of this struct, meaning that changes to the configuration file like reordering items or adding comments wouldn't invalidate the cache. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this would also allow There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Forget the bit about conforming to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done in c28e308, but had to bridge to |
||
self.rootPath = rootPath | ||
return | ||
} catch YamlParserError.yamlParsing(let message) { | ||
|
@@ -242,7 +248,8 @@ private func validKeys(ruleList: RuleList) -> [String] { | |
.reporter, | ||
.useNestedConfigs, | ||
.warningThreshold, | ||
.whitelistRules | ||
.whitelistRules, | ||
.cachePath | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This list is also otherwise alphabetized. |
||
].map({ $0.rawValue }) + ruleList.allValidIdentifiers() | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,6 +54,7 @@ extension Rule { | |
public struct Linter { | ||
public let file: File | ||
private let rules: [Rule] | ||
private let cache: LinterCache? | ||
|
||
public var styleViolations: [StyleViolation] { | ||
return getStyleViolations().0 | ||
|
@@ -64,6 +65,11 @@ public struct Linter { | |
} | ||
|
||
private func getStyleViolations(_ benchmark: Bool = false) -> ([StyleViolation], [(id: String, time: Double)]) { | ||
|
||
if let cached = cachedStyleViolations(benchmark) { | ||
return cached | ||
} | ||
|
||
if file.sourcekitdFailed { | ||
queuedPrintError("Most rules will be skipped because sourcekitd has failed.") | ||
} | ||
|
@@ -78,6 +84,11 @@ public struct Linter { | |
deprecatedToValidIdentifier[key] = value | ||
} | ||
|
||
if let cache = cache, let file = file.path { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
let hash = self.file.contents.hash | ||
cache.cacheFile(file, violations: violations, hash: hash) | ||
} | ||
|
||
for (deprecatedIdentifier, identifier) in deprecatedToValidIdentifier { | ||
queuedPrintError("'\(deprecatedIdentifier)' rule has been renamed to '\(identifier)' and will be " + | ||
"completely removed in a future release.") | ||
|
@@ -86,8 +97,34 @@ public struct Linter { | |
return (violations, ruleTimes) | ||
} | ||
|
||
public init(file: File, configuration: Configuration = Configuration()!) { | ||
private func cachedStyleViolations(_ benchmark: Bool = false) -> ([StyleViolation], [(id: String, time: Double)])? { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
let start: Date! = benchmark ? Date() : nil | ||
guard let cache = cache, | ||
let file = file.path, | ||
case let hash = self.file.contents.hash, | ||
let cachedViolations = cache.violations(for: file, hash: hash) else { | ||
return nil | ||
} | ||
|
||
var ruleTimes = [(id: String, time: Double)]() | ||
if benchmark { | ||
// let's assume that all rules should have the same duration and split the duration among them | ||
let totalTime = -start.timeIntervalSinceNow | ||
let fractionedTime = totalTime / TimeInterval(rules.count) | ||
ruleTimes = rules.flatMap { rule in | ||
let id = type(of: rule).description.identifier | ||
return (id, fractionedTime) | ||
} | ||
} | ||
|
||
return (cachedViolations, ruleTimes) | ||
} | ||
|
||
public init(file: File, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this declaration doesn't need to be broken up in multiple lines. |
||
configuration: Configuration = Configuration()!, | ||
cache: LinterCache? = nil) { | ||
self.file = file | ||
self.cache = cache | ||
rules = configuration.rules | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,125 @@ | ||
// | ||
// LinterCache.swift | ||
// SwiftLint | ||
// | ||
// Created by Marcelo Fabri on 12/27/16. | ||
// Copyright © 2016 Realm. All rights reserved. | ||
// | ||
|
||
import Foundation | ||
import SourceKittenFramework | ||
|
||
public enum LinterCacheError: Error { | ||
case invalidFormat | ||
case differentVersion | ||
case differentConfiguration | ||
} | ||
|
||
public final class LinterCache { | ||
private var cache: [String: Any] | ||
private let lock = NSLock() | ||
|
||
public init(currentVersion: Version = .current, configurationHash: Int? = nil) { | ||
cache = [String: Any]() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not just this? cache = [
"version": currentVersion.value,
"configuration_hash: configurationHash,
"files": [:]
] |
||
cache["version"] = currentVersion.value | ||
cache["configuration_hash"] = configurationHash | ||
cache["files"] = [:] | ||
} | ||
|
||
public init(cache: Any, currentVersion: Version = .current, configurationHash: Int? = nil) throws { | ||
guard let dictionary = cache as? [String: Any] else { | ||
throw LinterCacheError.invalidFormat | ||
} | ||
|
||
guard let version = dictionary["version"] as? String, version == currentVersion.value else { | ||
throw LinterCacheError.differentVersion | ||
} | ||
|
||
if dictionary["configuration_hash"] as? Int != configurationHash { | ||
throw LinterCacheError.differentConfiguration | ||
} | ||
|
||
self.cache = dictionary | ||
} | ||
|
||
public convenience init(contentsOf url: URL, currentVersion: Version = .current, | ||
configurationHash: Int? = nil) throws { | ||
let data = try Data(contentsOf: url) | ||
let json = try JSONSerialization.jsonObject(with: data, options: []) | ||
try self.init(cache: json, currentVersion: currentVersion, | ||
configurationHash: configurationHash) | ||
} | ||
|
||
public func cacheFile(_ file: String, violations: [StyleViolation], hash: Int) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't conform to Swift 3 API Design Guidelines, and implementation could be simplified: public func cache(violations: [StyleViolation], forFile file: String, fileHash: Int) {
lock.lock()
var filesCache = (cache["files"] as? [String: Any]) ?? [:]
filesCache[file] = [
"violations": violations.map(dictionaryForViolation),
"hash": fileHash
]
cache["files"] = filesCache
lock.unlock()
} Adds an insignificant amount of work inside the lock. |
||
var entry = [String: Any]() | ||
var fileViolations = entry["violations"] as? [[String: Any]] ?? [] | ||
|
||
for violation in violations { | ||
fileViolations.append(dictionaryForViolation(violation)) | ||
} | ||
|
||
entry["violations"] = fileViolations | ||
entry["hash"] = hash | ||
|
||
lock.lock() | ||
var filesCache = (cache["files"] as? [String: Any]) ?? [:] | ||
filesCache[file] = entry | ||
cache["files"] = filesCache | ||
lock.unlock() | ||
} | ||
|
||
public func violations(for file: String, hash: Int) -> [StyleViolation]? { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. weakly typed argument. Label should be |
||
lock.lock() | ||
|
||
guard let filesCache = cache["files"] as? [String: Any], | ||
let entry = filesCache[file] as? [String: Any], | ||
let cacheHash = entry["hash"] as? Int, | ||
cacheHash == hash, | ||
let violations = entry["violations"] as? [[String: Any]] else { | ||
lock.unlock() | ||
return nil | ||
} | ||
|
||
lock.unlock() | ||
return violations.flatMap { StyleViolation.fromCache($0, file: file) } | ||
} | ||
|
||
public func save(to url: URL) throws { | ||
lock.lock() | ||
let json = toJSON(cache) | ||
lock.unlock() | ||
try json.write(to: url, atomically: true, encoding: .utf8) | ||
} | ||
|
||
private func dictionaryForViolation(_ violation: StyleViolation) -> [String: Any] { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Swift 3 API Design Guidelines: |
||
return [ | ||
"line": violation.location.line ?? NSNull() as Any, | ||
"character": violation.location.character ?? NSNull() as Any, | ||
"severity": violation.severity.rawValue, | ||
"type": violation.ruleDescription.name, | ||
"rule_id": violation.ruleDescription.identifier, | ||
"reason": violation.reason | ||
] | ||
} | ||
} | ||
|
||
extension StyleViolation { | ||
fileprivate static func fromCache(_ cache: [String: Any], file: String) -> StyleViolation? { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Swift 3 API Design Guidelines: |
||
guard let severity = (cache["severity"] as? String).flatMap(ViolationSeverity.init(identifier:)), | ||
let name = cache["type"] as? String, | ||
let ruleId = cache["rule_id"] as? String, | ||
let reason = cache["reason"] as? String else { | ||
return nil | ||
} | ||
|
||
let line = cache["line"] as? Int | ||
let character = cache["character"] as? Int | ||
|
||
let ruleDescription = RuleDescription(identifier: ruleId, name: name, description: reason) | ||
let location = Location(file: file, line: line, character: character) | ||
let violation = StyleViolation(ruleDescription: ruleDescription, severity: severity, | ||
location: location, reason: reason) | ||
|
||
return violation | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
// | ||
// Version.swift | ||
// SwiftLint | ||
// | ||
// Created by Marcelo Fabri on 27/12/16. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📆 🇺🇸 🔥 |
||
// Copyright © 2016 Realm. All rights reserved. | ||
// | ||
|
||
import Foundation | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This whole file should just be The template idea that's rendered into this file via make is 👌 though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 for not reading from the bundle, but I think it'd be better if we kept this namespaced inside a |
||
|
||
public struct Version { | ||
public let value: String | ||
|
||
public static let current: Version = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't you do |
||
if let value = Bundle(identifier: "io.realm.SwiftLintFramework")? | ||
.object(forInfoDictionaryKey: "CFBundleShortVersionString") as? String { | ||
return Version(value: value) | ||
} | ||
|
||
return Version(value: "0.15.0") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should be reverted or done in a way that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in caee985 |
||
}() | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,10 @@ | |
public enum ViolationSeverity: String, Comparable { | ||
case warning | ||
case error | ||
|
||
public init?(identifier: String) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why this init when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was having issues with the |
||
self.init(rawValue: identifier) | ||
} | ||
} | ||
|
||
// MARK: Comparable | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,8 +23,9 @@ struct LintCommand: CommandProtocol { | |
var violations = [StyleViolation]() | ||
let configuration = Configuration(options: options) | ||
let reporter = reporterFrom(options: options, configuration: configuration) | ||
let cache = LinterCache.makeCache(options: options, configuration: configuration) | ||
let visitorMutationQueue = DispatchQueue(label: "io.realm.swiftlint.lintVisitorMutation") | ||
return configuration.visitLintableFiles(options) { linter in | ||
return configuration.visitLintableFiles(options, cache: cache) { linter in | ||
let currentViolations: [StyleViolation] | ||
if options.benchmark { | ||
let start = Date() | ||
|
@@ -58,6 +59,9 @@ struct LintCommand: CommandProtocol { | |
fileBenchmark.save() | ||
ruleBenchmark.save() | ||
} | ||
|
||
cache?.save(options: options, configuration: configuration) | ||
|
||
return LintCommand.successOrExit(numberOfSeriousViolations: numberOfSeriousViolations, | ||
strictWithViolations: options.strict && !violations.isEmpty) | ||
} | ||
|
@@ -113,12 +117,14 @@ struct LintOptions: OptionsProtocol { | |
let benchmark: Bool | ||
let reporter: String | ||
let quiet: Bool | ||
let cachePath: String | ||
let ignoreCache: Bool | ||
|
||
// swiftlint:disable line_length | ||
static func create(_ path: String) -> (_ useSTDIN: Bool) -> (_ configurationFile: String) -> (_ strict: Bool) -> (_ useScriptInputFiles: Bool) -> (_ benchmark: Bool) -> (_ reporter: String) -> (_ quiet: Bool) -> LintOptions { | ||
return { useSTDIN in { configurationFile in { strict in { useScriptInputFiles in { benchmark in { reporter in { quiet in | ||
self.init(path: path, useSTDIN: useSTDIN, configurationFile: configurationFile, strict: strict, useScriptInputFiles: useScriptInputFiles, benchmark: benchmark, reporter: reporter, quiet: quiet) | ||
}}}}}}} | ||
static func create(_ path: String) -> (_ useSTDIN: Bool) -> (_ configurationFile: String) -> (_ strict: Bool) -> (_ useScriptInputFiles: Bool) -> (_ benchmark: Bool) -> (_ reporter: String) -> (_ quiet: Bool) -> (_ cachePath: String) -> (_ ignoreCache: Bool) -> LintOptions { | ||
return { useSTDIN in { configurationFile in { strict in { useScriptInputFiles in { benchmark in { reporter in { quiet in { cachePath in { ignoreCache in | ||
self.init(path: path, useSTDIN: useSTDIN, configurationFile: configurationFile, strict: strict, useScriptInputFiles: useScriptInputFiles, benchmark: benchmark, reporter: reporter, quiet: quiet, cachePath: cachePath, ignoreCache: ignoreCache) | ||
}}}}}}}}} | ||
} | ||
|
||
static func evaluate(_ mode: CommandMode) -> Result<LintOptions, CommandantError<CommandantError<()>>> { | ||
|
@@ -137,5 +143,9 @@ struct LintOptions: OptionsProtocol { | |
<*> mode <| Option(key: "reporter", defaultValue: "", | ||
usage: "the reporter used to log errors and warnings") | ||
<*> mode <| quietOption(action: "linting") | ||
<*> mode <| Option(key: "cache-path", defaultValue: "", | ||
usage: "the cache that should be used when linting") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "the location of the cache used when linting" |
||
<*> mode <| Option(key: "no-cache", defaultValue: false, | ||
usage: "ignore cache when linting") | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,9 +9,9 @@ | |
import Commandant | ||
import Foundation | ||
import Result | ||
import SwiftLintFramework | ||
|
||
private let version = Bundle(identifier: "io.realm.SwiftLintFramework")! | ||
.object(forInfoDictionaryKey: "CFBundleShortVersionString")! | ||
private let version = Version.current.value | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can just use |
||
|
||
struct VersionCommand: CommandProtocol { | ||
let verb = "version" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the rest of this list is sorted