IndexedLet should not flag let-vars that correspond to keyword args

[jasonkarns]

Is your feature request related to a problem? Please describe.

The IndexedLet cop is complaining about let-vars that are named specifically to match keyword args (in particular, keyword args in the UUT). Adhering to this cop makes the let-vars inconsistent with the keyword args, and worse, makes it so kwarg shorthand can't be used.

Describe the solution you'd like

I would like the IndexedLet cop to automatically allow (without any configuration) any offending let-vars used as kwarg values. (ie, any let-vars whose name matches a kwarg and would be using kwarg shorthand)

Describe alternatives you've considered

One option is to rename the kwarg to match the IndexedLet-compliant names. However, in many (most?) cases, this would mean that a rubocop-rspec rule is forcing changes to application code, which is a tenuous line to cross.

Another option would be to explicitly list the allowed values. However, whether a let-var should be flagged or allowed is context dependent. In one case that maps to kwargs and uses shorthand, they should be allowed; but in another case where they are positional args or any other regular variable reference they should still be flagged. So listing them as exceptions is overly broad.

Additional context

Example that presently violates IndexedLet that should be allowed:

    let(:line1) { "333 Green St" }
    let(:line2) { "Unit 1000" }
    let(:address) { Address.new(line1:, line2:, city:, state:, zip:) }

[pirj]

Maybe just allow line1 and line2 in cop config?

[jasonkarns]

@pirj that's overly broad. line1 and line2 should only be allowed when they are keyword args used for shorthand. they aren't domain terms or anything special that should be allowed everywhere all the time.

Another option would be to explicitly list the allowed values. However, whether a let-var should be flagged or allowed is context dependent. In one case that maps to kwargs and uses shorthand, they should be allowed; but in another case where they are positional args or any other regular variable reference they should still be flagged. So listing them as exceptions is overly broad.

The underlying problem is that this cop is presuming that any let-var that ends in a numeral is "one of many" let-vars. (ie, that the test is setting up a handful bits of data representing different scenarios, etc) But when the let-var ends in a numeral due to that name in application code (ie, kwargs), then the numeral does not indicate the codesmell this cop is searching for. Ergo, the matches are false positives. But as noted above, the actual name line1 isn't necessarily special. It could just as easily be a codesmell in some_other_spec.rb that ought to be flagged.

[pirj]

It’s impossible to reliably detect if a let-var was ysed anywhere as a kwarg.
Also, what if it’s used in one place as a kwarg, while in another place its usage should be considered as an offence?
What if it’s not used in shorthand syntax?
What if it’s used in shorthand syntax to build a Hash?

Too many factors and cases to consider.

On the other hand, if some project makes extensive use of line1/line2 in specs, it’s easy for them to allowlist those two.

I’m not convinced we should go any further than improving the docs of the cop with an example where line1/line2 are legitimate.
WDYT @rubocop/rubocop-rspec ?

[bquorning]

For this to work, we could index all application code and make a list of keyword arguments in all method definitions. But we would probably not capture all method definitions (think define_method etc.) and we would still know nothing about methods defined outside the application code (e.g. gem dependencies).

We would need to do a lot more work than we do today, and there would still be false negatives. So no, I don’t think this is something we want to do.

The solution must be to either 1) configure the cop, or 2) disable the cop. And yes, perhaps we should add some more documentation to the cop as @pirj suggests.

[pirj]

A PR to improve docs on how to configure the cop is welcome!

[jasonkarns]

For this to work, we could index all application code and make a list of keyword arguments in all method definitions

I don't think any of that is necessary. The scope of the let-var is only within a single file, so its use as a keyword arg is only relevant within that single file.

But we would probably not capture all method definitions

Method definitions are also not relevant. The only exception is when a letvar is used as a keyword arg in its invocation, so we don't care about method definitions, only method invocations.

Literally the only thing we have to look for: "is this letvar used as a keyword arg or hash value in this same file". If so, ignore that particular offence.

[bquorning]

Hm, yes. Limiting its scope to within the same file might work. Of course there may be edge cases with shared_setup and such, but that may be ok.

I'd be curious to see an implementation, seeing how much extra complexity we'd take on.

And the issue mentioned by @pirj persists: the cop reporting different results for different files may confuse users. (Documentation may fix that.)

[jasonkarns]

And the issue mentioned by @pirj persists: the cop reporting different results for different files may confuse users.

agreed. My thinking is that this "softer" rule could be a :contextual option or something.

I also think the "different results" is likely tempered by the fact that you'd get an offense in one file but nothing in another file. So while that's technically different results per file, practically speaking most users would likely be unaware of the lack of offenses in other files.