Add unused_import config options to require imports for each module used #3123

jpsim · 2020-02-18T21:03:03Z

Continued from #3117

For example, if CGFloat is used in a file where only UIKit is imported but not CoreGraphics, this will be a violation even if the file previously compiled.

This is because Swift allows referencing some declarations that are only transitively imported.

Enabling the require_explicit_imports configuration option will require that the module of every declaration referenced in a source file be explicitly imported.

This will add significant noise to the imports list, but has a few advantages:

It will be easier to understand all the dependencies explicitly referenced in a source file.
Correcting the unused_import rule will no longer introduce compilation errors in files that compiled prior to the correction.

If missing imports are added to a file when correcting it, the sorted_imports rule will be automatically run on that file.

If you with to allow some imports to be implicitly importable transitively, you may specify the allowed_transitive_imports configuration:

unused_import:
  require_explicit_imports: true
  allowed_transitive_imports:
    - module: Foundation
      allowed_transitive_imports:
        - CoreFoundation
        - Darwin
        - ObjectiveC

For example, if `CGFloat` is used in a file where only `UIKit` is imported but not `CoreGraphics`, this will be a violation even if the file previously compiled. This is because Swift allows referencing some declarations that are only transitively imported. Enabling the `require_explicit_imports` configuration option will require that the module of every declaration referenced in a source file be explicitly imported. This will add significant noise to the imports list, but has a few advantages: 1. It will be easier to understand all the dependencies explicitly referenced in a source file. 2. Correcting the `unused_import` rule will no longer introduce compilation errors in files that compiled prior to the correction. Missing imports will always be added to the top of the file, which may cause `sorted_imports` violations. You may run SwiftLint autocorrect with the `sorted_imports` rule enabled to re-sort those import statements afterwards.

jpsim · 2020-02-18T21:12:10Z

I want to add tests and a changelog entry before merging this.

SwiftLintBot · 2020-02-18T21:43:42Z

	1 Warning
⚠️	Big PR

	12 Messages
📖	Linting Aerial with this PR took 1.29s vs 1.27s on master (1% slower)
📖	Linting Alamofire with this PR took 2.14s vs 2.13s on master (0% slower)
📖	Linting Firefox with this PR took 9.0s vs 8.88s on master (1% slower)
📖	Linting Kickstarter with this PR took 13.78s vs 13.74s on master (0% slower)
📖	Linting Moya with this PR took 1.3s vs 1.25s on master (4% slower)
📖	Linting Nimble with this PR took 1.34s vs 1.35s on master (0% faster)
📖	Linting Quick with this PR took 0.53s vs 0.53s on master (0% slower)
📖	Linting Realm with this PR took 2.31s vs 2.31s on master (0% slower)
📖	Linting SourceKitten with this PR took 1.0s vs 1.0s on master (0% slower)
📖	Linting Sourcery with this PR took 6.59s vs 6.61s on master (0% faster)
📖	Linting Swift with this PR took 12.28s vs 12.34s on master (0% faster)
📖	Linting WordPress with this PR took 14.97s vs 14.91s on master (0% slower)

Generated by 🚫 Danger

jpsim · 2020-02-19T01:05:48Z

Oh Swift on Linux 🤦‍♂

error: use of undeclared type 'CFData'
typealias Foo = CFData
                ^~~~~~

will fix tomorrow

jpsim · 2020-02-19T06:29:56Z

@ZevEisenberg @marcelofabri @PaulTaykalo I thought you all might be interested in the enhancements to Example included in this PR.

We can now use the AutomaticTestableRule protocol & associated codegen with multiple configurations. Previously it was necessary to create new test cases manually when testing custom configurations for rules.

PaulTaykalo

LGTM

PaulTaykalo · 2020-02-19T06:45:55Z

Source/SwiftLintFramework/Rules/Lint/UnusedImportRuleExamples.swift

+        typealias Foo = CFData
+        """, configuration: [
+            "require_explicit_imports": true
+        ])


I already wrote it somewhere. there' s an idea to add a comment to the Example structure so that
it will explain why trigger should be triggered :) Totally out of the scope of this PR

Example(""" ↓import Foundation typealias Foo = CFData """, configuration: [ "require_explicit_imports": true ],comment: "Using transitive type 'CFData' from CoreFoundation should require CoreFoundation import")

This example is pretty obvious, but there are some examples that will help understanding why one or another rule should or should not be triggered.

also, this comment part can be shown in violation. I think additional PR will be needed :)

Ah yeah, that would be great for documentation purposes, but also to have nicer descriptions when tests fail.

Added. as an issue here.
#3125

ZevEisenberg · 2020-02-19T16:55:38Z

Source/SwiftLintFramework/Models/Example.swift

+    ///                           Defaults to the file where this initializer is called.
+    ///   - line:                 The line in the file where the example is located.
+    ///                           Defaults to the line where this initializer is called.
+    init(_ code: String, configuration: Any? = nil, testMultiByteOffsets: Bool = true, testOnLinux: Bool = true,


It would be great to add a test asserting that the two new parameters default to true unless otherwise specified.

ZevEisenberg · 2020-02-19T16:58:31Z

Source/SwiftLintFramework/Models/SwiftLintFile.swift

+
+    /// Returns whether or not the file contains any attributes that require the Foundation module.
+    func containsAttributesRequiringFoundation() -> Bool {
+        guard contents.contains("@objc") else {


Could this catch @objc in a comment? i.e. should this trigger?

// We don't need @objc because we're cool @nonobjc func foo() {}

No, this is just a fast path that skips more expensive checks further down.

ZevEisenberg · 2020-02-19T16:59:57Z

Source/SwiftLintFramework/Rules/Lint/UnusedImportRuleExamples.swift

+        }
+        A.dispatchMain()
+        ↓import Dispatch
+


Extra newline intentional?

Yes, it's a limitation of the rule right now that the import statement can't be at the very end of the file. If the file has a trailing newline, that's fine.

ZevEisenberg · 2020-02-19T17:22:05Z

Source/SwiftLintFramework/Rules/Lint/UnusedImportRuleExamples.swift

+        typealias Foo = CFData
+        """, configuration: [
+            "require_explicit_imports": true
+        ], testMultiByteOffsets: false, testOnLinux: false):


Is it just a coincidence that testMultibyteOffsets and testOnLinux are always paired in this PR? Is there a scenario where you would want one but not the other?

Just a coincidence.

ZevEisenberg · 2020-02-19T17:22:57Z

Source/SwiftLintFramework/Rules/Lint/UnusedImportRuleExamples.swift

+          static func dispatchMain() {}
+        }
+        A.dispatchMain()
+        """): Example("""


If you put the values (as opposed to the keys) in this dictionary on their own line, they get indented, which to my eye is easier to read.

ZevEisenberg · 2020-02-19T17:23:30Z

Source/SwiftLintFramework/Rules/RuleConfigurations/UnusedImportConfiguration.swift

@@ -0,0 +1,53 @@
+public struct TransitiveModuleConfiguration: Equatable {


Docs on this type would be nice

Source/SwiftLintFramework/Extensions/Dictionary+SwiftLint.swift

ZevEisenberg · 2020-02-19T17:28:22Z

Source/SwiftLintFramework/Extensions/SwiftDeclarationAttributeKind+Swiftlint.swift

+            .objc,
+            .objcName,
+            .objcMembers,
+            .objcNonLazyRealization


Not in scope for this PR, but I wanted to call it out: the lack of trailing commas on the last line of collection literals made #3040 harder, because I had to special-case a bunch of search and replace. I strongly recommend enforcing trailing commas in any collection-heavy codebase. Just my 2¢

ZevEisenberg · 2020-02-19T17:28:57Z

Source/SwiftLintFramework/Models/Example.swift

@@ -3,22 +3,36 @@
 public struct Example {
    /// The contents of the example
    public private(set) var code: String
+    /// The untyped configuration to apply to the rule, if deviating from the default configuration
+    public private(set) var configuration: Any?


Why is the configuration untyped? Might be nice to include something in the comma, so others understand the intended usage.

ZevEisenberg · 2020-02-19T17:31:25Z

Source/SwiftLintFramework/Models/Example.swift

+    /// The untyped configuration to apply to the rule, if deviating from the default configuration
+    public private(set) var configuration: Any?
+    /// Whether the example should be tested by prepending multibyte grapheme clusters
+    public private(set) var testMultiByteOffsets: Bool


It's not very clear from the comment what is actually meant. After reading it, I have several questions that I'd prefer not to have to go digging through the source code to answer:

why do multibyte grapheme clusters matter?

why might I want to disable them?

which multibyte clusters are tested? Are they specific or random?

where and when are the multibyte characters prepended, and what effect does that have?

why do multibyte grapheme clusters matter?

In the past, some rules were written with some assumptions that each character was one byte.

Characters that extend beyond a single byte encoded in UTF-8 need to be handled correctly.

The testMultiByteOffsets setting prepends some multi-byte grapheme clusters to the code in the example (specifically /* 👨‍👩‍👧‍👦👨‍👩‍👧‍👦👨‍👩‍👧‍👦 */) to ensure that the locations where violations are reported are correct, and that corrections are properly applied.

why might I want to disable them?

Some examples aren't resilient to prepending this content to its code and need to disable this part of the test. For example, this unused import code:

import Foundation typealias Foo = CFData

Gets corrected by replacing import Foundation with import CoreFoundation:

import CoreFoundation typealias Foo = CFData

However, when the violating example gets translated to this by the testMultiByteOffsets setting:

/* 👨‍👩‍👧‍👦👨‍👩‍👧‍👦👨‍👩‍👧‍👦 */ import Foundation typealias Foo = CFData

then the correction becomes:

import CoreFoundation /* 👨‍👩‍👧‍👦👨‍👩‍👧‍👦👨‍👩‍👧‍👦 */ typealias Foo = CFData

Which fails the test. There's nothing wrong with the rule's behavior, it just doesn't match what the simplistic auto-generated rule expects.

which multibyte clusters are tested? Are they specific or random?

Specific: /* 👨‍👩‍👧‍👦👨‍👩‍👧‍👦👨‍👩‍👧‍👦 */

where and when are the multibyte characters prepended, and what effect does that have?

See above.

ZevEisenberg · 2020-02-19T17:34:06Z

Source/SwiftLintFramework/Rules/Lint/UnusedImportRuleExamples.swift

+          static func dispatchMain() {}
+        }
+        A.dispatchMain()
+


Intentional whitespace? Here and above.

Intentional, yes: #3123 (comment)

Co-Authored-By: Zev Eisenberg <zev@zeveisenberg.com>

To refer the reader to what the effect of that setting is.

AdrianBinDC · 2020-02-26T03:27:53Z

Awesome addition! Thank you for adding this.

* master: (101 commits) JUnit reporter for GitLab artifact:report:junit (realm#3177) Add empty changelog section release 0.39.2 Update CI to run jobs with Xcode 11.0 to 11.4 (realm#3168) Fix false positives in valid_ibinspectable rule when using Swift 5.2 (realm#3155) Fix attributes rule false positive with Swift 5.2 (realm#3154) Fix CHANGELOG link Fix false positives in redundant_objc_attribute with Swift 5.2 (realm#3152) Fix false positives on implicit_getter with Swift 5.2+ (realm#3151) Simplify regex (realm#3145) fix links about configuration rules (realm#3142) Add unused_import config options to require imports for each module used (realm#3123) Add empty changelog section release 0.39.1 Temporarily remove all SwiftSyntax rules and support (realm#3107) Fix unused_import rule reported locations and corrections (realm#3106) release 0.39.0 Fix false positive in `empty_string` rule with multiline literals (realm#3101) Fix PrivateActionRule in Swift 5.2 (realm#3092) Fix false positive in implicit_getter with Swift 5.2 (realm#3099) ... # Conflicts: # Source/SwiftLintFramework/Extensions/SwiftLintFile+Regex.swift

mthole · 2022-03-17T22:33:39Z

(two years later)

I used this today on a large project. Thanks for this addition!

jpsim · 2022-03-18T00:30:24Z

I’m glad you found it useful! We run it daily on Lyft’s 1M+ lines of Swift.

jpsim added 7 commits February 16, 2020 16:35

Move modulesAllowedToBeTransitivelyImported to UnusedImportConfiguration

ddf2eaf

Be smarter about where imports are added and don't import current module

29cc129

Sort imports

2d40c13

Only add missing imports when correcting

bc506ff

Reuse variable

4657b99

Clean up and organize

f6191d6

jpsim mentioned this pull request Feb 18, 2020

Add unused_import config option to require imports for each module used #3117

Closed

jpsim added 3 commits February 18, 2020 14:24

Add changelog entry

addde1a

Move examples to their own file

27c7559

Add new tests for require_explicit_imports

1678f15

Skip some tests on Linux

8ccd77c

PaulTaykalo approved these changes Feb 19, 2020

View reviewed changes

ZevEisenberg reviewed Feb 19, 2020

View reviewed changes

jpsim and others added 8 commits February 22, 2020 13:48

Fix typo in changelog

f2670aa

Co-Authored-By: Zev Eisenberg <zev@zeveisenberg.com>

Use backticks in docstring

f2696c1

Co-Authored-By: Zev Eisenberg <zev@zeveisenberg.com>

Add tests for Example.testMultiByteOffsets and Example.testOnLinux

bd10935

Indent unused import rule examples for improved readability

540c508

Add docs for TransitiveModuleConfiguration

f182ba3

Add more information to the Example.configuration docstring

d78cbe7

Add 'SeeAlso' to Example.testMultiByteOffsets

eb8adb3

To refer the reader to what the effect of that setting is.

make sourcery

446b971

jpsim merged commit d10ccac into master Feb 22, 2020

jpsim deleted the missing-imports branch February 22, 2020 22:39

AdrianBinDC mentioned this pull request Mar 1, 2020

require_explicit_imports: true nukes all import headers #3136

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unused_import config options to require imports for each module used #3123

Add unused_import config options to require imports for each module used #3123

jpsim commented Feb 18, 2020

jpsim commented Feb 18, 2020

SwiftLintBot commented Feb 18, 2020 •

edited

Loading

jpsim commented Feb 19, 2020

jpsim commented Feb 19, 2020

PaulTaykalo left a comment

PaulTaykalo Feb 19, 2020

jpsim Feb 19, 2020

PaulTaykalo Feb 19, 2020

ZevEisenberg Feb 19, 2020

ZevEisenberg Feb 19, 2020

jpsim Feb 19, 2020

ZevEisenberg Feb 19, 2020

jpsim Feb 22, 2020

ZevEisenberg Feb 19, 2020

jpsim Feb 19, 2020

ZevEisenberg Feb 19, 2020

ZevEisenberg Feb 19, 2020

ZevEisenberg Feb 19, 2020

ZevEisenberg Feb 19, 2020

ZevEisenberg Feb 19, 2020

jpsim Feb 22, 2020

ZevEisenberg Feb 19, 2020

jpsim Feb 22, 2020

AdrianBinDC commented Feb 26, 2020

mthole commented Mar 17, 2022

jpsim commented Mar 18, 2022

		@@ -0,0 +1,53 @@
		public struct TransitiveModuleConfiguration: Equatable {

Add unused_import config options to require imports for each module used #3123

Add unused_import config options to require imports for each module used #3123

Conversation

jpsim commented Feb 18, 2020

jpsim commented Feb 18, 2020

SwiftLintBot commented Feb 18, 2020 • edited Loading

jpsim commented Feb 19, 2020

jpsim commented Feb 19, 2020

PaulTaykalo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdrianBinDC commented Feb 26, 2020

mthole commented Mar 17, 2022

jpsim commented Mar 18, 2022

SwiftLintBot commented Feb 18, 2020 •

edited

Loading