AssignmentInCondition didn't find the style problem

[itsalongstory]

Expected behavior

RuboCop should find the style problem in the if statement

Actual behavior

RuboCop didn't find the style problem

Steps to reproduce the problem

test.rb

a = 1

if a&.to_i = 1
  puts '111'
end

Run:

rubocop --only Lint/AssignmentInCondition -a test.rb

Result:

Inspecting 1 file
.

1 file inspected, no offenses detected

RuboCop version

root@localhost:~$ rubocop -v
1.61.0

[jonas054]

I find this example to be too contrived. Running the code produces this output:

$ ruby test.rb 
test.rb:3:in `<main>': undefined method `to_i=' for 1:Integer (NoMethodError)
Did you mean?  to_i
               to_int
               to_c
               to_r
               to_f
               to_s

Can you give a better example where the inspected code is more realistic?

[itsalongstory]

@jonas054

Sorry for bad example, I will give a more realistic example.

test.rb

require 'bundler/inline'

gemfile(true) do
  source 'https://rubygems.org'
  gem 'activerecord', '~> 7.0', '>= 7.0.6', require: "active_record"
  gem 'sqlite3', '~> 1.6', '>= 1.6.3'
  gem 'minitest', '~> 5.19', require: "minitest/autorun"
end

ActiveRecord::Base.establish_connection(
  adapter:  "sqlite3",
  database: "./test.db"
)

ActiveRecord::Schema.define do
  drop_table(:categories, if_exists: true)
  drop_table(:topics, if_exists: true)

  create_table :categories do |t|
    t.string :name
  end

  create_table :topics do |t|
    t.belongs_to :category
    t.string :title
    t.text :content
  end
end

class ApplicationRecord < ActiveRecord::Base
  primary_abstract_class
end

class Category < ApplicationRecord
  has_many :topics
end

class Topic < ApplicationRecord
  belongs_to :category, optional: true
end

class MyTest < Minitest::Test

  Category.create(name: 'Ruby')
  Topic.create(title: 'Rubocop tutorial', category: Category.first)

  def test_1
    # rubocop will NOT fild this problem
    if Topic.first.category&.name = 'Ruby 1'
      puts 'test_1'
    end
  end

  def test_2
    # rubocop will find this problem
    if Topic.first.category.name = 'Ruby 2'
      puts 'test_2'
    end
  end
end

[itsalongstory]

root@dev-test:~$ ruby test.rb
Fetching gem metadata from https://gems.ruby-china.com/........
Resolving dependencies...
Using base64 0.2.0
Using bigdecimal 3.1.6
Using concurrent-ruby 1.2.3
Using drb 2.2.1
Using i18n 1.14.4
Using sqlite3 1.7.2 (x86_64-linux)
Using tzinfo 2.0.6
Using minitest 5.22.2
Using mutex_m 0.2.0
Using timeout 0.4.1
Using bundler 2.4.16
Using connection_pool 2.4.1
Using activesupport 7.1.3.2
Using activemodel 7.1.3.2
Using activerecord 7.1.3.2
-- drop_table(:categories, {:if_exists=>true})
   -> 0.0139s
-- drop_table(:topics, {:if_exists=>true})
   -> 0.0003s
-- create_table(:categories)
   -> 0.0006s
-- create_table(:topics)
   -> 0.0008s
Run options: --seed 48835

# Running:

test_2
.test_1
.

Finished in 0.006965s, 287.1380 runs/s, 0.0000 assertions/s.

2 runs, 0 assertions, 0 failures, 0 errors, 0 skips
root@dev-test:~$
root@dev-test:~$
root@dev-test:~$
root@dev-test:~$
root@dev-test:~$
root@dev-test:~$ rubocop --only Lint/AssignmentInCondition -a test.rb
Inspecting 1 file
W

Offenses:

test.rb:56:34: W: [Correctable] Lint/AssignmentInCondition: Use == if you meant to do a comparison or wrap the expression in parentheses to indicate you meant to assign in a condition.
    if Topic.first.category.name = 'Ruby 2'
                                 ^

1 file inspected, 1 offense detected, 1 more offense can be corrected with `rubocop -A`
root@dev-test:~$
root@dev-test:~$
root@dev-test:~$
root@dev-test:~$
root@dev-test:~$
root@dev-test:~$ ruby -v
ruby 3.1.2p20 (2022-04-12 revision 4491bb740a) [x86_64-linux]
root@dev-test:~$  rubocop -v
1.61.0
root@dev-test:~$

[jonas054]

Thanks, @itsalongstory, much better!

An idea I'm getting when looking at the example is that it would be better to add a new cop that reports offenses for code like Topic.first.category&.name = 'Ruby 1' because that code is very suspicious, even outside of the if statement. The cop could be called Lint/PossibleAssignmentToNil and report any use of &. on the left hand side of an assignment, because I can't see why you would ever need it.

[itsalongstory]

I can't see why you would ever need it.

@jonas054 , the category could be nil

class Topic < ApplicationRecord
  belongs_to :category, optional: true
end

And my original intent was to determine equality, not to assign values.

[jonas054]

Yes, I understand that. What I'm saying is that we have two major choices for how to fix what's missing in RuboCop. Either we add expressions with &. to the duties of Lint/AssignmentInCondition, or we implement them in a new Lint/PossibleAssignmentToNil cop.

Fixing Lint/AssignmentInCondition has two drawbacks IMO.

1. We're still not reporting an offense for Topic.first.category&.name = 'Ruby 1' when it's a statement of its own, even though the ampersand makes no sense.
2. We can't in good conscience autocorrect the if statement to if (Topic.first.category&.name = 'Ruby 1'). That's a bit too unsafe, even for an autocorrection marked as unsafe. So we'd need to make it a special case, with extra logic in the cop.

[jonas054]

I take back most of my comments above. I was thinking of these expressions as assignments rather than setter calls. They are setter calls, so the fix becomes pretty simple. We just need to treat &. the same as . in the context of Lint/AssignmentInCondition. A new PR is coming up.