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

RuboCop::Cop::Performance::Count is not safe and should be moved to the unsafe set #8

Open
jon-rse opened this issue Jun 12, 2023 · 1 comment

Comments

@jon-rse
Copy link

jon-rse commented Jun 12, 2023

Expected behavior: Running standardrb --fix will not break code.
Observed behavior: standard --fix includes cops which are unsafe.
Environment: Ruby 3.2.0, default standardrb install,

standard (1.28.2)
      language_server-protocol (~> 3.17.0.2)
      lint_roller (~> 1.0)
      rubocop (~> 1.50.2)
      standard-custom (~> 1.0.0)
      standard-performance (~> 1.0.1)

The count cop will transform

 hash.select { |k,v| k == "count me" }.count

into

 hash.count { |k,v| k == "count me" }

However the count method on a hash does not pass |k,v| to the block, but rather an array, [k,v]. This is not a safe transformation, and requires modification to the block, or the caller for the transformed code to operate correctly eg:

 hash.keys.count { |k| k == "count me" }

See this issue in rubocop where they mark the count cop as unsafe, rubocop/rubocop-performance#69

searls added a commit to standardrb/standard that referenced this issue Jun 13, 2023
searls added a commit to standardrb/standard that referenced this issue Jun 13, 2023
@searls
Copy link
Contributor

searls commented Jun 13, 2023

Confirmed this with a test in standard.

$ bundle exec m test/standard/cli_test.rb:36
Run options: -n "/^(test_unsafely_fixable_is_not_fixed_unsafely)$/" --seed 2437

# Running:

F

Finished in 0.551967s, 1.8117 runs/s, 7.2468 assertions/s.

  1) Failure:
Standard::CliTest#test_unsafely_fixable_is_not_fixed_unsafely [/Users/justin/code/standardrb/standard/test/standard/cli_test.rb:36]:
--- expected
+++ actual
@@ -2,7 +2,5 @@
 " +
 "  test/fixture/cli/unsafecorrectable-bad.rb:1:7: Lint/BooleanSymbol: Symbol with a boolean name - you probably meant to use `true`.
 " +
-"  test/fixture/cli/unsafecorrectable-bad.rb:6:24: Performance/Count: Use `count` instead of `select...count`.
-" +
-"standard: Run `standardrb --fix-unsafely` to DANGEROUSLY fix 2 problems.
+"standard: Run `standardrb --fix-unsafely` to DANGEROUSLY fix one problem.
 "

@koic, can you help me understand this? When I look at the source for these two unsafe cops (Performance/Count and Lint/BooleanSymbol, I can't tell what in their code listings are doing to tell RuboCop that their fix is unsafe, so I can't figure out what Standard is doing wrong here.

AFAICT, we're setting the two autocorrection flags correctly in our CLI config: https://github.com/standardrb/standard/blob/30fec014c448fed60a43ac48d8110967f7430498/lib/standard/merges_settings.rb#L27-L40

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

No branches or pull requests

2 participants