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

superfluous_disable_command in a multiline comment generates multiple false alarms for x is not a valid SwiftLint rule #4798

Closed
2 tasks done
mildm8nnered opened this issue Mar 4, 2023 · 1 comment · Fixed by #4791 or #4803
Labels
bug Unexpected and reproducible misbehavior.

Comments

@mildm8nnered
Copy link
Collaborator

mildm8nnered commented Mar 4, 2023

New Issue Checklist

Describe the bug

Given the following input:

/*extension SidebarViewController {
    // swiftlint:disable cyclomatic_complexity
    // Make sure we're right clicking a menu entry
}
*/

superfluous_disable_command will generate multiple false positives.

#4791 fixes the case of this that occurs in Aerial purely by chance (see the osstools output on that PR), because the problem stretch of code there happens to contain the word all which is interpreted as suppress all rules. However #4791 does not fix the problem for the above case, or any other cases in general.

Complete output when running SwiftLint, including the stack trace and command used
% swiftlint lint --no-cache --use-stdin --enable-all-rules <<EOF
/*extension SidebarViewController {
    // swiftlint:disable cyclomatic_complexity
    // Make sure we're right clicking a menu entry
}
*/
EOF
The `anyobject_protocol` rule is now deprecated and will be completely removed in a future release.
<nopath>:5:3: warning: Superfluous Disable Command Violation: SwiftLint rule 'cyclomatic_complexity' did not trigger a violation in the disabled region. Please remove the disable command. (superfluous_disable_command)
<nopath>:5:3: warning: Superfluous Disable Command Violation: 'sure' is not a valid SwiftLint rule. Please remove it from the disable command. (superfluous_disable_command)
<nopath>:5:3: warning: Superfluous Disable Command Violation: '//' is not a valid SwiftLint rule. Please remove it from the disable command. (superfluous_disable_command)
<nopath>:5:3: warning: Superfluous Disable Command Violation: 'a' is not a valid SwiftLint rule. Please remove it from the disable command. (superfluous_disable_command)
<nopath>:5:3: warning: Superfluous Disable Command Violation: 'clicking' is not a valid SwiftLint rule. Please remove it from the disable command. (superfluous_disable_command)
<nopath>:5:3: warning: Superfluous Disable Command Violation: 'entry' is not a valid SwiftLint rule. Please remove it from the disable command. (superfluous_disable_command)
<nopath>:5:3: warning: Superfluous Disable Command Violation: 'right' is not a valid SwiftLint rule. Please remove it from the disable command. (superfluous_disable_command)
<nopath>:5:3: warning: Superfluous Disable Command Violation: '}' is not a valid SwiftLint rule. Please remove it from the disable command. (superfluous_disable_command)
<nopath>:5:3: warning: Superfluous Disable Command Violation: 'menu' is not a valid SwiftLint rule. Please remove it from the disable command. (superfluous_disable_command)
<nopath>:5:3: warning: Superfluous Disable Command Violation: 'Make' is not a valid SwiftLint rule. Please remove it from the disable command. (superfluous_disable_command)
<nopath>:5:3: warning: Superfluous Disable Command Violation: 'we're' is not a valid SwiftLint rule. Please remove it from the disable command. (superfluous_disable_command)
Done linting! Found 11 violations, 0 serious in 1 file.

Environment

  • SwiftLint version (run swiftlint version to be sure)? 0.50.3

  • Installation method used (Homebrew, CocoaPods, building from source, etc)? Homebrew

  • Paste your configuration file: none

  • Are you using nested configurations? no

  • Which Xcode version are you using (check xcodebuild -version)?

Xcode 14.2
Build version 14C18
  • Do you have a sample that shows the issue? Run echo "[string here]" | swiftlint lint --no-cache --use-stdin --enable-all-rules
    to quickly test if your example is really demonstrating the issue. If your example is more
    complex, you can use swiftlint lint --path [file here] --no-cache --enable-all-rules.

See description above.

@mildm8nnered
Copy link
Collaborator Author

So ... the underlying problem is the way CommandVisitor parses the command.

private extension Trivia {
    func commands(offset: AbsolutePosition, locationConverter: SourceLocationConverter) -> [Command] {
        var triviaOffset = SourceLength.zero
        var results: [Command] = []
        for trivia in self {
            triviaOffset += trivia.sourceLength
            switch trivia {
            case .lineComment(let comment), .blockComment(let comment):
                if
                    let lower = comment.range(of: "swiftlint:")?.lowerBound,
                    case let actionString = String(comment[lower...]),
                    case let end = locationConverter.location(for: offset + triviaOffset),
                    let line = end.line,
                    let column = end.column
                {
                    let command = Command(actionString: actionString, line: line, character: column)
                    results.append(command)
                }
            default:
                break
            }
        }

        return results
    }
}

In the case of a block comment like

/*extension SidebarViewController {
    // swiftlint:disable cyclomatic_complexity
    // Make sure we're right clicking a menu entry
}
*/

actionString ends up being

 swiftlint:disable cyclomatic_complexity
    // Make sure we're right clicking a menu entry
}

and all of those extra symbols are interpreted as rules names to disable.

#4791 resolves this for the case of Aerial purely by chance, as that specific file contains the word all within the problem area of the code (e.g. https://github.com/JohnCoates/Aerial/blob/af271a649addb0303768918e9b1cde807da3c58b/Resources/MainUI/SidebarViewController.swift#L260), which is interpreted as swiftlint:disable all and #4791 suppresses the warning about unknown rule names when all rules, or the superfluous_disable_rule are disabled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment