Add Node#string_type? helper

[dvandersluis]

We have a number of places in rubocop where we check for str_type? || dstr_type? or str_type? || dstr_type? || xstr_type?.

In order to refactor those to reduce complexity, but still maintain the ability to do both types of checks, I've added two helper methods to Node:

• Node#string_literal? => node.str_type? || node.dstr_type?
• Node#string_type? => node.str_type? || node.dstr_type? || node.xstr_type?

I'm not 100% if the distinction between the two is too unclear, so I'm receptive to suggestions there. My thinking is that string_literal? are strings that can be written in quotes, whereas xstr is slightly different (since, among other things, it actually executes code), but still a "string" type.

[dvandersluis]

Found an incorrect test description so I fixed it.

[marcandre]

Ouf, it's really unclear to me if there's a way to make any of string_type? / string_literal? clear.

I can imagine node.str_type? || node.dstr_type? being useful, but I also couldn't find much places in the code where str_type? || dstr_type? || xstr_type? would be...?

[dvandersluis]

I originally had excluded xstr but there are four usages in the rubocop org that I found:

1. https://github.com/rubocop/rubocop/blob/68bfccbf83901a396801533c1e40c2c31c478f74/lib/rubocop/cop/autocorrect_logic.rb#L112
2. https://github.com/rubocop/rubocop/blob/c68385b714d47b6794a8b529db6cade54b960e1a/lib/rubocop/cop/mixin/check_line_breakable.rb#L230
3. https://github.com/rubocop/rubocop/blob/873cb9f1dfd2d02a574f0ad95de36a3f33547b3a/lib/rubocop/cop/style/redundant_exception.rb#L54
4. https://github.com/rubocop/rubocop-rspec/blob/5d96600242e849526a2aaa6e35ecf16c2982ff87/lib/rubocop/cop/rspec/predicate_matcher.rb#L177

I'm ok with excluding xstr_type? and using node.string_type? || node.xstr_type? if that works better for you.

[marcandre]

Thanks for the list.

... if that works better for you.

It's not really for me, but for everyone. This one really seems tricky. Maybe I'm thinking too much about it and xstr really doesn't matter much, but it's simply not clear (to me) if it should or should not be excluded.

For example, the 3rd example would be for RuboCop to actually correct code like

raise RuntimeError, `???`

That's a good example where I don't think anybody cares how this xstr is auto-corrected.
I'm curious of examples that use str_type? and dstr_type? but not xstr_type? if they have good reasons to exclude xstr_type? (other than we never see that code, like the raise example above)

So I'd really like input for @rubocop/rubocop-core about if we want string_type? and/or string_literal? and if xstr should be included or not. We have to remember that string_type? means that string can be used in node patterns too.

[dvandersluis]

That's a good example where I don't think anybody cares how this xstr is auto-corrected.

Agreed. I think it probably is almost always unintended to even write code where the message of an exception is a shell execution. That could even in itself be something a cop prevents.

So I'd really like input for @rubocop/rubocop-core about if we want string_type? and/or string_literal? and if xstr should be included or not. We have to remember that string_type? means that string can be used in node patterns too.

From my perspective I do not have strong feelings about including xstr, and only did so to cover the cases I found after initially writing it without.

The reason I think that having string_type? is useful is that, aside from instances that are specifically concerned with interpolation or lack thereof, anywhere we look for a str would most likely make sense to look for a dstr as well. Similar to send and csend where there are often post hoc patches to cop to add support for the forgotten &. operator, so now we often use node.call_type? instead.

[dvandersluis]

Now that #329 is merged, I updated to define string_type? in terms of a group. I've removed xstr as well as the string_literal? method so now there's only string_type? => str_type? || dstr_type?

[marcandre]

I'd love others to chime in.

My concerns are: definition not clear / possibly confusion / hard to debug errors. The current situation (no grouping) has the advantage that one has to be explicit in specifying which of str / dstr / xstr are targetted.

Current proposal is for Node#string_type? => node.str_type? || node.dstr_type?.
This implies that xstr is excluded from that definition.
This also means that node patterns (string ...) and (str ...) would have slightly different meanings.

Better proposal?

[dvandersluis]

FWIW, If we adopt this, it probably would make sense to do the same for sym/dsym.

@marcandre I'd prefer string_type? because then it'll work in type? with the metatype groups we defined in #329, but do you have the same concerns if we use string_literal? instead of string_type??

[marcandre]

FWIW, If we adopt this, it probably would make sense to do the same for sym/dsym.

👍 totally, and no confusion possible

@marcandre I'd prefer string_type? because then it'll work in type? with the metatype groups we defined in #329, but do you have the same concerns if we use string_literal? instead of string_type??

Yes. My main concern is confusion. I mean, I don't have a strong argument to include, or to exclude xstr, and that's the main issue. I don't even know if I have a preference 😅