Fix Performance/Squeeze cop error on frozen AST string node value
#480
Conversation
|
Yes, I think this should instead be handled in rubocop itself instead of potentially having to handle this at every callsite. https://github.com/rubocop/rubocop/blob/9f2ffa2270bbc174b2a622350d120b05672a1ce5/lib/rubocop/cop/util.rb#L198-L201. Other |
Shouldn't it still be also fixed here since latest |
|
I guess, but for this I would just be tempted to force those select few users that run into this to upgrade instead. It is so very specific and doesn't look like something that someone would actually write (?). Not my decision though. |
That's actually the code I wrote right after learning about "character literals" 😄 Anyway, I'll open a PR at rubocop's side to address this issue |
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
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
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
| @@ -46,7 +46,7 @@ def on_send(node) | |||
| message = format(MSG, current: bad_method, prefer: good_method) | |||
|
|
|||
| add_offense(node.loc.selector, message: message) do |corrector| | |||
| string_literal = to_string_literal(replace_str) | |||
| string_literal = to_string_literal(replace_str.dup) | |||
There was a problem hiding this comment.
Can you add a FIXME comment? e.g.:
| string_literal = to_string_literal(replace_str.dup) | |
| # FIXME: When requiring only RuboCop 1.70.0 and above, | |
| # `dup` in `replace_str.dup` becomes unnecessary, so please remove it. | |
| string_literal = to_string_literal(replace_str.dup) |
Since sometimes AST string nodes might have their values frozen, we should call `dup` explicitly to make sure `to_string_literal` (which does not work with frozen strings) does not raise. Maybe we should also apply a patch on RuboCop's core side to fix the `to_string_literal` itself (it now uses `force_encoding` on its argument). See [1] and [2] for details. [1] ruby/prism#3309 [2] rubocop/rubocop-ast#342
viralpraxis
force-pushed
the
fix-performance-squeeze-cop-error-on-frozen-ast-literal-node-value
branch
from
b18dd80 to
9fdbe4e
Compare
|
Thanks! |
Since sometimes AST string nodes might have their values frozen, we should call
dupexplicitly to make sureto_string_literal(which does not work with frozen strings) does not raise. Maybe we should also apply a patch on RuboCop's core side to fix theto_string_literalitself (it now usesforce_encodingon its argument).See [1] and [2] for details.
[1] ruby/prism#3309
[2] rubocop/rubocop-ast#342
Before submitting the PR make sure the following are checked:
[Fix #issue-number](if the related issue exists).master(if not - rebase it).bundle exec rake default. It executes all tests and runs RuboCop on its own code.{change_type}_{change_description}.mdif the new code introduces user-observable changes. See changelog entry format for details.