Adds invalid swiftlint command rule #4546

mildm8nnered · 2022-11-13T12:37:14Z

Rule-based approach to #4543

Adds an invalid_swiftlint_command rule to detect malformed swiftlint commands. Bungled enable's in particular, after a valid disable can lead to stretches of code going unintentionally un-linted.

nonTriggeringExamples:

// swiftlint:disable unused_import
// swiftlint:enable unused_import
// swiftlint:disable:next unused_import
// swiftlint:disable:previous unused_import
// swiftlint:disable:this unused_import

triggeringExamples:

// swiftlint:
// swiftlint: 
// swiftlint::
// swiftlint:: 
// swiftlint:disable
// swiftlint:dissable unused_import
// swiftlint:enaaaable unused_import
// swiftlint:disable:nxt unused_import
// swiftlint:enable:prevus unused_import
// swiftlint:enable:ths unused_import
// swiftlint:enable
// swiftlint:enable:
// swiftlint:enable: 
// swiftlint:disable: unused_import

Command Parsing

This PR also slightly changes the parsing of commands. Previously cases like

// swiftlint:disable: unused_import
// swiftlint:disable:nxt unused_import
// swiftlint:enable:prevus unused_import
// swiftlint:enable:ths unused_import

Would have been parsed, incorrectly, as

// swiftlint:disable unused_import
// swiftlint:disable unused_import
// swiftlint:enable unused_import
// swiftlint:enable unused_import

These cases will now be ignored, and will be warned about by this rule.

SwiftLintBot · 2022-11-13T12:59:33Z

	1 Warning
⚠️	This PR introduced a violation in WordPress: /WordPress/UITestsFoundation/Screens/ReaderScreen.swift:15:53: warning: Invalid SwiftLint Command Violation: swiftlint command does not have a valid action or modifier (invalid_swiftlint_command)

	18 Messages
📖	Linting Aerial with this PR took 1.03s vs 1.03s on main (0% slower)
📖	Linting Alamofire with this PR took 1.34s vs 1.35s on main (0% faster)
📖	Linting Brave with this PR took 7.2s vs 7.17s on main (0% slower)
📖	Linting DuckDuckGo with this PR took 3.02s vs 3.01s on main (0% slower)
📖	Linting Firefox with this PR took 9.15s vs 9.09s on main (0% slower)
📖	Linting Kickstarter with this PR took 10.0s vs 10.05s on main (0% faster)
📖	Linting Moya with this PR took 0.54s vs 0.53s on main (1% slower)
📖	Linting NetNewsWire with this PR took 2.99s vs 2.98s on main (0% slower)
📖	Linting Nimble with this PR took 0.58s vs 0.58s on main (0% slower)
📖	Linting PocketCasts with this PR took 7.03s vs 7.0s on main (0% slower)
📖	Linting Quick with this PR took 0.22s vs 0.22s on main (0% slower)
📖	Linting Realm with this PR took 11.29s vs 11.32s on main (0% faster)
📖	Linting SourceKitten with this PR took 0.43s vs 0.43s on main (0% slower)
📖	Linting Sourcery with this PR took 2.19s vs 2.16s on main (1% slower)
📖	Linting Swift with this PR took 4.44s vs 4.41s on main (0% slower)
📖	Linting VLC with this PR took 1.32s vs 1.32s on main (0% slower)
📖	Linting Wire with this PR took 8.2s vs 8.15s on main (0% slower)
📖	Linting WordPress with this PR took 10.91s vs 10.88s on main (0% slower)

Generated by 🚫 Danger

mildm8nnered · 2023-01-29T14:04:30Z

So I managed to work out make sourcery to get the new rule into PrimaryRuleList, but I'm failing the generated tests because of the nature of the rule, it needs to be validated with

    func testExamples() {
        verifyRule(InvalidSwiftLintCommandRule.description, skipCommentTests: true, skipDisableCommandTests: true)
    }

as I've done in InvalidSwiftLintCommandRuleTests, but there doesn't seem to be any way to pass these parameters into the generated tests, or to exclude that rule from the generated tests.

Not sure what the best way is to go from here ...

SimplyDanny · 2023-01-29T16:29:44Z

Not sure what the best way is to go from here ...

See for example ExpiringTodoRule. You can pass the options to skip certain tests to every example or an array of examples.

mildm8nnered · 2023-01-29T16:36:42Z

So I guess I could special case this rule in .sourcery/GeneratedTests.stencil, something like

@_spi(TestHelper)
@testable import SwiftLintFramework
import SwiftLintTestHelpers
import XCTest

// swiftlint:disable file_length single_test_class type_name

{% for rule in types.structs %}
{% if rule.name|hasSuffix:"Rule" and rule.name != "InvalidSwiftLintCommandRule" %}
class {{ rule.name }}GeneratedTests: XCTestCase {
    func testWithDefaultConfiguration() {
        verifyRule({{ rule.name }}.description)
    }
}
{% if not forloop.last %}

{% endif %}
{% endif %}
{% endfor %}

and rely on the handwritten test.

Or I could further edit the stencil to generate the appropriate code for this case. That doesn't feel very graceful. I will also face a similar problem for #4684, so ideally any mechanism here would be easily extensible.

Source/SwiftLintFramework/Rules/Lint/InvalidSwiftLintCommandRule.swift

mildm8nnered · 2023-02-26T21:02:27Z

See also my comment at https://github.com/realm/SwiftLint/issues/3769#issuecomment-1445466113 - it looks like all kinds of odd strings, like // hjkhjkkswiftlint:disable all will be treated as valid, which seems off.

SimplyDanny

Sorry to miss your last comment. Apart from some smaller findings, it looks good to me now.

I like that the rule itself is very easy now. Good work!

Source/SwiftLintFramework/Models/Command.swift

SimplyDanny · 2023-02-28T18:20:52Z

Source/SwiftLintFramework/Models/Command.swift

@@ -27,6 +30,8 @@ public struct Command: Equatable {
        case this
        /// The command should only apply to the line following its definition.
        case next
+        /// The modifier string was invalid


Nit: In documentation, prefered style is a period at the end of sentences/descriptions.

SimplyDanny · 2023-02-28T18:21:16Z

Source/SwiftLintFramework/Models/Command.swift

@@ -8,13 +8,16 @@ public struct Command: Equatable {
        case enable
        /// The rule(s) associated with this command should be disabled by the SwiftLint engine.
        case disable
+        /// The action string was invalid


Same here: Prefer a period at the end.

There should be a rule for that :-)

…mands initializer

SimplyDanny · 2023-03-04T12:53:46Z

Merged. 😊 Thanks for all the work @mildm8nnered!

mildm8nnered mentioned this pull request Nov 13, 2022

Warn on malformed SwiftLint directives #4543

Closed

mildm8nnered mentioned this pull request Nov 13, 2022

Malformed SwiftLint directives should generate an error or warning #4539

Closed

mildm8nnered force-pushed the mildm8nnered-invalid-swiftlint-directives-rule branch 2 times, most recently from 6f63adc to 8fd470f Compare January 29, 2023 13:16

SimplyDanny reviewed Jan 29, 2023

View reviewed changes

Source/SwiftLintFramework/Rules/Lint/InvalidSwiftLintCommandRule.swift Outdated Show resolved Hide resolved

mildm8nnered marked this pull request as ready for review January 29, 2023 23:55

mildm8nnered force-pushed the mildm8nnered-invalid-swiftlint-directives-rule branch from 3c1d2f3 to 9fed2a9 Compare February 11, 2023 11:18

mildm8nnered force-pushed the mildm8nnered-invalid-swiftlint-directives-rule branch 2 times, most recently from 2a8191e to 5656430 Compare February 26, 2023 17:05

SimplyDanny approved these changes Feb 28, 2023

View reviewed changes

mildm8nnered added 15 commits February 28, 2023 21:47

Started to add invalid_swiftlint_directive rule

3e2eb0b

detect bad modifiers

31a75b1

rename directive -> command

59da76b

Added unit tests

7969323

We now locate the violation at the start of the "swiftlint:" command.

3083517

line_length fix

f420052

trailing comma

9e62684

Removed our manual entry

e45e731

Ran sourcery

0fdf318

Hacked the template for now to get through CI

bdfc42f

suppress generated test

8d12600

Fixed generated tests

fbde4ae

Removed unnecessary disables

c359040

Updated change log

8c521f4

invalid commands (with invalid modifier's) should return nil from Com…

1f93737

…mands initializer

mildm8nnered added 5 commits February 28, 2023 21:47

implicit return

10b44c1

Not actually implicit

a024bbe

Moved to a one-pass approach

a4783de

use ternary operator

516ffee

Whitespace

f3abe43

mildm8nnered force-pushed the mildm8nnered-invalid-swiftlint-directives-rule branch from 5656430 to f3abe43 Compare February 28, 2023 21:47

CR Comments

bdcf3ce

SimplyDanny merged commit 1c3c62e into realm:main Mar 4, 2023

mildm8nnered deleted the mildm8nnered-invalid-swiftlint-directives-rule branch April 17, 2023 23:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds invalid swiftlint command rule #4546

Adds invalid swiftlint command rule #4546

mildm8nnered commented Nov 13, 2022 •

edited

Loading

SwiftLintBot commented Nov 13, 2022 •

edited

Loading

mildm8nnered commented Jan 29, 2023

SimplyDanny commented Jan 29, 2023

mildm8nnered commented Jan 29, 2023

mildm8nnered commented Feb 26, 2023

SimplyDanny left a comment

SimplyDanny Feb 28, 2023

SimplyDanny Feb 28, 2023

mildm8nnered Feb 28, 2023

SimplyDanny commented Mar 4, 2023

Adds invalid swiftlint command rule #4546

Adds invalid swiftlint command rule #4546

Conversation

mildm8nnered commented Nov 13, 2022 • edited Loading

Command Parsing

SwiftLintBot commented Nov 13, 2022 • edited Loading

mildm8nnered commented Jan 29, 2023

SimplyDanny commented Jan 29, 2023

mildm8nnered commented Jan 29, 2023

mildm8nnered commented Feb 26, 2023

SimplyDanny left a comment

Choose a reason for hiding this comment

SimplyDanny Feb 28, 2023

Choose a reason for hiding this comment

SimplyDanny Feb 28, 2023

Choose a reason for hiding this comment

mildm8nnered Feb 28, 2023

Choose a reason for hiding this comment

SimplyDanny commented Mar 4, 2023

mildm8nnered commented Nov 13, 2022 •

edited

Loading

SwiftLintBot commented Nov 13, 2022 •

edited

Loading