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

Rroblem with Style/HashSyntax cop in Ruby 3.1 #10359

Closed
tycooon opened this issue Jan 17, 2022 · 5 comments · Fixed by #10363
Closed

Rroblem with Style/HashSyntax cop in Ruby 3.1 #10359

tycooon opened this issue Jan 17, 2022 · 5 comments · Fixed by #10363
Labels

Comments

@tycooon
Copy link

tycooon commented Jan 17, 2022

When using ruby 3.1 hash syntax, I get the Lint/UselessAssignment cop warning.

Expected behavior

No warnings.

Actual behavior

1.rb:4:3: W: Lint/UselessAssignment: Useless assignment to variable - user.
  user = double :user, id: 42, token:
  ^^^^

1 file inspected, 1 offense detected

Steps to reproduce the problem

def some_method(token)
  user = double :user, id: 42, token:
  described_class.build(user)
end

Version with brackets works as expected:

def some_method(token)
  user = double(:user, id: 42, token:)
  described_class.build(user)
end

RuboCop version

1.24.1 (using Parser 3.1.0.0, rubocop-ast 1.15.1, running on ruby 3.1.0 arm64-darwin21)

Update:

It turns out that Ruby interprets the first one as

  user = double :user, id: 42, token: described_class.build(nil)

So technically this is not a false positive. However, this also means that auto-correction for Style/HashSyntax cop is not safe in this case. So probably this should be fixed somehow.

@tycooon tycooon changed the title False positive for Lint/UselessAssignment cop Rroblem with Style/HashSyntax cop in Ruby 3.1 Jan 17, 2022
@koic
Copy link
Member

koic commented Jan 17, 2022

In Ruby 3.1, the following code is equivalent. So, line breaked expression is the hash value.

def some_method(token)
  user = double :user, id: 42, token:
  described_class.build(user)
end
def some_method(token)
  user = double :user, id: 42, token: described_class.build(user)
end

This behavior is being considered in the following Ruby core issue.
https://bugs.ruby-lang.org/issues/18396

So technically this is not a false positive. However, this also means that auto-correction for Style/HashSyntax cop is not safe in this case. So probably this should be fixed somehow.

This issue has been resolved by #10357. It will be included in the next release.

@koic koic closed this as completed Jan 17, 2022
@tycooon
Copy link
Author

tycooon commented Jan 18, 2022

I tested with version 1.25.0 and it looks like it still breaks the code with auto-correction even in safe mode. For example, it replaces

build :user, token: token
SomeClass.new(true) 

with

build :user, token:
SomeClass.new(true)

Which breaks the code. Note that if there is SomeClass.new without an argument, it doesn't report a warning.

Also, now it doesn't do auto-correction in a lot of safe cases even with EnforcedShorthandSyntax: always option. So I guess some more fixes are needed.

@tycooon
Copy link
Author

tycooon commented Jan 18, 2022

Example when 1.25 doesn't report a warning where 1.24 did:

p Hash[x: x]

and

def attrs(user, token)
  {
    user: user,
    token: token
  }
end

@koic
Copy link
Member

koic commented Jan 19, 2022

@tycooon Your feedback is correct. I've opened #10363 to resolve the issues. Thank you!

koic added a commit to koic/rubocop that referenced this issue Jan 19, 2022
…Syntax`

Fixes rubocop#10359.

This PR fixes a false positive and negative for `Style/HashSyntax`
when using hash value omission.

It resolves the lack of consideration for https://bugs.ruby-lang.org/issues/18396
and false positives for modifier forms.
bbatsov pushed a commit that referenced this issue Jan 19, 2022
Fixes #10359.

This PR fixes a false positive and negative for `Style/HashSyntax`
when using hash value omission.

It resolves the lack of consideration for https://bugs.ruby-lang.org/issues/18396
and false positives for modifier forms.
@tycooon
Copy link
Author

tycooon commented Jan 19, 2022

Great, thank you 👍

kevindew added a commit to alphagov/rubocop-govuk that referenced this issue Jun 6, 2022
This includes better compatibility with Ruby 3.1 due to rubocop/rubocop#10359

This release has been tested against:

- Content Publisher - correctable RSpec/ExpectChange issues https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/ExpectChange
- govuk_publishing_components - no issues
- Search API - no issues
- Whitehall - no issues
kevindew added a commit to alphagov/rubocop-govuk that referenced this issue Jun 7, 2022
This includes better compatibility with Ruby 3.1 due to rubocop/rubocop#10359

This release has been tested against:

- Content Publisher - correctable RSpec/ExpectChange issues https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/ExpectChange
- govuk_publishing_components - no issues
- Search API - no issues
- Whitehall - no issues
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants