[Fix #9144] Add `aggressive` and `conservative` enforced styles for `Style/StringConcatenation` cop #9153

tejasbubane · 2020-12-02T20:33:22Z

Before submitting the PR make sure the following are checked:

The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
Wrote good commit messages.
Commit message starts with [Fix #issue-number] (if the related issue exists).
Feature branch is up-to-date with master (if not - rebase it).
Squashed related commits together.
Added tests.
Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

ianfixes · 2020-12-03T04:16:08Z

This is a small step forward, but I want to be clear that my aim in opening #9144 was simply to show that the + operator could not be trusted to indicate a concatenation operation. What's being addressed in this PR is only my very contrived counterexample.

A more common counterexample would be in a form like this:

def for_example(a_pathname)
    a_pathname + "a_string"
end

RuboCop really has no business pretending that string concatenation is the only thing that + "a_string" could be accomplishing.

bbatsov · 2020-12-03T06:11:02Z

I also don't like the proposed change as I don't think the problem is solvable by ignoring some cases. Fundamentally, the problem is that with operator overloading we can never know what happens in certain cases and we can only guess. The cop is already marked as unsafe because of this. The only thing I can think of is to add a mode that ignores cases where the first operand is not a string literal.

tejasbubane · 2020-12-03T08:13:38Z

This is a small step forward, but I want to be clear that my aim in opening #9144 was simply to show that the + operator could not be trusted to indicate a concatenation operation. What's being addressed in this PR is only my very contrived counterexample.

Yes I had the same thing in mind, but thought Pathname being stdlib made sense to add an exception.

The only thing I can think of is to add a mode that ignores cases where the first operand is not a string literal.

That sounds a lot better. I will change this PR accordingly.

ianfixes · 2020-12-03T14:34:06Z

The only thing I can think of is to add a mode that ignores cases where the first operand is not a string literal.

Cross posting my latest comment from #9144, I think this could be expanded to say "only consider cases where the first and last operands in a chain of + operators are string lterals"

def some_fn(var_of_unknown_type)
  puts "hello" + "world"                       # string concatenation 
  puts var_of_unknown_type + "world"           # insufficient info to decide
  puts "hello" + var_of_unknown_type           # string concatenation (I think? Is there a case where that's not true?)
  puts "hello" + var_of_unknown_type + "world" # string concatenation 
end

tejasbubane · 2020-12-03T19:11:10Z

@bbatsov Changed this PR to use a config option.

lib/rubocop/cop/style/string_concatenation.rb

bbatsov · 2020-12-05T06:34:08Z

I'm also thinking we can have some more generic names for the enforced style (e.g. conservative and aggressive) or perhaps it'd be even better if this was some boolean flag like SafeMode. We're not really supporting different styles here, we're rather limitting the scope of the cop.

bbatsov · 2021-01-10T08:00:24Z

Ping :-)

tejasbubane · 2021-01-28T12:04:38Z

I forgot about this PR. I will fix this in a day or two.

bbatsov · 2021-02-15T06:27:33Z

@tejasbubane another friendly reminder about this PR :-)

tejasbubane · 2021-02-21T17:23:26Z

@bbatsov Changes done. Sorry for the delay.

bbatsov · 2021-03-17T07:33:25Z

lib/rubocop/cop/style/string_concatenation.rb

@@ -15,7 +15,13 @@ module Style
      # lines, this cop does not register an offense; instead,
      # `Style/LineEndConcatenation` will pick up the offense if enabled.
      #
-      # @example
+      # Two modes are supported:


You should also mention how this ties to the cop safety - e.g. with the Pathname example.

bbatsov · 2021-03-17T07:35:57Z

Seems I totally forgot about this myself. After some further thought let's not use EnforcedStyle, but rather some other configuration option name (e.g. Mode/Detection), as that's not really a different style just an instruction how hard to try to find offenses. If someone from @rubocop/rubocop-core can suggest a better config setup - be my guest.

bbatsov

Overall things look good. We just need to add some safety documentation and adopt some dedicated configuration option for the two modes of operation.

lib/rubocop/cop/style/string_concatenation.rb

bquorning · 2021-03-19T09:01:11Z

spec/rubocop/cop/style/string_concatenation_spec.rb

+      it 'does not register offense' do
+        expect_no_offenses(<<~RUBY)
+          user.name + "!!"
+          user.name + "<" + "user.email" + ">"


Actually, I would expect an offense here. Not on user.name + "<", but on "<" + "user.email" (and "user.email" + ">").

Good catch!

I disagree.

# yes, I know that a variable called user.name is unlikely to be a Pathname user.name = Pathname.new("user.name") result = user.name + "<" + "user.email" + ">" puts "result is #{result}" # result is user.name/</user.email/> # which is in no way equivalent to "user.name<user.email>"

@tejasbubane -- I think the fix that was called for here was in error. See my above comment

tejasbubane · 2021-04-20T19:21:03Z

Review comments fixed.

Changed config name to Mode.
Fixed the issue @bquorning pointed out.
Added Pathname example to cop documentation to show how the config ties to cop safety.

ianfixes · 2021-05-17T13:27:18Z

lib/rubocop/cop/style/string_concatenation.rb

+      # either the left or right side of `+` is a string literal.
+      # 2. `conservative` style on the other hand, checks and corrects only if
+      # left side (receiver of `+` method call) is a string literal.
+      # This is useful when the receiver is some expression that returns string like `Pathname`


Pathname.new returns a Pathname, not a string. Pathname.new("x") + "y" also returns a Pathname.

I think this comment should read

This is useful when the receiver is an object whose + operator accepts a string, but does not return a string -- such as Pathname

bbatsov · 2021-05-19T05:30:43Z

changelog/change_string_concatenation_enforced_styles.md

@@ -0,0 +1 @@
+* [#9144](https://github.com/rubocop/rubocop/issues/9144): Add `aggressive` and `conservative` enforced styles for `Style/StringConcatenation` cop. ([@tejasbubane][])


Those are not enforced styles, but "modes of operation".

bbatsov · 2021-05-19T05:31:13Z

spec/rubocop/cop/style/string_concatenation_spec.rb

@@ -203,4 +203,36 @@
      RUBY
    end
  end
+
+  context 'EnforcedStyle = conservative' do


bbatsov · 2021-06-10T06:01:55Z

@tejasbubane ping :-)

…tyle/StringConcatenation` cop Closes rubocop#9144

tejasbubane · 2021-06-27T11:55:46Z

@bbatsov Minor wording changes done.

Fixes rubocop#10685. This PR fixes a false positive for `Style/StringConcatenation` when `Mode: conservative` and first operand is not string literal. This also resolves the following feedback: rubocop#9153 (comment) `Mode: conservative` should be allowed considering the possibility that receiver is a `Pathname` object.

tejasbubane force-pushed the fix-9144 branch from e4f86b8 to 8e245c7 Compare December 3, 2020 18:59

tejasbubane changed the title ~~[Fix #9144] Add IgnoredPatterns config to Style/StringConcatenation~~ [Fix #9144] Add literal_first_operand enforced-style for Style/StringConcatenation cop Dec 3, 2020

tejasbubane force-pushed the fix-9144 branch from 8e245c7 to 27beed5 Compare December 3, 2020 19:03

bbatsov reviewed Dec 4, 2020

View reviewed changes

lib/rubocop/cop/style/string_concatenation.rb Outdated Show resolved Hide resolved

bbatsov self-assigned this Feb 15, 2021

tejasbubane force-pushed the fix-9144 branch from 27beed5 to 210b091 Compare February 21, 2021 17:22

tejasbubane force-pushed the fix-9144 branch from 210b091 to 0895a25 Compare February 21, 2021 17:25

tejasbubane requested a review from bbatsov February 21, 2021 20:02

bbatsov reviewed Mar 17, 2021

View reviewed changes

bbatsov requested changes Mar 17, 2021

View reviewed changes

bquorning reviewed Mar 19, 2021

View reviewed changes

tejasbubane force-pushed the fix-9144 branch 2 times, most recently from 3871cec to e381cbf Compare April 20, 2021 19:09

tejasbubane changed the title ~~[Fix #9144] Add literal_first_operand enforced-style for Style/StringConcatenation cop~~ [Fix #9144] Add aggressive and conservative enforced styles for Style/StringConcatenation cop Apr 20, 2021

tejasbubane force-pushed the fix-9144 branch 2 times, most recently from cc708b1 to 7932e00 Compare April 20, 2021 19:15

tejasbubane requested review from bbatsov and bquorning April 20, 2021 19:15

tejasbubane force-pushed the fix-9144 branch from 7932e00 to 748962f Compare April 20, 2021 19:19

tejasbubane force-pushed the fix-9144 branch from 748962f to f64e109 Compare May 16, 2021 14:00

ianfixes reviewed May 17, 2021

View reviewed changes

bbatsov assigned dvandersluis and unassigned bbatsov May 19, 2021

bbatsov reviewed May 19, 2021

View reviewed changes

bbatsov requested a review from dvandersluis May 19, 2021 05:31

[Fixes rubocop#9144] Add aggressive and conservative modes for `S…

e99d392

…tyle/StringConcatenation` cop Closes rubocop#9144

tejasbubane force-pushed the fix-9144 branch from f64e109 to e99d392 Compare June 27, 2021 11:39

tejasbubane requested a review from bbatsov June 27, 2021 11:42

bbatsov approved these changes Jun 28, 2021

View reviewed changes

bbatsov merged commit 9b5402d into rubocop:master Jun 28, 2021

tejasbubane deleted the fix-9144 branch June 28, 2021 08:51

koic mentioned this pull request Jun 2, 2022

[Fix #10685] Fix a false positive for Style/StringConcatenation #10688

Merged

8 tasks

koic mentioned this pull request Apr 4, 2023

Maybe Style/StringConcatenation should be conservative by default #11757

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix #9144] Add `aggressive` and `conservative` enforced styles for `Style/StringConcatenation` cop #9153

[Fix #9144] Add `aggressive` and `conservative` enforced styles for `Style/StringConcatenation` cop #9153

tejasbubane commented Dec 2, 2020 •

edited

ianfixes commented Dec 3, 2020

bbatsov commented Dec 3, 2020

tejasbubane commented Dec 3, 2020

ianfixes commented Dec 3, 2020

tejasbubane commented Dec 3, 2020 •

edited

bbatsov commented Dec 5, 2020

bbatsov commented Jan 10, 2021

tejasbubane commented Jan 28, 2021

bbatsov commented Feb 15, 2021

tejasbubane commented Feb 21, 2021

bbatsov Mar 17, 2021

bbatsov commented Mar 17, 2021

bbatsov left a comment

bquorning Mar 19, 2021

bbatsov Mar 19, 2021

ianfixes Apr 20, 2021

ianfixes Apr 20, 2021

tejasbubane commented Apr 20, 2021

ianfixes May 17, 2021

bbatsov May 19, 2021

bbatsov May 19, 2021

bbatsov commented Jun 10, 2021

tejasbubane commented Jun 27, 2021

		@@ -0,0 +1 @@
		* [#9144](https://github.com/rubocop/rubocop/issues/9144): Add `aggressive` and `conservative` enforced styles for `Style/StringConcatenation` cop. ([@tejasbubane][])

[Fix #9144] Add aggressive and conservative enforced styles for Style/StringConcatenation cop #9153

[Fix #9144] Add aggressive and conservative enforced styles for Style/StringConcatenation cop #9153

Conversation

tejasbubane commented Dec 2, 2020 • edited

ianfixes commented Dec 3, 2020

bbatsov commented Dec 3, 2020

tejasbubane commented Dec 3, 2020

ianfixes commented Dec 3, 2020

tejasbubane commented Dec 3, 2020 • edited

bbatsov commented Dec 5, 2020

bbatsov commented Jan 10, 2021

tejasbubane commented Jan 28, 2021

bbatsov commented Feb 15, 2021

tejasbubane commented Feb 21, 2021

Choose a reason for hiding this comment

bbatsov commented Mar 17, 2021

bbatsov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tejasbubane commented Apr 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Jun 10, 2021

tejasbubane commented Jun 27, 2021

[Fix #9144] Add `aggressive` and `conservative` enforced styles for `Style/StringConcatenation` cop #9153

[Fix #9144] Add `aggressive` and `conservative` enforced styles for `Style/StringConcatenation` cop #9153

tejasbubane commented Dec 2, 2020 •

edited

tejasbubane commented Dec 3, 2020 •

edited