diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b15562a2..71fa4ee90 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,11 @@ function parameter can be replaced with an opaque `some` type. [SimplyDanny](https://github.com/SimplyDanny) +* Add a new rationale property to rule descriptions, providing a more expansive + description of the motivation behind each rule. + [Martin Redington](https://github.com/mildm8nnered) + [#5681](https://github.com/realm/SwiftLint/issues/5681) + ### Bug Fixes * Fix issue referencing the Tests package from another Bazel workspace. diff --git a/Source/SwiftLintBuiltInRules/Rules/Idiomatic/AnonymousArgumentInMultilineClosureRule.swift b/Source/SwiftLintBuiltInRules/Rules/Idiomatic/AnonymousArgumentInMultilineClosureRule.swift index f418e5e8c..2755e17a4 100644 --- a/Source/SwiftLintBuiltInRules/Rules/Idiomatic/AnonymousArgumentInMultilineClosureRule.swift +++ b/Source/SwiftLintBuiltInRules/Rules/Idiomatic/AnonymousArgumentInMultilineClosureRule.swift @@ -8,6 +8,23 @@ struct AnonymousArgumentInMultilineClosureRule: Rule { identifier: "anonymous_argument_in_multiline_closure", name: "Anonymous Argument in Multiline Closure", description: "Use named arguments in multiline closures", + rationale: """ + In multiline closures, for clarity, prefer using named arguments + + ``` + closure { arg in + print(arg) + } + ``` + + to anonymous arguments + + ``` + closure { + print(↓$0) + } + ``` + """, kind: .idiomatic, nonTriggeringExamples: [ Example("closure { $0 }"), diff --git a/Source/SwiftLintBuiltInRules/Rules/Lint/AccessibilityLabelForImageRule.swift b/Source/SwiftLintBuiltInRules/Rules/Lint/AccessibilityLabelForImageRule.swift index c946cef5e..57cf76b26 100644 --- a/Source/SwiftLintBuiltInRules/Rules/Lint/AccessibilityLabelForImageRule.swift +++ b/Source/SwiftLintBuiltInRules/Rules/Lint/AccessibilityLabelForImageRule.swift @@ -1,14 +1,5 @@ import SourceKittenFramework -/// In UIKit, a `UIImageView` was by default not an accessibility element, and would only be visible to VoiceOver -/// and other assistive technologies if the developer explicitly made them an accessibility element. In SwiftUI, -/// however, an `Image` is an accessibility element by default. If the developer does not explicitly hide them from -/// accessibility or give them an accessibility label, they will inherit the name of the image file, which often creates -/// a poor experience when VoiceOver reads things like "close icon white". -/// -/// Known false negatives for Images declared as instance variables and containers that provide a label but are -/// not accessibility elements. Known false positives for Images created in a separate function from where they -/// have accessibility properties applied. struct AccessibilityLabelForImageRule: ASTRule, OptInRule { var configuration = SeverityConfiguration(.warning) @@ -17,6 +8,17 @@ struct AccessibilityLabelForImageRule: ASTRule, OptInRule { name: "Accessibility Label for Image", description: "Images that provide context should have an accessibility label or should be explicitly hidden " + "from accessibility", + rationale: """ + In UIKit, a `UIImageView` was by default not an accessibility element, and would only be visible to VoiceOver \ + and other assistive technologies if the developer explicitly made them an accessibility element. In SwiftUI, \ + however, an `Image` is an accessibility element by default. If the developer does not explicitly hide them \ + from accessibility or give them an accessibility label, they will inherit the name of the image file, which \ + often creates a poor experience when VoiceOver reads things like "close icon white". + + Known false negatives for Images declared as instance variables and containers that provide a label but are \ + not accessibility elements. Known false positives for Images created in a separate function from where they \ + have accessibility properties applied. + """, kind: .lint, minSwiftVersion: .fiveDotOne, nonTriggeringExamples: AccessibilityLabelForImageRuleExamples.nonTriggeringExamples, diff --git a/Source/SwiftLintBuiltInRules/Rules/Lint/AccessibilityTraitForButtonRule.swift b/Source/SwiftLintBuiltInRules/Rules/Lint/AccessibilityTraitForButtonRule.swift index c14ffb7b8..006783a2d 100644 --- a/Source/SwiftLintBuiltInRules/Rules/Lint/AccessibilityTraitForButtonRule.swift +++ b/Source/SwiftLintBuiltInRules/Rules/Lint/AccessibilityTraitForButtonRule.swift @@ -1,12 +1,5 @@ import SourceKittenFramework -/// The accessibility button and link traits are used to tell assistive technologies that an element is tappable. When -/// an element has one of these traits, VoiceOver will automatically read "button" or "link" after the element's label -/// to let the user know that they can activate it. When using a UIKit `UIButton` or SwiftUI `Button` or -/// `Link`, the button trait is added by default, but when you manually add a tap gesture recognizer to an -/// element, you need to explicitly add the button or link trait. In most cases the button trait should be used, but for -/// buttons that open a URL in an external browser we use the link trait instead. This rule attempts to catch uses of -/// the SwiftUI `.onTapGesture` modifier where the `.isButton` or `.isLink` trait is not explicitly applied. struct AccessibilityTraitForButtonRule: ASTRule, OptInRule { var configuration = SeverityConfiguration(.warning) @@ -15,6 +8,18 @@ struct AccessibilityTraitForButtonRule: ASTRule, OptInRule { name: "Accessibility Trait for Button", description: "All views with tap gestures added should include the .isButton or the .isLink accessibility " + "traits", + rationale: """ + The accessibility button and link traits are used to tell assistive technologies that an element is tappable. \ + When an element has one of these traits, VoiceOver will automatically read "button" or "link" after the \ + element's label to let the user know that they can activate it. + + When using a UIKit `UIButton` or SwiftUI `Button` or `Link`, the button trait is added by default, but when \ + you manually add a tap gesture recognizer to an element, you need to explicitly add the button or link trait. \ + + In most cases the button trait should be used, but for buttons that open a URL in an external browser we use \ + the link trait instead. This rule attempts to catch uses of the SwiftUI `.onTapGesture` modifier where the \ + `.isButton` or `.isLink` trait is not explicitly applied. + """, kind: .lint, minSwiftVersion: .fiveDotOne, nonTriggeringExamples: AccessibilityTraitForButtonRuleExamples.nonTriggeringExamples, diff --git a/Source/SwiftLintBuiltInRules/Rules/Lint/ArrayInitRule.swift b/Source/SwiftLintBuiltInRules/Rules/Lint/ArrayInitRule.swift index d359c6340..a461e9a0e 100644 --- a/Source/SwiftLintBuiltInRules/Rules/Lint/ArrayInitRule.swift +++ b/Source/SwiftLintBuiltInRules/Rules/Lint/ArrayInitRule.swift @@ -8,6 +8,37 @@ struct ArrayInitRule: Rule, @unchecked Sendable { identifier: "array_init", name: "Array Init", description: "Prefer using `Array(seq)` over `seq.map { $0 }` to convert a sequence into an Array", + rationale: """ + When converting the elements of a sequence directly into an `Array`, for clarity, prefer using the `Array` \ + constructor over calling `map`. For example + + ``` + Array(foo) + ``` + + rather than + + ``` + foo.↓map({ $0 }) + ``` + + If some processing of the elements is required, then using `map` is fine. For example + + ``` + foo.map { !$0 } + ``` + + Constructs like + + ``` + enum MyError: Error {} + let myResult: Result = .success("") + let result: Result = myResult.map { $0 } + ``` + + may be picked up as false positives by the `array_init` rule. If your codebase contains constructs like this, \ + consider using the `typesafe_array_init` analyzer rule instead. + """, kind: .lint, nonTriggeringExamples: [ Example("Array(foo)"), diff --git a/Source/SwiftLintBuiltInRules/Rules/Lint/BalancedXCTestLifecycleRule.swift b/Source/SwiftLintBuiltInRules/Rules/Lint/BalancedXCTestLifecycleRule.swift index 0b4401664..09b912d71 100644 --- a/Source/SwiftLintBuiltInRules/Rules/Lint/BalancedXCTestLifecycleRule.swift +++ b/Source/SwiftLintBuiltInRules/Rules/Lint/BalancedXCTestLifecycleRule.swift @@ -8,6 +8,16 @@ struct BalancedXCTestLifecycleRule: Rule { identifier: "balanced_xctest_lifecycle", name: "Balanced XCTest Life Cycle", description: "Test classes must implement balanced setUp and tearDown methods", + rationale: """ + The `setUp` method of `XCTestCase` can be used to set up variables and resources before \ + each test is run (or with the `class` variant, before all tests are run). + + This rule verifies that every class with an implementation of a `setUp` method also has \ + a `tearDown` method (and vice versa). + + The `tearDown` method should be used to cleanup or reset any resources that could \ + otherwise have any effects on subsequent tests, and to free up any instance variables. + """, kind: .lint, nonTriggeringExamples: [ Example(#""" diff --git a/Source/SwiftLintBuiltInRules/Rules/Lint/BlanketDisableCommandRule.swift b/Source/SwiftLintBuiltInRules/Rules/Lint/BlanketDisableCommandRule.swift index 2a576740f..22bdd74d0 100644 --- a/Source/SwiftLintBuiltInRules/Rules/Lint/BlanketDisableCommandRule.swift +++ b/Source/SwiftLintBuiltInRules/Rules/Lint/BlanketDisableCommandRule.swift @@ -9,6 +9,27 @@ struct BlanketDisableCommandRule: Rule, SourceKitFreeRule { single line, or `swiftlint:enable` to re-enable the rules immediately after the violations \ to be ignored, instead of disabling the rule for the rest of the file. """, + rationale: """ + The intent of this rule is to prevent code like + + ``` + // swiftlint:disable force_unwrapping + let foo = bar! + ``` + + which disables the `force_unwrapping` rule for the remainder of the file, instead of just for the specific \ + violation. + + `next`, `this`, or `previous` can be used to restrict the disable command's scope to a single line, or it \ + can be re-enabled after the violations. + + To disable this rule in code you will need to do something like + + ``` + // swiftlint:disable:next blanket_disable_command + // swiftlint:disable force_unwrapping + ``` + """, kind: .lint, nonTriggeringExamples: [ Example(""" diff --git a/Source/SwiftLintBuiltInRules/Rules/Lint/ClassDelegateProtocolRule.swift b/Source/SwiftLintBuiltInRules/Rules/Lint/ClassDelegateProtocolRule.swift index ef41645d9..32cb20848 100644 --- a/Source/SwiftLintBuiltInRules/Rules/Lint/ClassDelegateProtocolRule.swift +++ b/Source/SwiftLintBuiltInRules/Rules/Lint/ClassDelegateProtocolRule.swift @@ -8,6 +8,24 @@ struct ClassDelegateProtocolRule: Rule { identifier: "class_delegate_protocol", name: "Class Delegate Protocol", description: "Delegate protocols should be class-only so they can be weakly referenced", + rationale: """ + Delegate protocols are usually `weak` to avoid retain cycles, or bad references to deallocated delegates. + + The `weak` operator is only supported for classes, and so this rule enforces that protocols ending in \ + "Delegate" are class based. + + For example + + ``` + protocol FooDelegate: class {} + ``` + + versus + + ``` + ↓protocol FooDelegate {} + ``` + """, kind: .lint, nonTriggeringExamples: [ Example("protocol FooDelegate: class {}"), diff --git a/Source/SwiftLintBuiltInRules/Rules/Metrics/ClosureBodyLengthRule.swift b/Source/SwiftLintBuiltInRules/Rules/Metrics/ClosureBodyLengthRule.swift index b230e57e4..5e322e518 100644 --- a/Source/SwiftLintBuiltInRules/Rules/Metrics/ClosureBodyLengthRule.swift +++ b/Source/SwiftLintBuiltInRules/Rules/Metrics/ClosureBodyLengthRule.swift @@ -1,10 +1,16 @@ struct ClosureBodyLengthRule: OptInRule, SwiftSyntaxRule { - var configuration = SeverityLevelsConfiguration(warning: 30, error: 100) + private static let defaultWarningThreshold = 30 + var configuration = SeverityLevelsConfiguration(warning: Self.defaultWarningThreshold, error: 100) static let description = RuleDescription( identifier: "closure_body_length", name: "Closure Body Length", description: "Closure bodies should not span too many lines", + rationale: """ + "Closure bodies should not span too many lines" says it all. + + Possibly you could refactor your closure code and extract some of it into a function. + """, kind: .metrics, nonTriggeringExamples: ClosureBodyLengthRuleExamples.nonTriggeringExamples, triggeringExamples: ClosureBodyLengthRuleExamples.triggeringExamples diff --git a/Source/SwiftLintBuiltInRules/Rules/Style/AttributesRule.swift b/Source/SwiftLintBuiltInRules/Rules/Style/AttributesRule.swift index 95fce3a98..be3ad3b7d 100644 --- a/Source/SwiftLintBuiltInRules/Rules/Style/AttributesRule.swift +++ b/Source/SwiftLintBuiltInRules/Rules/Style/AttributesRule.swift @@ -11,6 +11,22 @@ struct AttributesRule: Rule { Attributes should be on their own lines in functions and types, but on the same line as variables and \ imports """, + rationale: """ + Erica Sadun says: + + > My take on things after the poll and after talking directly with a number of \ + developers is this: Placing attributes like `@objc`, `@testable`, `@available`, `@discardableResult` on \ + their own lines before a member declaration has become a conventional Swift style. + + > This approach limits declaration length. It allows a member to float below its attribute and supports \ + flush-left access modifiers, so `internal`, `public`, etc appear in the leftmost column. Many developers \ + mix-and-match styles for short Swift attributes like `@objc` + + See https://ericasadun.com/2016/10/02/quick-style-survey/ for discussion. + + SwiftLint's rule requires attributes to be on their own lines for functions and types, but on the same line \ + for variables and imports. + """, kind: .style, nonTriggeringExamples: AttributesRuleExamples.nonTriggeringExamples, triggeringExamples: AttributesRuleExamples.triggeringExamples diff --git a/Source/SwiftLintCore/Models/RuleDescription.swift b/Source/SwiftLintCore/Models/RuleDescription.swift index 4df2960ea..ae5f9565a 100644 --- a/Source/SwiftLintCore/Models/RuleDescription.swift +++ b/Source/SwiftLintCore/Models/RuleDescription.swift @@ -11,6 +11,12 @@ public struct RuleDescription: Equatable, Sendable { /// explanation of the rule's purpose and rationale. public let description: String + /// A longer explanation of the rule's purpose and rationale. Typically defined as a multiline string, long text + /// lines should be wrapped. Markdown formatting is supported. Multiline code blocks will be formatted as + /// `swift` code unless otherwise specified, and will automatically be indented by four spaces when printed + /// to the console. + public let rationale: String? + /// The `RuleKind` that best categorizes this rule. public let kind: RuleKind @@ -54,6 +60,16 @@ public struct RuleDescription: Equatable, Sendable { /// The console-printable string for this description. public var consoleDescription: String { "\(name) (\(identifier)): \(description)" } + /// The console-printable rationale for this description. + public var consoleRationale: String? { + rationale?.consoleRationale + } + + /// The rationale for this description, with Markdown formatting. + public var formattedRationale: String? { + rationale?.formattedRationale + } + /// All identifiers that have been used to uniquely identify this rule in past and current SwiftLint versions. public var allIdentifiers: [String] { Array(deprecatedAliases) + [identifier] @@ -74,6 +90,7 @@ public struct RuleDescription: Equatable, Sendable { public init(identifier: String, name: String, description: String, + rationale: String? = nil, kind: RuleKind, minSwiftVersion: SwiftVersion = .five, nonTriggeringExamples: [Example] = [], @@ -84,6 +101,7 @@ public struct RuleDescription: Equatable, Sendable { self.identifier = identifier self.name = name self.description = description + self.rationale = rationale self.kind = kind self.nonTriggeringExamples = nonTriggeringExamples self.triggeringExamples = triggeringExamples @@ -99,3 +117,30 @@ public struct RuleDescription: Equatable, Sendable { lhs.identifier == rhs.identifier } } + +private extension String { + var formattedRationale: String { + formattedRationale(forConsole: false) + } + + var consoleRationale: String { + formattedRationale(forConsole: true) + } + + private func formattedRationale(forConsole: Bool) -> String { + var insideMultilineString = false + return components(separatedBy: "\n").compactMap { line -> String? in + if line.contains("```") { + if insideMultilineString { + insideMultilineString = false + return forConsole ? nil : line + } + insideMultilineString = true + if line.hasSuffix("```") { + return forConsole ? nil : (line + "swift") + } + } + return line.indent(by: (insideMultilineString && forConsole) ? 4 : 0) + }.joined(separator: "\n") + } +} diff --git a/Source/SwiftLintFramework/Documentation/RuleDocumentation.swift b/Source/SwiftLintFramework/Documentation/RuleDocumentation.swift index 00dd8f35e..e80d72a43 100644 --- a/Source/SwiftLintFramework/Documentation/RuleDocumentation.swift +++ b/Source/SwiftLintFramework/Documentation/RuleDocumentation.swift @@ -40,6 +40,10 @@ struct RuleDocumentation { var fileContents: String { let description = ruleType.description var content = [h1(description.name), description.description, detailsSummary(ruleType.init())] + if let formattedRationale = description.formattedRationale { + content += [h2("Rationale")] + content.append(formattedRationale) + } let nonTriggeringExamples = description.nonTriggeringExamples.filter { !$0.excludeFromDocumentation } if nonTriggeringExamples.isNotEmpty { content += [h2("Non Triggering Examples")] diff --git a/Source/swiftlint/Commands/Rules.swift b/Source/swiftlint/Commands/Rules.swift index 930b41e31..9184305a1 100644 --- a/Source/swiftlint/Commands/Rules.swift +++ b/Source/swiftlint/Commands/Rules.swift @@ -58,6 +58,9 @@ extension SwiftLint { } print("\(description.consoleDescription)") + if let consoleRationale = description.consoleRationale { + print("\nRationale:\n\n\(consoleRationale)") + } let configDescription = rule.createConfigurationDescription() if configDescription.hasContent { print("\nConfiguration (YAML):\n")