Skip to content

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

Merged
takkanm merged 1 commit into
nahi:masterfrom
yahonda:address_literal_string_will_be_frozen_warnings_at_rails_ci
Feb 18, 2025
Merged

Address warning: literal string will be frozen in the future warnings#462
takkanm merged 1 commit into
nahi:masterfrom
yahonda:address_literal_string_will_be_frozen_warnings_at_rails_ci

Conversation

@yahonda

@yahonda yahonda commented Oct 8, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

hartator commented Oct 8, 2024

Copy link
Copy Markdown

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

hsbt commented Oct 9, 2024

Copy link
Copy Markdown

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

byroot commented Oct 9, 2024

Copy link
Copy Markdown
Contributor

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)

byroot
byroot reviewed Oct 9, 2024
View reviewed changes
Comment thread lib/httpclient.rb Outdated
return res
end
content = block ? nil : ''
content = block ? nil : String.new('')

@byroot byroot Oct 9, 2024

Copy link
Copy Markdown
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?

@yahonda yahonda Oct 11, 2024

Copy link
Copy Markdown
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

byroot commented Oct 9, 2024

Copy link
Copy Markdown
Contributor

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
jeremy
jeremy reviewed Dec 17, 2024
View reviewed changes

@jeremy jeremy left a comment

Copy link
Copy Markdown

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 jeremy mentioned this pull request Dec 17, 2024
Closed
@jeremy

jeremy commented Dec 17, 2024

Copy link
Copy Markdown

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

yahonda commented Feb 18, 2025

Copy link
Copy Markdown
Contributor Author

Rebased master branch to make CI run.

@yahonda
Address warning: literal string will be frozen in the future warnings
ab12aba 
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

byroot commented Feb 18, 2025

Copy link
Copy Markdown
Contributor

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.

@byroot byroot mentioned this pull request Feb 18, 2025
Merged
@takkanm
takkanm merged commit ab12aba into nahi:master Feb 18, 2025
@wyardley

wyardley commented Feb 20, 2025

Copy link
Copy Markdown

Will a release be getting cut with these changes soon?

@takkanm

takkanm commented Feb 20, 2025

Copy link
Copy Markdown
Collaborator

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

Reviewers

2 more reviewers
@jeremy jeremy jeremy left review comments
@byroot byroot byroot left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@yahonda @hartator @hsbt @byroot @jeremy @wyardley @takkanm