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

Style/ClassEqualityComparison gives incorrect message and autocorrection when comparing class names by calling Module#name #9582

Closed
caalberts opened this issue Mar 10, 2021 · 3 comments
Labels

Comments

@caalberts
Copy link
Contributor

@caalberts caalberts commented Mar 10, 2021

When calling Module#name in a class equality comparison, the error message and autocorrection contains Class.name, when it should be just Class

Expected behavior

The following test should pass:

  it 'registers an offense and corrects when comparing module name for equality' do
    expect_offense(<<~RUBY)
      var.class.name == Date.name
          ^^^^^^^^^^^^^^^^^^^^^^^ Use `instance_of?(Date)` instead of comparing classes.
    RUBY

    expect_correction(<<~RUBY)
      var.instance_of?(Date)
    RUBY
  end

Actual behavior

Rubocop message uses Date.name instead of Date:

Diff:
@@ -1,3 +1,3 @@
 var.class.name == Date.name
-    ^^^^^^^^^^^^^^^^^^^^^^^ Use `instance_of?(Date)` instead of comparing classes.
+    ^^^^^^^^^^^^^^^^^^^^^^^ Use `instance_of?(Date.name)` instead of comparing classes.

Autocorrection includes Date.name instead of Date:

expected: "var.instance_of?(Date)\n"
     got: "var.instance_of?(Date.name)\n"

Steps to reproduce the problem

Add the test to spec/rubocop/cop/style/class_equality_comparison_spec.rb

RuboCop version

Seen in 0.93.1, but the test also fails on master.

@koic koic added the bug label Mar 10, 2021
koic added a commit to koic/rubocop that referenced this issue Mar 10, 2021
…yComparison`

Fixes rubocop#9582.

This PR fixes incorrect auto-correct for `Style/ClassEqualityComparison`
when comparing `Module#name` for equality.
@splattael
Copy link

@splattael splattael commented Mar 10, 2021

Thanks for creating the issue @caalberts and @koic for the quick PR 🙇 ❤️

I was wondering if it's safe to assume if var.class.name == Date.name can always be corrected to var.instance_of?(Date) in cases where .name is overridden.

Please consider this example based on RuboCop warning in GitLab which raises a warning for this bit of code.

I think as long as no-one overrides .name we safe to do the auto-correction but the auto-correction might cause bugs if .name is overridden like:

# frozen_string_literal: true

class Foo
  def self.name
    'hi'
  end
end

views = [Foo.new]

# Before
p views.find { |v| v.class.name == Foo.name }

# After (auto-)correction
p views.find { |v| v.instance_of?(Foo.name) } # fails with `instance_of?': class or module required (TypeError)

Note that p views.find { |v| v.instance_of?(Foo) } would work in this case but I am not sure if there are edges where it breaks 🤷

Should the cop stop warning in those ☝️ cases where .name used? 🤔

@bbatsov
Copy link
Collaborator

@bbatsov bbatsov commented Mar 10, 2021

I was wondering if it's safe to assume if var.class.name == Date.name can always be corrected to var.instance_of?(Date) in cases where .name is overridden.

Well, it's definitely not super safe, but that's a common problem we're facing and in general we aim to optimize for the most common scenario. E.g. in my entire career I've never seen name overriden on any project I worked on, so this seems like a rather uncommon scenario to me. That being said - it'd be great if expanded the documentation to mention this can be a problem. Down the road I want each cop to have a detailed section on its safety, that outlines assumptions and potential problems.

bbatsov added a commit that referenced this issue Mar 10, 2021
…ison`

Fixes #9582.

This PR fixes incorrect auto-correct for `Style/ClassEqualityComparison`
when comparing `Module#name` for equality.
@splattael
Copy link

@splattael splattael commented Mar 10, 2021

@bbatsov

E.g. in my entire career I've never seen name overriden on any project I worked on, so this seems like a rather uncommon scenario to me.

Same here but then I checked GitLab's code:

$ rg "def self.name" -l | wc -l
79

🙈

Anyhow, that's why we have specs and the ability to disable cops in certain cases if needed 😅

Thanks for checking and merging 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants