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

Glob.resolveGlob() is veeeery sloooow #5501

Open
2 tasks done
roddi opened this issue Mar 20, 2024 · 6 comments
Open
2 tasks done

Glob.resolveGlob() is veeeery sloooow #5501

roddi opened this issue Mar 20, 2024 · 6 comments
Labels

Comments

@roddi
Copy link
Contributor

roddi commented Mar 20, 2024

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
@SimplyDanny
Copy link
Collaborator

SimplyDanny commented Mar 20, 2024

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.

@roddi
Copy link
Contributor Author

roddi commented Mar 21, 2024

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

@roddi
Copy link
Contributor Author

roddi commented Mar 21, 2024

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
Copy link
Contributor Author

roddi commented Mar 21, 2024

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
Copy link
Contributor Author

roddi commented Mar 21, 2024

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

@SimplyDanny
Copy link
Collaborator

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.

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

No branches or pull requests

2 participants