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

Question: What is the expected behaviour of opt_in_rules and disabled_rules in a child configuration? #4876

Closed
2 tasks done
mildm8nnered opened this issue Apr 11, 2023 · 0 comments · Fixed by #4863
Closed
2 tasks done
Labels
discussion Topics that cannot be categorized as bugs or enhancements yet. They require further discussions.

Comments

@mildm8nnered
Copy link
Collaborator

New Issue Checklist

Describe the bug

Replaces #4859

Some of the behaviours of opt_in_rules and disabled_rules seem, at face value, to be incorrect.

For example, consider a case where the parent.yml looks like:

opt_in_rules:
  - implicit_return
disabled_rules:
  - implicit_return

and the child.yml looks like:

parent_config: parent.yml

Naively, I would expect the specified opt-in rule to be disabled in the parent configuration, and to also be disabled in the child, but in practice, with 0.51.0, the rule will be enabled in the child, according to the rules subcommand:

% swiftlint rules  --config parent.yml | grep implicit_return
| implicit_return                          | yes    | yes         | NO                     | style       | no       | yes            | severity: ... |
% swiftlint rules  --config child.yml | grep implicit_return
| implicit_return

And observing actual linting behaviour, given

class ImplicitReturn {
   var foo: Int {
       return 0
   }
}
 % swiftlint lint  --config parent.yml ImplicitReturn.swift 
Linting Swift files at paths ImplicitReturn.swift
Linting 'ImplicitReturn.swift' (1/1)
Done linting! Found 0 violations, 0 serious in 1 file.
%
% swiftlint lint --config child.yml ImplicitReturn.swift 
Linting Swift files at paths ImplicitReturn.swift
Linting 'ImplicitReturn.swift' (1/1)
/Users/martin.redington/Documents/Hudl/Source/SwiftLint/ImplicitReturn.swift:3:8: warning: Implicit Return Violation: Prefer implicit returns in closures, functions and getters (implicit_return)
Done linting! Found 1 violation, 0 serious in 1 file.

Similarly, if the child configuration is

parent_config: parent.yml

opt_in_rules:
  - implicit_return
disabled_rules:
  - implicit_return

and the parent has a "default" configuration (using opt_in_rules and disabled_rules), no matter what the parent settings, the specified rule will always be enabled in the child, which seems wrong.

Below is the complete truth table for whether an optin rule is enabled in the merged configuration, as a function of whether it is listed in the opt_in_rules, and/or the disabled_rules sections of the parent and child configurations.

The "unexpected" cases are asterisked.

        parent    parent      child      child
        opt_in   disabled    opt_in     disabled    current     proposed

0       false      false      false      false       false       false
1        true      false      false      false        true        true
2       false       true      false      false       false       false
3        true       true      false      false        true       false *
4       false      false       true      false        true        true
5        true      false       true      false        true        true
6       false       true       true      false        true        true
7        true       true       true      false        true        true
8       false      false      false       true       false       false
9        true      false      false       true       false       false
10      false       true      false       true       false       false
11       true       true      false       true       false       false
12      false      false       true       true        true       false *
13       true      false       true       true        true       false *
14      false       true       true       true        true       false *
15       true       true       true       true        true       false *

The current behaviour is actually "better" for some use cases - if I want my child configuration to be stricter than the parent, the current behaviour can actually end up being more compact than the new configuration, and I suspect that fixing this would be a breaking change for some users, but the current behaviour nevertheless seems counter-intuitive to me.

My "question" is, is the current behaviour intentional, or it should it actually behave as the proposed column above, or in some other way?

A similar situation occurs when the parent configuration uses only_rules, and the child has a default configuration.

The truth table for this case (of whether the rule is enabled in the merged configuration) is:

       child       child
      opt_in     disabled    current   proposed

0      false      false       true       true
1       true      false       true       true
2      false       true      false      false
3       true       true       true      false *

Examining case 3

parent.yml:

only_rules:
   - implicit_return

and child.yml:

parent_config: parent.yml

opt_in_rules:
  - implicit_return
disabled_rules:
  - implicit_return

Currently (in main/0.51.0) the rule would end up enabled for the child, even though it is explicitly disabled in the child configuration.

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

See above

Environment

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

0.51.0

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

  • Paste your configuration file:

See above

  • Are you using nested configurations? Nested no, but parent and child yes.

  • 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

@SimplyDanny SimplyDanny added the discussion Topics that cannot be categorized as bugs or enhancements yet. They require further discussions. label Apr 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Topics that cannot be categorized as bugs or enhancements yet. They require further discussions.
Projects
None yet
2 participants