Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Autocorrection of Lint/LiteralInInterpolation changes code behaviour in regexes #6822

Closed
patrick-gleeson opened this issue Mar 7, 2019 · 5 comments · Fixed by #6979
Closed
Labels

Comments

@patrick-gleeson
Copy link

patrick-gleeson commented Mar 7, 2019

Rubocop generally auto-replaces "#{'a'}" with "a" as a response to the "Lint/LiteralInInterpolation: Literal interpolation detected." offence. However, when it comes to regex capture group literals, this changes the behaviour of the code, making this auto-correction unsafe


Expected behavior

When running rubocop -a on this line of code:

x = "ABC".gsub(/(A)(B)(C)/, "D#{'\2'}F")

The resulting code should look like:

x = "ABC".gsub(/(A)(B)(C)/, "D\\2F")

In both cases, the value of x is "DBF"

Actual behavior

When running rubocop -a on this line of code:

x = "ABC".gsub(/(A)(B)(C)/, "D#{'\2'}F")

The resulting code actually looks like:

x = "ABC".gsub(/(A)(B)(C)/, "D\2F")

The value of x is now "D\u0002F"

Steps to reproduce the problem

Put this in a file:

"ABC".gsub(/(A)(B)(C)/, "D#{'\2'}F")

run rubocop -a /path/to/file.rb

RuboCop version

$ [bundle exec] rubocop -V
0.65.0 (using Parser 2.6.0.0, running on ruby 2.5.3 x86_64-darwin17)
@koic koic added the bug label Mar 7, 2019
@rrosenblum
Copy link
Contributor

Does anyone have thoughts on the best way to fix this? Ignore the offenses if they are an argument to sub, gsub, and similar methods? Do not register an offense if the string contains \<number>?

@hoshinotsuyoshi
Copy link
Contributor

@rrosenblum

I think that there is no need to treat #gsub, any methods, or \<number> specially.

The problem is that auto-correcting results in wrong value replacing; like "\\2" -> "\2".


I think that RuboCop::Cop::Util#escape_string will fit to fix that...

--- a/lib/rubocop/cop/lint/literal_in_interpolation.rb
+++ b/lib/rubocop/cop/lint/literal_in_interpolation.rb
@@ -55,7 +55,7 @@ module RuboCop
           when :float
             node.children.last.to_f.to_s
           when :str
-            node.children.last
+            escape_string(node.children.last)
           when :sym
             autocorrected_value_for_symbol(node)
           else

I will investigate later.

Escaping is difficult...

@hoshinotsuyoshi
Copy link
Contributor

I think that RuboCop::Cop::Util#escape_string will fit to fix that...

Sorry, I found that it breaks other behaviors. 😭

Forget that...

memo:
"foo#{"\n"}bar"

now latest rubocop (v0.67.2) auto-corrects to that

"foo
bar"

but using RuboCop::Cop::Util#escape_string breaks this:

"foo\nbar"

@hoshinotsuyoshi
Copy link
Contributor

hoshinotsuyoshi commented Apr 26, 2019

But I believe that the current behavior below is simply a bug.

# a.rb
"foo#{'\n'}bar"

rubocop (v0.67.2) corrects

rubocop -a a.rb --only Lint/LiteralInInterpolation
Inspecting 1 file
W

Offenses:

a.rb:3:7: W: [Corrected] Lint/LiteralInInterpolation: Literal interpolation detected.
"foo#{'\n'}bar"
      ^^^^

1 file inspected, 1 offense detected, 1 offense corrected

results in:

# a.rb
"foo\nbar"

I want to fix this problem.

hoshinotsuyoshi added a commit to hoshinotsuyoshi/rubocop that referenced this issue Apr 27, 2019
… single quotes

  ## Current behavior:

  Doing `$ rubocop --only Lint/LiteralInInterpolation`;

  source:

  ```ruby
  x = "ABC".gsub(/(A)(B)(C)/, "D#{'\2'}F")
  "foo#{'\n'}bar"
  "this is #{'"'} silly"
  ```

  corrected to:

  ```ruby
  x = "ABC".gsub(/(A)(B)(C)/, "D\2F")
  "foo\nbar"
  "this is " silly"
  ```

  ## New behavior:

  corrected to:

  ```ruby
  x = "ABC".gsub(/(A)(B)(C)/, "D\\2F")
  "foo\\nbar"
  "this is \" silly"
  ```
bbatsov pushed a commit that referenced this issue Apr 29, 2019
… quotes (#6979)

## Current behavior:

  Doing `$ rubocop --only Lint/LiteralInInterpolation`;

  source:

  ```ruby
  x = "ABC".gsub(/(A)(B)(C)/, "D#{'\2'}F")
  "foo#{'\n'}bar"
  "this is #{'"'} silly"
  ```

  corrected to:

  ```ruby
  x = "ABC".gsub(/(A)(B)(C)/, "D\2F")
  "foo\nbar"
  "this is " silly"
  ```

  ## New behavior:

  corrected to:

  ```ruby
  x = "ABC".gsub(/(A)(B)(C)/, "D\\2F")
  "foo\\nbar"
  "this is \" silly"
  ```
@patrick-gleeson
Copy link
Author

@hoshinotsuyoshi you are a true pillar of the community. Thank you for fixing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants