Style/RedundantInterpolation auto-correct should not be marked safe or should be implemented differently
#10656
Labels
Comments
jimryan
changed the title
Style/RedundantInterpolation auto-correct should not be marked safe or should be implemented differently
koic
added a commit
to koic/rubocop
that referenced
this issue
May 23, 2022
…ocorrection Fixes rubocop#10656. This PR marks `Style/RedundantInterpolation` as unsafe autocorrection because when calling a destructive method to string, the resulting string may have different behavior or raise `FrozenError`. ```ruby x = 'a' y = "#{x}" y << 'b' # return 'ab' x # return 'a' y = x.to_s y << 'b' # return 'ab' x # return 'ab' x = 'a'.freeze y = "#{x}" y << 'b' # return 'ab'. y = x.to_s y << 'b' # raise `FrozenError`. ``` Updating the auto-corrected code from `'a'.to_s` to `'a'.to_s.dup` will get safe, but it will make `dup` redundant when unneeded. And This would go against the cop's name "redundant". That means replacing it with a different redundancy that cannot be removed by static analysis. So I decided to mark it as unsafe.
8 tasks
bbatsov
pushed a commit
that referenced
this issue
May 23, 2022
…tion Fixes #10656. This PR marks `Style/RedundantInterpolation` as unsafe autocorrection because when calling a destructive method to string, the resulting string may have different behavior or raise `FrozenError`. ```ruby x = 'a' y = "#{x}" y << 'b' # return 'ab' x # return 'a' y = x.to_s y << 'b' # return 'ab' x # return 'ab' x = 'a'.freeze y = "#{x}" y << 'b' # return 'ab'. y = x.to_s y << 'b' # raise `FrozenError`. ``` Updating the auto-corrected code from `'a'.to_s` to `'a'.to_s.dup` will get safe, but it will make `dup` redundant when unneeded. And This would go against the cop's name "redundant". That means replacing it with a different redundancy that cannot be removed by static analysis. So I decided to mark it as unsafe.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The
Style/RedundantInterpolationcop's auto-correct should not be marked safe because it cannot guarantee that subsequent code wasn't relying on the "redundant" interpolation essentially dup'ing the string.I've put together a brief demo of two scenarios where replacing the interpolation with
to_sfails to produce equivalent code: https://github.com/jimryan/rubocop-redundant-interpolation-unsafe-autocorrect-demoAn alternative solution to not marking as safe would be to replace redundant string interpolations with
.to_s.dupwhich is more precisely what the redundant interpolation was doing. E.g."#{foo}"becomesfoo.to_s.dup. I believe that would be safe.Expected behavior
Autocorrection should not be marked "safe" for this cop. I would not expect the potential for an error or change in behavior of any kind after running a safe auto-correction.
Alternative behavior could be to replace redundant string interpolations with
to_s.dup.Actual behavior
It is marked safe, yet safe autocorrection can break/change code in some scenarios.
Steps to reproduce the problem
Steps are in the
READMEof the demo repo, but in general:Style/RedundantInterpolationwill replace that interpolation with.to_son the original string, which just returns the same String object. Now the mutation is mutating the original string (the one that was interpolated), and not a copy of it. When a string is the subject of the redundant interpolation, the interpolation is equivalent todup, notto_s.RuboCop version
The text was updated successfully, but these errors were encountered: