SwiftLint icon indicating copy to clipboard operation
SwiftLint copied to clipboard

Glob.resolveGlob() is veeeery sloooow

Open roddi opened this issue 1 year ago • 6 comments

New Issue Checklist

Describe the bug

A clear and concise description of what the bug is.

swiftlint became very slow in the last release. As if someone implemented a O(n^2) in Glob.resolveGlob(). In our project we are from several seconds (a dozen or so) to around of half an hour.

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

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:

# insert yaml contents here
disabled_rules:
  - line_length
  - identifier_name
  - cyclomatic_complexity
  - large_tuple
  - function_body_length
  - todo
  - class_delegate_protocol
  - implicit_getter
  - orphaned_doc_comment
  - closure_parameter_position
  - void_function_in_ternary
  - for_where
  - type_body_length
  - nsobject_prefer_isequal
opt_in_rules:
  - empty_count
  - implicit_return
  - empty_string
  - empty_collection_literal
  - first_where
  - modifier_order
  - toggle_bool
  - convenience_type
#  - implicitly_unwrapped_optional
#  - force_unwrapping
#  - unavailable_function
#  - function_default_parameter_at_end
excluded: # paths to ignore during linting. Takes precedence over `included`.
  - Build
  - Danger
  - Scripts
  - "**/*Test*"
  - "Packages/Dealer/**/Mocks.generated.swift"
  - Mobile/Libraries
  - "**/.build/**"

# relax all those (we could/should tighten those later on)
# line_length: 110
file_length: 1000
type_name:
  min_length: 4
  max_length: 55
function_parameter_count: 8

custom_rules:
  no_singleton_variable:
    include: ".*\\.swift"
    exclude: ".*Test.*\\.swift"
    name: "No singleton variable"
    regex: "^[ \\t]+(?:(if )|(private (?!func ))|(let )|(var )|(weak )|(unowned )|(@(IB\\w+) ))([^=:]+(: ?\\w+)?) ?= ?[\\w.]+(shared|singelton|sharedInstance).+$"
    message: "Please inject singletons into classes or methods. This helps to make the testing easier and don't hide the dependency."
    severity: warning

  private_iboutlet_variable:
    include: ".*\\.swift"
    exclude: ".*Test.*\\.swift"
    name: "IBOutlet definitions should be private or fileprivate to avoid side effects from modifiing them outside of their container."
    regex: "^[ \\t]+@IBOutlet +(weak +)?var +[a-zA-Z0-9]+ *: *[a-zA-Z0-9]+(\\!|\\?)$"
    message: "Please use UI detached properties for assigning values to the final UI element within this class."
    severity: warning

  no_empty_line_before_closing_brace:
    include: ".*\\.swift"
    exclude: ".*Test.*\\.swift"
    name: "For readability, please avoid having an empty line before a brace."
    regex: '(\n[ \t]?\n)+[ \t]?\}'
    message: "Remove the empty line in front of the closing brace."
    severity: warning

  empty_line_after_closing_brace:
    include: ".*\\.swift"
    exclude: ".*Test.*\\.swift"
    name: "For readability, please have an empty line after a brace before a new code line starts."
    regex: '\}[ \t]?\n[ \t]?[a-zA-Z]'
    message: "Add an empty line in after the closing brace."
    severity: warning

  #no_empty_string_after_nil_coalescing:
  #  include: ".*\\.swift"
  #  excluded:
  #    - ".*Test.*\\.swift"
  #    - "Packages/Dealer"
  #    - "Packages/Consumer/Features/UserConsent"
  #  name: "Prefer using unwrapOrBlank over using empty string with nil coalescing operator"
  #  regex: '\?\? ""'
  #  message: "Please use unwrapOrBlank."
  #  severity: warning

  dead_function:
    include: ".*\\.swift"
    exclude: ".*Test.*\\.swift"
    name: "Dead code"
    regex: 'func [^\n]*\{\n(\s*super\.[^\n]*\n(\s*\/\/[^\n]*\n)*|(\s*\/\/[^\n]*\n)+)\s*\}'
    message: "Delete auto-generated functions, functions calling just super or commented functions you don't use"
    severity: warning

  final_class:
    included: ".*.swift"
    name: "Final class requirement"
    regex: "^(class |@objc class |public class |internal class |private class)"
    message: "All classes must be final or non-final. If you completely sure your class if non-final, please mark it as /* @non-final */ class"
    severity: warning

  print_usage:
    included: ".*.swift"
    name: "Print usage"
    regex: '(^print| print)+\('
    message: "Please prefer os_log for logging or debugPrint if you don't need this log in production"
    severity: warning

  objc_members_usage:
    included: ".*.swift"
    name: "objcMembers usage"
    regex: 'objcMembers'
    message: "Do not use @objcMembers. It creates unnecessary amounts of code and makes removing @objc step by step impossible"
    severity: error

  use_size_class:
    included: ".*.swift"
    name: "Use size classes"
    regex: '(UIDevice[. ]isP(hone|ad)|userInterfaceIdiom == \.p(ad|hone))'
    message: "Make use of device size classes instead of device characteristics."
    severity: error
  • Are you using nested configurations? If so, paste their relative paths and respective contents.
  • Which Xcode version are you using (check xcodebuild -version)? Xcode 15.2 Build version 15C500b
  • 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 Instruments image Bildschirmfoto 2024-03-20 um 16 40 52

roddi avatar Mar 20 '24 16:03 roddi

This is a known issue that #5157 tries to address. It manifests when wildcards used in the excluded list resolve to a lot of files.

Changes in this area caused some slight differences in the set of linted files in the past which led to other issues. We should be really sure that a changed algorithm still produces the same matches. As pointed out in a comment, real world testing is difficult for us. So if you'd like to support, please try with #5157 if the performance issue is fixed, but the output is unchanged at the same time.

Off topic: Instead of your custom rule private_iboutlet_variable you might want to try the built-in (and opt-in) rule private_outlet.

SimplyDanny avatar Mar 20 '24 22:03 SimplyDanny

Thank You for your answer. I will try and report back.

roddi avatar Mar 21 '24 09:03 roddi

We should be really sure that a changed algorithm still produces the same matches.

Short answer: it doesn't seem so. Give me a bit of time for a deeper analysis.

roddi avatar Mar 21 '24 10:03 roddi

Ok, I figured it out: Looks like the exclude: part of custom rules is not used in the fix. To be honest the regex in our config is highly questionable and needs to go away. So from my perspective the change is good to go.

But in the interest of science and correctness that part of the fix doesn't work. See screenshot. brew 0.53.0 of the left, fix on the right. (I removed the (…/…) part of the output and sorted the lines. I can post the playground code if you are interested.) Bildschirmfoto 2024-03-21 um 14 32 50

roddi avatar Mar 21 '24 13:03 roddi

I just realized there is a line - "**/*Test*" in the global exclude section, so the whole exclusion seems broken.

roddi avatar Mar 21 '24 14:03 roddi

Thanks for playing around with the fix!

Looks like the exclude: part of custom rules is not used in the fix.

Yes, it's not part of the fix but should be taken into account at a later step when the rule runs. So if that doesn't work, there might be another issue.

I just realized there is a line - "**/*Test*" in the global exclude section, so the whole exclusion seems broken.

I could reproduce that in a unit test. The thing here is that the last component could match files in the previous folder(s) or all folders (and their contained files) named *Test*. That should be handled correctly now in the updated PR. Please try again.

SimplyDanny avatar Mar 21 '24 22:03 SimplyDanny