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 bug #5788

Closed
artsimonyan23 opened this issue Sep 11, 2024 · 6 comments
Closed

superfluous_disable_command bug #5788

artsimonyan23 opened this issue Sep 11, 2024 · 6 comments
Labels
bug Unexpected and reproducible misbehavior.

Comments

@artsimonyan23
Copy link

superfluous_disable_command incorrectly runs all custom rules.

@SimplyDanny
Copy link
Collaborator

Please provide a concrete example.

@SimplyDanny SimplyDanny added the repro-needed Issues that cannot be reproduced or miss proper descriptive examples. label Sep 11, 2024
@artsimonyan23
Copy link
Author

I created a custom rule weak_self_closure. In some places, I disabled it ( // swiftlint:disable:this weak_self_closure). After updating with the new version (0.57.0) in all disabled places I receive superfluous_disable_command warning. When I am removing the "// swiftlint:disable:this weak_self_closure", it shows "weak_self_closure" warning.

@mildm8nnered
Copy link
Collaborator

I wish I'd never touched this.

I think there is a problem here, where disabled regions are nested.

Given the following config:

only_rules:
  - custom_rules
  - superfluous_disable_command

# Custom rules
custom_rules:
  fatal_error:
    name: Fatal Error
    excluded: "Tests/*"
    message: Prefer using `queuedFatalError` over `fatalError` to avoid leaking compiler host machine paths.
    regex: \bfatalError\b
    match_kinds:
      - identifier
    severity: error
  fattal_error:
    name: Fattal Error
    excluded: "Tests/*"
    message: Prefer using `queuedFatalError` over `fatalError` to avoid leaking compiler host machine paths.
    regex: \bfattalError\b
    match_kinds:
      - identifier
    severity: error    

and the following input:

// swiftlint:disable fatal_error
// swiftlint:disable:next fattal_error
fatalError("something")
// swiftlint:enable fatal_error

then

% swiftlint tt.swift --config tt.config
Linting Swift files at paths tt.swift
Linting 'tt.swift' (1/1)
/Users/martin.redington/Documents/Source/SwiftLint/tt.swift:1:33: warning: Superfluous Disable Command Violation: SwiftLint rule 'fatal_error' did not trigger a violation in the disabled region; remove the disable command (superfluous_disable_command)
/Users/martin.redington/Documents/Source/SwiftLint/tt.swift:3:1: warning: Superfluous Disable Command Violation: SwiftLint rule 'fattal_error' did not trigger a violation in the disabled region; remove the disable command (superfluous_disable_command)
/Users/martin.redington/Documents/Source/SwiftLint/tt.swift:3:9223372036854775807: warning: Superfluous Disable Command Violation: SwiftLint rule 'fatal_error' did not trigger a violation in the disabled region; remove the disable command (superfluous_disable_command)
Done linting! Found 3 violations, 0 serious in 1 file.

But fatalError should not be triggering superfluous_disable_command here at all.

I think this is being caused by the way regions are returned from SwiftLintFile's regions(restrictingRuleIdentifiers:) method, and I'm hoping we can fix it just by making a change there - right now each new swiftlint: command seems to generate a region with all disabled identifiers, but we probably really want a region for each command.

@mildm8nnered mildm8nnered added bug Unexpected and reproducible misbehavior. and removed repro-needed Issues that cannot be reproduced or miss proper descriptive examples. labels Sep 15, 2024
@Patrick-Kladek
Copy link

Any update on this?

@mildm8nnered
Copy link
Collaborator

Any update on this?

Yes - there's a PR at #5797, which is awaiting code review, which should hopefully fix the issue.

@mildm8nnered
Copy link
Collaborator

This should be resolved by #5797

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Unexpected and reproducible misbehavior.
Projects
None yet
Development

No branches or pull requests

4 participants