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

[Fix #12842] Add new Style/StaticSend cop #12850

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

koic
Copy link
Member

@koic koic commented Apr 16, 2024

Fixes #12842.

Summary

Detects the use of the public_send method with a static method name argument. Since the send method can be used to call private methods, by default, only the public_send method is detected.

# bad
obj.public_send(:static_name)
obj.public_send('static_name')

# good
obj.static_name

Safety

This cop is not safe because it can incorrectly detect based on the receiver. Additionally, when AllowSend is set to true, it cannot determine whether the send method being detected is calling a private method.

AllowSend option

This cop has AllowSend option.

AllowSend: true (default)

# good
obj.send(:static_name)
obj.send('static_name')
obj.__send__(:static_name)
obj.__send__('static_name')

AllowSend: false

# bad
obj.send(:static_name)
obj.send('static_name')
obj.__send__(:static_name)
obj.__send__('static_name')

# good
obj.static_name

Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

Fixes rubocop#12842.

## Summary

Detects the use of the `public_send` method with a static method name argument.
Since the `send` method can be used to call private methods, by default,
only the `public_send` method is detected.

```ruby
# bad
obj.public_send(:static_name)
obj.public_send('static_name')

# good
obj.static_name
```

## Safety

This cop is not safe because it can incorrectly detect based on the receiver.
Additionally, when `AllowSend` is set to `true`, it cannot determine whether
the `send` method being detected is calling a private method.

## `AllowSend` option

This cop has `AllowSend` option.

### AllowSend: true (default)

```ruby
# good
obj.send(:static_name)
obj.send('static_name')
obj.__send__(:static_name)
obj.__send__('static_name')
```

### AllowSend: false

```ruby
# bad
obj.send(:static_name)
obj.send('static_name')
obj.__send__(:static_name)
obj.__send__('static_name')

# good
obj.static_name
```
@koic koic force-pushed the add_new_style_static_send_cop branch from 09071bc to 52a1e53 Compare April 16, 2024 16:54
@bbatsov
Copy link
Collaborator

bbatsov commented Apr 19, 2024

I like the cop, but I'm not sure about the name. Perhaps this cop should consider constants as well (e.g. METHOD_NAME).

@Earlopain
Copy link
Contributor

I don't think this cop should concern itself with constants (at least by default). Surely I used a constant to intentionally factor it out, unless the constant is referenced only once I'd want to keep it.

@koic
Copy link
Member Author

koic commented Apr 30, 2024

Since a constant is generally immutable by definition, I think it can always be targeted without needing options. Are there any valid cases for excluding a constant?

@Earlopain
Copy link
Contributor

There are barely any cases I could find that use public_send over send for this (send is just so much more used). This is basically what I was thinking about: https://github.com/hopsoft/turbo_boost-commands/blob/9b4535ca0fe17481e70ae292d03db5b3e984d376/test/application_system_test_case.rb#L11

It uses a constant to have one place that holds which browser is being tested. I don't believe the code will improve by calling the method directly.

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 this pull request may close these issues.

Cop Idea: Replace #send(LITERAL, ...) with method call
3 participants