Using --strict --fix hides errors and succeeds

ladislas commented 10 months ago

New Issue Checklist

[x] Updated SwiftLint to the latest version
[x] I searched for existing GitHub issues

Describe the bug

We are using swiftlint with pre-commit with the command provided in the documentation

  - repo: https://github.com/realm/SwiftLint
    rev: 0.54.0
    hooks:
      - id: swiftlint
        entry: swiftlint --fix --strict

It does fix the issues but it does not fail.

changing to just swiftlint fails as expected but doesn't fix the issues

Complete output when running SwiftLint, including the stack trace and command used

with swiftlint --fix --strict

$ gc -a

trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
check yaml...............................................................Passed
check json...........................................(no files to check)Skipped
check for added large files..............................................Passed
check that executables have shebangs.................(no files to check)Skipped
check that scripts with shebangs are executable..........................Passed
detect private key.......................................................Passed
forbid submodules....................................(no files to check)Skipped
mixed line ending........................................................Passed
SwiftLint................................................................Passed
SwiftFormat..............................................................Passed

just with swiftlint

$ git commit -a
trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
check yaml...............................................................Passed
check json...........................................(no files to check)Skipped
check for added large files..............................................Passed
check that executables have shebangs.................(no files to check)Skipped
check that scripts with shebangs are executable..........................Passed
detect private key.......................................................Passed
forbid submodules....................................(no files to check)Skipped
mixed line ending........................................................Passed
SwiftLint................................................................Failed
- hook id: swiftlint
- exit code: 2

Linting Swift files at paths Modules/DesignKit/Sources/Colors.swift
Linting 'Colors.swift' (1/1)
/Users/ladislas/dev/leka/ios-monorepo/Modules/DesignKit/Sources/Colors.swift
⛔️ Line 10: Variable name 'arr' should be between 4 and 40 characters long (identifier_name)
⛔️ Line 8: Type name 'leka' should start with an uppercase character (type_name)
Done linting! Found 2 violations, 2 serious in 1 file.

SwiftFormat..............................................................Passed

Environment

SwiftLint version (run swiftlint version to be sure)?

swiftlint version
0.53.0

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

Homebrew

Paste your configuration file:

# Leka - iOS Monorepo
# Copyright 2023 APF France handicap
# SPDX-License-Identifier: Apache-2.0

opt_in_rules: []

trailing_comma:
  mandatory_comma: true

type_name:
  min_length: 4 # only warning
  max_length: # warning and error
    warning: 40
    error: 50
  allowed_symbols: ["_"] # these are allowed in type names
  excluded:
    - i18n
    - l10n

identifier_name:
  min_length: # only min_length
    error: 4 # only error
  excluded:
    - a
    - app
    - args
    - b
    - ble
    - BLE
    - cmd
    - g
    - i
    - i18n
    - id
    - img
    - key
    - l10n
    - lhs
    - log
    - new
    - r
    - red
    - rgb
    - rhs
    - rx
    - tx
    - URL

line_length:
  warning: 150
  ignores_urls: true
  ignores_function_declarations: true
  ignores_interpolated_strings: true

disabled_rules:
  - opening_brace
  - switch_case_alignment
  - closure_parameter_position

excluded:
  - Tuist/Dependencies
  - ./*/Derived

reporter: "emoji"

Are you using nested configurations?

no

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

xcodebuild -version
Xcode 15.0.1
Build version 15A507

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.

It involves using pre-commit, so no

ladislas commented 10 months ago

funny thing is that using swiftlint --strict, doesn't change all the warnings to errors, I only see the two errors I already have with swiftlint alone

SimplyDanny commented 10 months ago

Linting and fixing are two different modes. With --fix, SwiftLint only reports corrected violations. They don't fail. That said, --strict doesn't have any effect when auto-correcting code.

For which rules specifically does no promotion happen when running swiftlint --strict?

JaviSoto commented 7 months ago

Example:

Modify 2 files:

A has a violation of rule #1, which is correctable.
B has a violation of rule #2, which isn't correctable.

Run:

swiftlint lint --fix --strict

Behavior:

A violation is fixed
B violation isn't reported
swiftlint exists with non-0 code.

If you run the same command again, however, B isn't reported, and swiftlint exists with a 0 code, despite the fact that there's a linting issue.

I believe this may be what @ladislas was pointing out.

SimplyDanny commented 7 months ago

--fix fixes what's fixable. Nothing else is reported. Thus, --strict doesn't have an effect.

A check could be added that warns about --strict not having any effect if it comes piggyback with --fix.

swiftlint exists with non-0 code.

I cannot see that. swiftlint --fix [--strict] always finishes with 0.

SimplyDanny commented 7 months ago

I'll keep this open awaiting your feedback.

ladislas commented 7 months ago

I believe this may be what @ladislas was pointing out.

@JaviSoto yes exactly! thanks for clarifying :)

--fix fixes what's fixable. Nothing else is reported. Thus, --strict doesn't have an effect.

@SimplyDanny that makes it clear!

any reason why they don't work together?

JaviSoto commented 7 months ago

A check could be added that warns about --strict not having any effect if it comes piggyback with --fix.

This is nice so at least the user knows this is going on, but it's still undesireable behavior: if you're using --strict it's because you want it to fail if it has any issues. Using --fix should mean "fix anything you can", but given that a lot of the rules can't be fixed, it shouldn't just swallow errors. Because of this behavior we're having to run Swiftlint twice, once with --fix and one without, which is very slow in our codebase. We'd like to be able to run it just once and have it fail if there are any outstanding issues

SimplyDanny commented 7 months ago

I get the confusion now. With --fix being an option of lint, people expect the normal lint behavior with automatic fixes for correctable rules on top. This would be different if fix was its own command and not an option of lint.

Without a new option, this would be a breaking change, though.

realm / SwiftLint