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

Address warning: literal string will be frozen in the future warnings #462

Conversation

yahonda
Copy link
Contributor

@yahonda yahonda commented Oct 8, 2024

This commit addresses the warning: literal string will be frozen in the future warnings appeared at Rails CI https://buildkite.com/rails/rails-nightly/builds/1122#019263a3-2200-4e62-a6a8-028cffa60aaf

Here are the warnings appeared:

/usr/local/lib/ruby/gems/3.4.0+0/bundler/gems/httpclient-d57cc6d5ffee/lib/httpclient.rb:1256: warning: literal string will be frozen in the future
/usr/local/lib/ruby/gems/3.4.0+0/bundler/gems/httpclient-d57cc6d5ffee/lib/httpclient/http.rb:580: warning: literal string will be frozen in the future
/usr/local/lib/ruby/gems/3.4.0+0/bundler/gems/httpclient-d57cc6d5ffee/lib/httpclient/session.rb:954: warning: literal string will be frozen in the future
/usr/local/lib/ruby/gems/3.4.0+0/bundler/gems/httpclient-d57cc6d5ffee/lib/httpclient/util.rb:71: warning: literal string will be frozen in the future

There should be some warnings remained because Rails CI does not use all of httpclient code. I'll open some pull requests later to address remaining ones.

Ref: https://bugs.ruby-lang.org/issues/20205

@hartator
Copy link

hartator commented Oct 8, 2024

I think we should reasonably push back on these new warnings and the restriction itself.

I don't think they realized how big of an impact it will be while negatively impacting performance at least in the short term.

I have added a note: https://bugs.ruby-lang.org/issues/20205#note-55

@hsbt
Copy link

hsbt commented Oct 9, 2024

I also found warnings by stdlib with @yahonda:

/Users/hsbt/.local/share/rbenv/versions/ruby-dev/lib/ruby/3.4.0+0/open-uri.rb:455: warning: literal string will be frozen in the future
/Users/hsbt/.local/share/rbenv/versions/ruby-dev/lib/ruby/3.4.0+0/logger/log_device.rb:45: warning: literal string will be frozen in the future

@mame found open-uri warning caused by

def set_body_encoding
  if type = self.content_type
    OpenURI::Meta.init(o = '')
    o.meta_add_field('content-type', type)
    @body_encoding = o.encoding
  end
end

from https://github.com/nahi/httpclient/blob/master/lib/httpclient/http.rb#L241

It's hard to fix that from open-uri warnings.

@byroot
Copy link
Contributor

byroot commented Oct 9, 2024

The warning isn't really from open-uri but from passing a literal to a method that mutate the argument.

The fix is to do: OpenURI::Meta.init(o = ''.dup)

@@ -1240,7 +1240,7 @@ def do_get_block(req, proxy, conn, &block)
conn.push(res)
return res
end
content = block ? nil : ''
content = block ? nil : String.new('')
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
content = block ? nil : String.new('')
content = block ? nil : ''.dup

Why not a simple .dup?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since these strings starts out as an empty string and the string is added later with the << method, I was feeling like it seemed more appropriate to create empty string via String.new than duplicating an empty string. However, I do not have strong opinion on this so let me update them to use dup.

@byroot
Copy link
Contributor

byroot commented Oct 9, 2024

An easy way to solve these warnings is to run ruby with --enable-frozen-string-literal --debug-frozen-string-literal, this will include the location where the string was allocated in the FrozenError message.

@yahonda yahonda force-pushed the address_literal_string_will_be_frozen_warnings_at_rails_ci branch from a049d6f to 409fa27 Compare October 11, 2024 03:24
Copy link

@jeremy jeremy left a comment

Choose a reason for hiding this comment

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

Looks like HTTP::Message::Body#dump and #dump_chunked need to be fixed as well. Both have frozen empty strings as default args.

@jeremy
Copy link

jeremy commented Dec 17, 2024

Covered other cases in #465

@yahonda yahonda force-pushed the address_literal_string_will_be_frozen_warnings_at_rails_ci branch from 409fa27 to d12178d Compare February 18, 2025 04:58
@yahonda
Copy link
Contributor Author

yahonda commented Feb 18, 2025

Rebased master branch to make CI run.

This commit addresses the `warning: literal string will be frozen in the future` warnings
appeared at Rails CI https://buildkite.com/rails/rails-nightly/builds/1122#019263a3-2200-4e62-a6a8-028cffa60aaf

Here are the warnings appeared:
```
/usr/local/lib/ruby/gems/3.4.0+0/bundler/gems/httpclient-d57cc6d5ffee/lib/httpclient.rb:1256: warning: literal string will be frozen in the future
/usr/local/lib/ruby/gems/3.4.0+0/bundler/gems/httpclient-d57cc6d5ffee/lib/httpclient/http.rb:580: warning: literal string will be frozen in the future
/usr/local/lib/ruby/gems/3.4.0+0/bundler/gems/httpclient-d57cc6d5ffee/lib/httpclient/session.rb:954: warning: literal string will be frozen in the future
/usr/local/lib/ruby/gems/3.4.0+0/bundler/gems/httpclient-d57cc6d5ffee/lib/httpclient/util.rb:71: warning: literal string will be frozen in the future
```

There should be some warnings remained because Rails CI does not use all of httpclient code. I'll open some pull requests later to address remaining ones.

Ref: https://bugs.ruby-lang.org/issues/20205
@yahonda yahonda force-pushed the address_literal_string_will_be_frozen_warnings_at_rails_ci branch from d12178d to ab12aba Compare February 18, 2025 05:01
@byroot
Copy link
Contributor

byroot commented Feb 18, 2025

I recommend you add one Ruby with --enable-frozen-string-literal --debug-frozen-string-literal in the CI Matrix.

Also I silenced those warnings when I revamped CI, it should now be removed.

@takkanm takkanm merged commit ab12aba into nahi:master Feb 18, 2025
9 checks passed
@wyardley
Copy link

Will a release be getting cut with these changes soon?

@takkanm
Copy link
Collaborator

takkanm commented Feb 20, 2025

We are currently planning a new release. Please wait a little longer.

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.

7 participants