Add RSpec/StringAsInstanceDoubleConstant

[corsonknowles]

Begins Addressing: #1136

Adds a cop which can autocorrect from String declarations for instance_double to Class declarations. Passing a variable -- which could hold a String or a Class -- is not affected by this cop.

Autocorrect is not marked as safe, because the correction will cause instantiation of the Class, which depending on test setup may or may not have been loaded yet. In addition, instantiating the Class before stubbing an instance_double means that RSpec will treat it as a verifying double. A string reference to a Class that is not loaded is treated as a normal non-verifying double.

We have been running this as safe autocorrect internally for years, and have not met with any issues as we use Zeitwerk and eager load once before running specs. In pushing this upstream, I don't want to assume that all test suites will have autoloading in place.

---

Before submitting the PR make sure the following are checked:

• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Updated documentation.
• Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
• The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

If you have created a new cop:

• Added the new cop to config/default.yml.
• The cop is configured as Enabled: pending in config/default.yml.
• The cop is configured as Enabled: true in .rubocop.yml.
• The cop documents examples of good and bad code.
• The tests assert both that bad code is reported and that good code is not reported.
• Set VersionAdded: "<<next>>" in default/config.yml.

If you have modified an existing cop's configuration options:

• Set VersionChanged: "<<next>>" in config/default.yml.

[corsonknowles]

Getting

* generated /Users/.../Documents/GitHub/rubocop-rspec/docs/modules/ROOT/pages//cops_rspec.adoc
rake aborted!
Please run `bundle exec rake generate_cops_documentation` and add docs/ to the commit.
/Users/.../Documents/GitHub/rubocop-rspec/Rakefile:79:in `block in <top (required)>'
/Users/.../.rbenv/versions/3.3.0/bin/ruby_executable_hooks:22:in `eval'
/Users/.../.rbenv/versions/3.3.0/bin/ruby_executable_hooks:22:in `<main>'
Tasks: TOP => default => confirm_documentation
(See full trace by running task with --trace)

However, have run rake generate_cops_documentation

[bquorning]

However, have run rake generate_cops_documentation

Did you also “add docs/ to the commit”? When I run the rake task I see a diff in the docs folder.

[pirj]

There are some wip rubocop cop naming guidelines, and they insist on naming cops so that they highlight the problem. This should be “StringifiedInstanceDoubleConstant”

[corsonknowles]

Did you also “add docs/ to the commit”? When I run the rake task I see a diff in the docs folder.

Looks like my rake problems were mostly self-inflicted, I tried to add my own documentation while I was following the contribution guidelines.

For ease of new development, I'd recommend updating all of our rake task feedback to say "bundle exec rake etc" so it's easier to copy and execute

[technicalpickles]

I am biased since it was extracted from our app, but in favor of this 👍🏻

Interestingly, instance_double does support a string as the name, at least according to its rdoc. I am looking at some of the implementation (instance_double and ObjectReference.for(doubled_class) but I couldn't tell you what happens for classes vs strings.

[bquorning]

Two questions:

1. Could / should we make the same rule for class_double too?
2. Shouldn’t we also detect and replace symbols? As I understand the source, calling instance_double with a symbol won’t even work.

[corsonknowles]

I don't think we need to extend the cop to cover symbols because RSpec will raise like this:

     ArgumentError:
       Module or String expected, got :Task

I have updated the PR description to be clearer about that.

[corsonknowles]

I think enforcing this for class_double would be another Cop, right?

[bquorning]

I think enforcing this for class_double would be another Cop, right?

@pirj What’s your take on this?

[pirj]

No errors whatsoever during a run against real-world-rspec. 499 offences look legit at a glance.

Thank you!

A few minor tweaks, and good to go.

[corsonknowles]

As an aside, while following the test coverage results, I noticed what appears to be a dead code method:
top_level_group?

https://github.com/search?q=repo%3Arubocop%2Frubocop-rspec%20top_level_group%3F&type=code

All specs pass without it.

The only other uncovered line is raise ::NotImplementedError

I'd be happy to raise another PR after this, if its useful. I don't know if you prefer to handle all maintenance PRs internally.

[bquorning]

As an aside, while following the test coverage results, I noticed what appears to be a dead code method:
top_level_group?

Yeah, it seems its last use was removed in 6276370 / #977.

I'd be happy to raise another PR after this, if its useful. I don't know if you prefer to handle all maintenance PRs internally.

You’re more than welcome to open a PR to remove that method 👍🏼

[corsonknowles]

Are we ready to merge here?

[bquorning]

The original issue #1957 mentions all of double/instance_double/class_double/object_double/spy etc. Since the cop name includes the method name instance_double, I think before we merge we should talk a bit more about if this cop should also police some of the other methods, of we want to have separate cops for each.

[pirj]

Even though for the rest of double methods we may want to enforce consistency, the reason for that would be … just consistency. While for instance_double that is to enforce verification.

My only question is if the class_double is the same?

For the rest I’d vote for a separate cop.

[corsonknowles]

Thanks @pirj --- so the problem with adding class_double is that it is a complete re-write.
We need to:

1. Rename the cop
2. Rewrite the description
3. Redo the matcher
4. Double the specs
5. Re-run the new rule on live codebases

Then I have an additional downstream problem where that rule doesn't exist in my codebase, so instead of merging this and then pulling the upstream, I need to go work through all class_doubles at the same time.

I guess what I'm saying is it's much more preferable for me to decouple these things.

In comparison to all of the above, it's relatively trivial for anyone to duplicate this new Cop and adapt it to work for class_double -- it's just a find and replace exercise, very modular.

I can't think of any performance reasons to combine them either, with restrict on send in place. A combined rule would need an OR matcher like {instance_double class_double} , or to leverage something to insert the method name in the pattern, if something like that exists.

[pirj]

Makes total sense to me. Thanks for the explanation.

[bquorning]

Yep, that makes sense. Thank you 👍🏼

[bquorning]

Hi @corsonknowles, I think you rebased the branch yesterday, but it looks like you have got a few extra/unintended changes in. Would you mind taking a look before we merge?

[corsonknowles]

Thanks @bquorning, amended!

[bquorning]

Thank you for the contribution, and for your patience with our nitpicking 😅

[lucthev]

👋 Hello! I was looking through the offences this cop turned up in the codebase I work on, and noticed they're identical to those produced by RSpec/VerifiedDoubleReference (which we had disabled via TODO file). The docs would suggest they're doing largely the same thing — is there any meaningful difference that could be clarified in the docs?

[pirj]

Hi!
Nice catch, we’ve missed this completely.

The new cop is essentially the old one, but with no option to select the style. And this is certainly the direction we choose to enforce.

Your input is valuable, why did you decide to turn the cop off?

In a nutshell, we’ve discovered that an instance_double with a string argument does not verify, and behaves identically to a regular double. It makes sense to make the usage explicit, and to use a double for abstract doubles, and instant_double for specific ones, and verify them.

[lucthev]

why did you decide to turn the cop off

We didn't totally turn off VerifiedDoubleReference, just ignored a number of existing offences via .rubocop_todo.yml — it's a large, old codebase, but Rubocop was only added semi-recently. It's definitely a useful cop for the reasons you've outlined!

That being said, if this new cop is wholly redundant, then I'll likely turn it off 😅. Would it make sense to deprecate one or the other? For what it's worth, VerifiedDoubleReference appears to support e.g. class_double as well, which I see was a concern raised in this PR.

[pirj]

Thanks for sharing.

Yes, it would make sense to deprecate one of the cops, and I’d remove the EnforcedStyle: string option from the old one if it is less intrusive.

[corsonknowles]

Ah, thank you for immediately reporting this Luc!

I'm happy to deprecate the new cop and focus on updating the original cop.

So the action items are

1. deprecate StringAsInstanceDoubleConstant -- is there a way to make it an alias of the other cop?
2. remove String style from VerifiedDoubleReference as it is not consistently verifying. Is there a way to deprecate this or do we simply remove it?

I'll also take a look if there's anything in the new implementation worth porting over to [VerifiedDoubleReference](https://docs.rubocop.org/rubocop-rspec/cops_rspec.html#rspecverifieddoublereference

I can probably get started on this on Monday. If anyone wants to pitch in or get started earlier, you are by all means welcome to.

[corsonknowles]

I guess we could have it inherit from VerifiedDoubleReference and add the deprecation message, if that makes sense?