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

Apply frozen string literal magic comment #424

Merged

Conversation

koic
Copy link
Contributor

@koic koic commented May 28, 2024

Starting with Ruby 3.4, there is a gradual plan to freeze strings: https://bugs.ruby-lang.org/issues/20205#note-35

Using Ruby 3.4dev with code prior to this PR results in the following warning:

$ cd path/to/ruby/lrama
$ ruby -v
ruby 3.4.0dev (2024-05-27T01:45:38Z master 5853a38043) [x86_64-darwin23]
$ RUBYOPT=-W:deprecated bundle exec rake
(snip)

/Users/koic/src/github.com/ruby/lrama/lib/lrama/output.rb:74: warning: literal string will be frozen in the future

This warning suggests that applying the frozen string magic comment will lead to FrozenError. So, this indicates that the future behavior of (default frozen) string will produce FrozenError.

This PR applies the frozen string magic comment to detect FrozenError and uses String#dup to prevent it: https://gist.github.com/fxn/bf4eed2505c76f4fca03ab48c43adc72#ruby-34

This ensures compatibility for the future.

NOTE: From Ruby 3.3+, there is no speed difference between String#+@ and String#dup. For readability, String#dup has been chosen: ruby/ruby#8952

@koic koic force-pushed the apply_frozen_string_literal_magic_comment branch from f67b8d5 to 4cdb305 Compare May 28, 2024 13:50
Starting with Ruby 3.4, there is a gradual plan to freeze strings:
https://bugs.ruby-lang.org/issues/20205#note-35

Using Ruby 3.4dev with code prior to this PR results in the following warning:

```console
$ cd path/to/ruby/lrama
$ ruby -v
ruby 3.4.0dev (2024-05-27T01:45:38Z master 5853a38043) [x86_64-darwin23]
$ RUBYOPT=-W:deprecated bundle exec rake
(snip)

/Users/koic/src/github.com/ruby/lrama/lib/lrama/output.rb:74: warning: literal string will be frozen in the future
```

This warning suggests that applying the frozen string magic comment will lead to `FrozenError`.
So, this indicates that the future behavior of (default frozen) string will produce `FrozenError`.

This PR applies the frozen string magic comment to detect `FrozenError` and uses `String#dup` to prevent it:
https://gist.github.com/fxn/bf4eed2505c76f4fca03ab48c43adc72#ruby-34

This ensures compatibility for the future.

NOTE: From Ruby 3.3+, there is no speed difference between `String#+@` and `String#dup`. For readability, `String#dup` has been chosen:
ruby/ruby#8952
@koic koic force-pushed the apply_frozen_string_literal_magic_comment branch from 4cdb305 to c335174 Compare May 28, 2024 13:54
@junk0612
Copy link
Collaborator

Thank you for your contribution. I believe I understand the issue you have pointed out correctly, and it is necessary to address this to support CRuby 3.4 and later.

However, I do not think the diff you proposed is the best solution to apply across the entire project for two main reasons:

  1. There are places where there are better solutions than using String#dup. For instance, Lrama::Warning expects an object with an IO-like interface as an argument, so using StringIO.new

    let(:out) { "".dup }
    seems more appropriate than String#dup.

  2. By suppressing warnings using String#dup in such places, we might only be delaying the resolution of the issue. Fortunately, in CRuby 3.4, these places only generate warnings without causing execution issues, giving us some leeway to address them in a more appropriate manner. I believe it is premature to apply this mechanical fix at this point.

@koic
Copy link
Contributor Author

koic commented May 29, 2024

Thank you for considering this. Here are my thoughts on each point:

  1. This PR is preparing for future changes by making strings into frozen string literals. This means that they remain strings. If using something like StringIO.new is more appropriate, that suggests an issue with the current use of strings at those specific points, which can be considered independently of this PR.
  2. In Ruby 3.4, the concept of chilled strings is introduced as a transition measure. My understanding is that it is meant to prevent breaking changes to the Ruby ecosystem and to allow the entire ecosystem to prepare for frozen string literals. For example, Rails framework, and many gems are already adopting measures like using String#dup for frozen string literals.

How to proceed with this PR is up to the maintainers' discretion. Thank you.

ydah added a commit to ydah/lrama that referenced this pull request Jun 3, 2024
@ydah
Copy link
Member

ydah commented Jun 3, 2024

Thank you for making this suggestion.
It seems reasonable to address future changes. Given that procrastination requires action on more files, I think it's good to rein in the here and now.

Perhaps @junk0612 concerned about reining in with String#dup.
So I created a PR for the following Follow up PRs. Now, I think we have a root solution.

@ydah ydah requested review from junk0612 and yui-knk June 3, 2024 15:05
Copy link
Member

@ydah ydah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍🏻 @yui-knk @junk0612 WDYT?

Copy link
Collaborator

@junk0612 junk0612 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @ydah . I was reluctant to propose my opinion because of some concerns, but now that you have created a follow up, we can discuss it there, I think this PR is good to merge.

@junk0612 junk0612 merged commit 3247ef4 into ruby:master Jun 4, 2024
16 checks passed
@koic koic deleted the apply_frozen_string_literal_magic_comment branch June 4, 2024 14:38
ydah added a commit to ydah/lrama that referenced this pull request Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants