Style/StringConcatenation mistakenly assumes that the + operator means concatenation, e.g. with Pathname

[ianfixes]

Expected behavior

Rubocop should not suggest replacing an expression that returns a Pathname with an expression that returns a String, even if we use the + operator to combine a Pathname and String (which returns a Pathname)

Actual behavior

1.5.0 (using Parser 2.7.2.0, rubocop-ast 1.3.0, running on ruby 2.6.6 x86_64-linux)
Inspecting 1 file
C

Offenses:

bad.rb:4:7: C: Style/StringConcatenation: Prefer string interpolation to string concatenation.
puts (Pathname.new('/') + 'test').class
      ^^^^^^^^^^^^^^^^^^^^^^^^^^

Steps to reproduce the problem

Here are the files I used to produce the error:

Gemfile

source 'https://rubygems.org'
gem 'rubocop', '=1.5.0'

bad.rb

# frozen_string_literal: true

require 'pathname'
puts (Pathname.new('/') + 'test').class

Note that running the bad.rb will print Pathname

Faster way to reproduce the issue

FROM ruby:2.6-slim
ARG TESTFILE=bad.rb

RUN true \
  && echo "source 'https://rubygems.org'" > Gemfile \
  && echo "gem 'rubocop', '=1.5.0'" >> Gemfile \
  && cat Gemfile \
  && bundle install

RUN true \
  && echo "# frozen_string_literal: true\n" > $TESTFILE \
  && echo "require 'pathname'" >> $TESTFILE \
  && echo "puts (Pathname.new('/') + 'test').class" >> $TESTFILE \
  && cat $TESTFILE

RUN ruby $TESTFILE
RUN bundle exec rubocop -V && bundle exec rubocop --debug -D $TESTFILE

Saving this as Dockerfile and running docker build . should show the entire process

[marcandre]

The problem is larger than this though.

path = Pathname.new('/')
puts (path + 'xyz')

The cop is already marked as unsafe, I'm not sure there's much more we can do (until we can know more about the class of some expressions)

[ianfixes]

The cop is already marked as unsafe

This is good to know, but (forgive my ignorance) how do I know which cops are unsafe? I upgraded from a version 0.55.0 (or so) of rubocop, and the new version had this rule automatically enabled, it seems. Is there something in my existing config file that might have caused an unsafe cop to run?

[bbatsov]

@ianfixes It's marked as Unsafe in the cop's yaml metadata. You also see this in the cop's documentation. https://docs.rubocop.org/rubocop/1.5/cops_style.html#stylestringconcatenation

Unsafe cops are enabled by default, as they are pretty useful in most cases, but they are excluded from the default auto-correct.

Btw, after pondering about this cop we can add some option to ignore the unsafe cases - meaning we'll need the first arg to be a string literal. Not sure if this is worth investing time into, but I think it's the only meaningful improvement that can be done.

[ianfixes]

Having an option to reduce the detection for this cop to just "string literal" [+ 0 or more other expressions] + "string literal" sounds like a great improvement, honestly.

I'm not sure what the sentiment is around creating these cops but it sounds like there is a tradeoff between confidence and scope. In this case, this cop's rule is broad in scope but not high in confidence (well, my confidence anyway). That means I'll be disabling it for now, and it will probably stay disabled forever as I'm unlikely to re-evaluate all my disabled cops next time I upgrade Rubocop. I'd rather have a cop that starts with a very narrow scope but 100% confidence, and widens the scope over time -- that way I can keep the rule enabled in my config and just fix the new violations as they are detected.

What kills my productivity is having to go violation by violation and say to myself "hmm, is this really a violation?"; it's instant decision fatigue.

[marcandre]

@ianfixes instead of disabling just that cop, consider running rubocop --safe or modify your .rubocop.yml instead (sorry, not sure how to get the same effect off hand).

[ianfixes]

I'm not sure where to say this (as I'm sure it's been discussed at length elsewhere) but it's disheartening to hear that rubocop --safe isn't the default behavior, i.e. that potential false positives come enabled by default (instead of being something you look for after everything else has been cleaned up).

No action required, I just needed to express that. I will plan to use rubocop --safe exclusively in the future.

[Drenmi]

I'd rather have a cop that starts with a very narrow scope but 100% confidence, and widens the scope over time -- that way I can keep the rule enabled in my config and just fix the new violations as they are detected.

There are plenty of options for marking false positives, and more are added by the day. False negatives, on the other hand, are beyond reach of the end user, and so the trade-off is (generally) to prefer false positives.

What kills my productivity is having to go violation by violation and say to myself "hmm, is this really a violation?"; it's instant decision fatigue.

While RuboCop can save a great deal of time, it will never be "smart" in the human sense, and thus can not save us from thinking about our code. Thinking is hard work and all that, but unfortunately (or maybe fortunately for us who do it for a living 🙂) we need to do it ourselves.

it's disheartening to hear that rubocop --safe isn't the default behavior, i.e. that potential false positives come enabled by default

I think the most common use case for RuboCop is to help maintain a consistent style throughout a large code base. If applied only partially, then the result would be something that isn't "consistent" by any appreciable definition of the word.

It sounds like maybe you are trying to apply RuboCop retroactively to an old project. There are some good options for making that process more manageable, see e.g. the TODO feature, which lets you assess and phase in cops one by one.

[ianfixes]

It sounds like maybe you are trying to apply RuboCop retroactively to an old project

Not quite, I had rubocop enabled from the beginning of this project but recently upgraded its version. I make heavy use of rubocop in both personal and professional projects, and my positive experience there is driven by high confidence: each cop is (until now) so perfectly consistent, so the cop as a whole either does or doesn't match my desired style (in which case, I turn it off).

What jumped out at me in this case was an instance of a cop being sometimes useful. That's what I'm reacting to here. I got through fixing a few actual problems flagged by the cop, then went through 20 or so that were invalid (for the reason posted in the issue description). And because both valid and invalid errors came up within the same file, I can't "phase in" the cop in any reasonable way.

[Rubocop] will never be "smart" in the human sense

If you're hearing that in what I'm saying, I apologize. I'm not expecting it to be "smart". If anything, I'm asking for it to be less smart by not trying to flag cases for which there is insufficient context to judge.