Skip to content

Commit

Permalink
Dup possibly frozen string in to_string_literal helper
Browse files Browse the repository at this point in the history
Currently, there are exactly three instances where the receiver-mutating
`String#force_encoding` method is used. In all cases except for
`to_string_literal`, a `String#dup` call is included to ensure compatibility
with frozen strings. I couldn't identify any counterexamples in rubocop core that would result
in a runtime error, but there is at least one instance in `rubocop-performance` [1]
where a frozen string might be passed. To ensure everything works correctly,
we need to add a `#dup` call in the implementation of `to_string_literal`.

For additional context, it's worth noting that the Prism parser might start
freezing AST leaf nodes in the future [2],
and whitequark's parser has some unpredictability in this regard [3].

[1] rubocop/rubocop-performance#480
[2] ruby/prism#3309
[3] rubocop/rubocop-ast#342
  • Loading branch information
viralpraxis committed Dec 20, 2024
1 parent f9e9c01 commit f9aaaf6
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 2 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#13605](/~https://github.com/rubocop/rubocop/pull/13605): Fix `RuboCop::Cop::Util.to_string_literal` to work correctly with frozen strings. ([@viralpraxis][])
3 changes: 1 addition & 2 deletions lib/rubocop/cop/util.rb
Original file line number Diff line number Diff line change
Expand Up @@ -204,8 +204,7 @@ def parse_regexp(text)
private

def compatible_external_encoding_for?(src)
src = src.dup if RUBY_ENGINE == 'jruby'
src.force_encoding(Encoding.default_external).valid_encoding?
src.dup.force_encoding(Encoding.default_external).valid_encoding?
end

def include_or_equal?(source, target)
Expand Down
10 changes: 10 additions & 0 deletions spec/rubocop/cop/util_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -86,4 +86,14 @@ def some_method
expect(described_class.parse_regexp('+')).to be_nil
end
end

describe '#to_string_literal' do
it 'returns literal for normal string' do
expect(TestUtil.new.send(:to_string_literal, 'foo')).to eq("'foo'")
end

it 'returns literal for string which requires escaping' do
expect(TestUtil.new.send(:to_string_literal, 'foo\'')).to eq('"foo\'"')
end
end
end

0 comments on commit f9aaaf6

Please sign in to comment.