Always correct conditional to multiline in CallbackConditionalsBinding

[vinistock]

This PR applies a few fixes to the CallbackConditionalsBinding cop. It also adds support for using proc in the conditional instead of a lambda.

Always breaks lines for multi statement procs

# this
if: -> { something && other_thing }

# no longer is corrected to
if: -> { T.bind(self, Class); something && other_thing }

# And is instead broken down to
if: -> {
  T.bind(self, Class)
  something && other_thing
}

Substitute T.unsafe usage for bind

# this
if: -> { T.unsafe(self).something? }

# becomes
if: -> { T.bind(self, Class).something? }

Array conditionals are not considered offenses anymore

# this is okay
if: [:method1, :method2]

Procs that accept arguments are not considered offenses

# this could only be corrected when the receiver is the proc's argument, but it gets pretty complicated
# Since these don't produce type errors, I think it's okay to not try to auto correct it
if: ->(record) { invoke_method(record) }

Conditionals where the recursive receiver is not self are now corrected to multiline

# this
if: -> { %w(a b).include?(some_method) }

# becomes
if: -> {
  T.bind(self, Class)
  %w(a b).include?(some_method)
}

Chained send statements are properly corrected in a single line

# this
if: -> { something.present? }

# becomes
if: -> { T.bind(self, Class).something.present? }

[RyanBrushett]

I dig it

[Morriar]

Can't it be as problem with this?

something: -> { T.unsafe(self).passing_splat(*args) }

[vinistock]

Ugh, yes, that would be indeed be an issue. Should we just ignore T.unsafes then?

[Morriar]

If we're going to change working code into non-working code yes.

Another solution is to move the T.unsafe part in another cop that is optional and just used to auto-correct the T.unsafe in T.bind

[vinistock]

I made two corrections based on recent feedback.

1. I removed the T.unsafe auto-correct since it can lead to incorrect code
2. Started using the fully qualified name of constants only if the namespace is defined on the same line (*)

• If we always use the fully qualified name, then we can accidentally create Sorbet errors for private constants. E.g.:

module Something
  class PrivateClass
    # Here if we auto correct to the fully qualified name, it will give a typing error of non-private reference to a private constant
    before_create :do_it, if: -> { T.bind(self, Something::PrivateClass)... }
  end
  private_constant(:PrivateClass)
end

However, if we never use the fully qualified name, then these scenarios will also fail:

class Something::MyClass
  # If we bind to MyClass, Sorbet will error saying it cannot find the definition of MyClass
  before_create :do_it, if: -> { T.bind(self, MyClass)... }
end

Therefore, the cop will now auto-correct using the fully qualified name for constants defined in this pattern Namespace::Class and will use the simple name for all other cases.

[Morriar]

Is this change expected?

[vinistock]

I assume it happened because my bundler version is newer. Would you prefer to roll this back? IMO, I'd just keep it

[Morriar]

Let's remove it 🔥

[vinistock]

Added another test case for when two classes are defined inside the same namespace.

module Something
  class Namespace::Klass
    # Properly corrects to Namespace::Klass because it's defined on the same line
    validates :it, if: -> { T.bind(self, Namespace::Klass).should? }
  end

  class AnotherOne
    # Properly corrects to AnotherOne because using the fully qualified name here is not necessary
    validates :it, if: -> { T.bind(self, AnotherOne).should? }
  end
end

[Morriar]

Let's remove it 🔥

[Morriar]

Shouldn't the block be changed in a do .. end if it's going to be multiline? In our style guide we enforce BlockDelimiters.

[vinistock]

I think there's already a cop for fixing {} into do, end, which would pick that up. Also, it can get quite complex when the lambda has parameters or whether you use -> or lambda and this cop is complex enough as it is.

[Morriar]

Fair 👍

[Morriar]

Fair 👍