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

Performance/Sum not autocorrecting injects or reduces with blocks #171

Closed
jmkoni opened this issue Sep 21, 2020 · 4 comments · Fixed by #188
Closed

Performance/Sum not autocorrecting injects or reduces with blocks #171

jmkoni opened this issue Sep 21, 2020 · 4 comments · Fixed by #188

Comments

@jmkoni
Copy link

jmkoni commented Sep 21, 2020

Expected behavior

After enabling Performance/Sum, I expect that the following lines will all be autocorrected to use sum:

[1, 2, 3].inject(:+)
[1, 2, 3].reduce(10, :+)
[1, 2, 3].reduce { |acc, elem| acc + elem }

Actual behavior

It only autocorrects the second line:

[1, 2, 3].inject(:+)
[1, 2, 3].sum(10)
[1, 2, 3].reduce { |acc, elem| acc + elem }

Steps to reproduce the problem

  1. Create a test file with those lines.
  2. Run rubocop -A
    OR
    You can replicate this bug by pulling down this project, doing a bundle install, and running rubocop -A

RuboCop version

rubocop -V
0.91.0 (using Parser 2.7.1.4, rubocop-ast 0.4.2, running on ruby 2.7.0 x86_64-darwin19)
@jmkoni
Copy link
Author

jmkoni commented Oct 5, 2020

[1,2,3].reduce(:+) also doesn't autocorrect

jmkoni pushed a commit to standardrb/standard that referenced this issue Oct 6, 2020
- Fixes #208
- The rule isn't [working as intentended](rubocop/rubocop-performance#171)
@fatkodima
Copy link
Contributor

This is an expected behavior, example - https://github.com/rubocop-hq/rubocop-performance/blob/368b748d1c6df62002f2a1b58e4258100e693841/spec/rubocop/cop/performance/sum_spec.rb#L73-L80

We cannot autocorrect when initial value is not given, because we do not know array item types.
For example, if we try to autocorrect arr.inject(:+) to arr.sum, where arr = ['foo', 'bar'], then this will produce an error, because it should be autocorrected to arr.sum('').

@fatkodima
Copy link
Contributor

Or we can autocorrect, but mark this cop as unsafe.

koic added a commit to koic/rubocop-performance that referenced this issue Nov 5, 2020
Fixes rubocop#171.

This PR makes `Performance/Sum` cop can be changed auto-correction scope
depending on the value of `SafeAutoCorrect` in .rubocop.yml.

Its auto-correction is marked as safe by default (`SafeAutoCorrect: true`)
to prevent `TypeError` in auto-correced code when initial value is not
specified as shown below:

```ruby
['a', 'b'].sum # => (String can't be coerced into Integer)
```

Therefore if initial value is not specify as in that code, it will not be auto-corrected.

If user always want to enable auto-correction, user can set `SafeAutoCorrect: false`.

```yaml
Performance/Sum:
  SafeAutoCorrect: false
```

`SafeAutoCorrect` remains `true` by default, so the behavior does not change.
@jmkoni
Copy link
Author

jmkoni commented Nov 9, 2020

Somehow I was not notified on these updates. Thank you for the clarification.

koic added a commit to koic/rubocop-performance that referenced this issue Nov 12, 2020
Fixes rubocop#171.

This PR makes `Performance/Sum` cop can be changed auto-correction scope
depending on the value of `SafeAutoCorrect` in .rubocop.yml.

Its auto-correction is marked as safe by default (`SafeAutoCorrect: true`)
to prevent `TypeError` in auto-correced code when initial value is not
specified as shown below:

```ruby
['a', 'b'].sum # => (String can't be coerced into Integer)
```

Therefore if initial value is not specify as in that code, it will not be auto-corrected.

If user always want to enable auto-correction, user can set `SafeAutoCorrect: false`.

```yaml
Performance/Sum:
  SafeAutoCorrect: false
```

`SafeAutoCorrect` remains `true` by default, so the behavior does not change.
@koic koic closed this as completed in #188 Nov 14, 2020
koic added a commit that referenced this issue Nov 14, 2020
…ormance_sum

[Fix #171] Extend auto-correction support for `Performance/Sum`
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

Successfully merging a pull request may close this issue.

2 participants