Skip to content
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

Started to add rationales #5681

Draft
wants to merge 20 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,23 @@ struct AnonymousArgumentInMultilineClosureRule: OptInRule {
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 }"),
Expand Down
Original file line number Diff line number Diff line change
@@ -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<Self>(.warning)

Expand All @@ -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,
Expand Down
Original file line number Diff line number Diff line change
@@ -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<Self>(.warning)

Expand All @@ -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,
Expand Down
31 changes: 31 additions & 0 deletions Source/SwiftLintBuiltInRules/Rules/Lint/ArrayInitRule.swift
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,37 @@ struct ArrayInitRule: OptInRule, @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<String, MyError> = .success("")
let result: Result<Any, MyError> = 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)"),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,39 @@ struct BalancedXCTestLifecycleRule: OptInRule {
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).

The memory model for XCTestCase objects is non-obvious. An instance of the `XCTestCase` \
subclass will be created for each test method, and these will persist for the entire test run.

So in a test class with 10 tests, given

```
private var foo: String = "Bar"
```

"Bar" will be stored 10 times over, but with

```
// swiftlint:disable:next implicitly_unwrapped_optional
private var foo: String!

func setUp() {
foo = "Bar"
}

func tearDown() {
foo = nil
}
```

No memory will be consumed by the value of the variable.

More generally, if `setUp` is implemented, then `tearDown` should also be implemented, \
and cleanup performed there.
""",
kind: .lint,
nonTriggeringExamples: [
Example(#"""
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,34 @@ 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 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
```

There are some rules which only make sense in the context of the entire file, for example `file_header`, \
`file_length`, `file_name`, `file_name_no_space`, and `single_test_class`. These can be configured with \
the `allowed_rules` configuration parameter.

You can also specify rules which must always be applied to the entire file, and should never be scoped or \
re-enabled with the `always_blanket_disable` configuration parameter.
""",
kind: .lint,
nonTriggeringExamples: [
Example("""
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {}"),
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,19 @@
struct ClosureBodyLengthRule: OptInRule, SwiftSyntaxRule {
var configuration = SeverityLevelsConfiguration<Self>(warning: 30, error: 100)
private static let defaultWarningThreshold = 30
var configuration = SeverityLevelsConfiguration<Self>(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.

Many installations, including SwiftLint's, increase the default warning value from \
\(Self.defaultWarningThreshold) to 50, which is a bit more permissive.
""",
kind: .metrics,
nonTriggeringExamples: ClosureBodyLengthRuleExamples.nonTriggeringExamples,
triggeringExamples: ClosureBodyLengthRuleExamples.triggeringExamples
Expand Down
16 changes: 16 additions & 0 deletions Source/SwiftLintBuiltInRules/Rules/Style/AttributesRule.swift
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,22 @@ struct AttributesRule: OptInRule {
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
Expand Down
27 changes: 27 additions & 0 deletions Source/SwiftLintCore/Documentation/RuleDocumentation.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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 rationale = description.rationale {
content += [h2("Rationale")]
content.append(formattedRationale(rationale))
}
let nonTriggeringExamples = description.nonTriggeringExamples.filter { !$0.excludeFromDocumentation }
if nonTriggeringExamples.isNotEmpty {
content += [h2("Non Triggering Examples")]
Expand All @@ -53,6 +57,10 @@ struct RuleDocumentation {
return content.joined(separator: "\n\n")
}

private func formattedRationale(_ rationale: String) -> String {
rationale.formattedAsRationale
}

private func formattedCode(_ example: Example) -> String {
if let config = example.configuration, let configuredRule = try? ruleType.init(configuration: config) {
let configDescription = configuredRule.createConfigurationDescription(exclusiveOptions: Set(config.keys))
Expand Down Expand Up @@ -98,3 +106,22 @@ private func detailsSummary(_ rule: some Rule) -> String {
}
return ruleDescription
}

extension String {
var formattedAsRationale: String {
var insideMultilineString = false
return components(separatedBy: "\n").map { line in
if line.contains("```") {
if insideMultilineString {
insideMultilineString = false
} else {
insideMultilineString = true
if line.hasSuffix("```") {
return line + "swift"
}
}
}
return line
}.joined(separator: "\n")
}
}
15 changes: 15 additions & 0 deletions Source/SwiftLintCore/Models/RuleDescription.swift
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ 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.
public let rationale: String?

/// The `RuleKind` that best categorizes this rule.
public let kind: RuleKind

Expand Down Expand Up @@ -54,6 +59,14 @@ 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? {
guard let rationale else {
return nil
}
return rationale.formattedAsRationale
}

/// All identifiers that have been used to uniquely identify this rule in past and current SwiftLint versions.
public var allIdentifiers: [String] {
Array(deprecatedAliases) + [identifier]
Expand All @@ -74,6 +87,7 @@ public struct RuleDescription: Equatable, Sendable {
public init(identifier: String,
name: String,
description: String,
rationale: String? = nil,
kind: RuleKind,
minSwiftVersion: SwiftVersion = .five,
nonTriggeringExamples: [Example] = [],
Expand All @@ -84,6 +98,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
Expand Down
3 changes: 3 additions & 0 deletions Source/swiftlint/Commands/Rules.swift
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,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")
Expand Down
Loading