Skip to content

Eagerly defrost chilled strings #93

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

Closed
wants to merge 1 commit into from

Conversation

casperisfine
Copy link
Contributor

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

One minor backward compatibility issue with chilled strings is that since they claim to be frozen, code using the pattern:

StringIO.new("")

Is now producing read only StringIO instances. It would be preferable to emit a warning instead.

We can detect whether the current Ruby produce chilled strings and if so preemptively attempt to mutate the given string to produce the warning immediately and preserve the original behavior.

cc @nobu

@nobu
Copy link
Member

nobu commented Mar 22, 2024

Why not add a new predicate function for chilled strings?

@casperisfine
Copy link
Contributor Author

Why not add a new predicate function for chilled strings?

We could do it indeed. My intent was for them to eventually go away in a future, so I wasn't thinking of adding a predicate on String for that. But if you think it's a good idea, I'm not opposed.

What were you thinking of? String#chilled?.

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

One minor backward compatibility issue with chilled strings
is that since they claim to be frozen, code using the pattern:

```ruby
StringIO.new("")
```

Is now producing read only `StringIO` instances. It would be
preferable to emit a warning instead.

We can detect whether the current Ruby produce chilled strings
and if so preemptively attempt to mutate the given string to
produce the warning immediately and preserve the original
behavior.
Earlopain added a commit to Earlopain/unicorn-maintained that referenced this pull request Mar 22, 2024
There is a ridiculous amount of warnings from Ruby 3.4 chilled strings,
and also a few errors because of StringIO incompatibility, see ruby/stringio#93

I'm not comfortable setting this to anything else than false. This at least
fully keeps the current behaviour
Earlopain added a commit to Earlopain/unicorn-maintained that referenced this pull request Mar 22, 2024
There is a ridiculous amount of warnings from Ruby 3.4 chilled strings,
and also a few errors because of StringIO incompatibility, see ruby/stringio#93

I'm not comfortable setting this to anything else than false. This at least
fully keeps the current behaviour
@eregon
Copy link
Member

eregon commented Mar 22, 2024

Adding lib/stringio.rb will break using the stringio gem on truffleruby, which relies on the fact that https://github.com/oracle/truffleruby/blob/master/lib/truffle/stringio.rb gets used (by not having a stringio.rb in the gem), and the stringio C extension is never used or even compiled:

if RUBY_ENGINE == 'ruby'
create_makefile('stringio')
else
File.write('Makefile', dummy_makefile("").join)
end

It might be possible to still require the stdlib stringio but that's a bit messy:
ruby/strscan@3efd592#diff-8629a2ff706a5015ed1623142dd173041183ccaf2aff0802f4f33493279347cbR7-R8

Alternatives I can think of:

  • Move that logic to C code
  • Use another Ruby file than stringio.rb, like stringio/foo.rb and required that from the C ext.

@eregon
Copy link
Member

eregon commented Mar 22, 2024

How about exposing an optional predicate for C extensions? Like rb_str_chilled_p() (or a better name).
Optional so other Ruby implementations don't have to implement chilled strings (which are more of a deprecation tool than a feature that makes sense to stay), and could be checked via #ifdef HAVE_RB_STR_CHILLED_P.
One advantage is this change would be much simpler, and there would be no FrozenError if passing a true frozen string to StringIO (which seems quite noisy when running with -d and quite slow).

@nobu
Copy link
Member

nobu commented Mar 24, 2024

What I thought is same as @eregon, “a predicate function”, not “method”.

@nobu
Copy link
Member

nobu commented Mar 24, 2024

Or rb_str_defrost()?

ioquatix pushed a commit to unicorn-ruby/unicorn that referenced this pull request Mar 25, 2024
There is a ridiculous amount of warnings from Ruby 3.4 chilled strings,
and also a few errors because of StringIO incompatibility, see ruby/stringio#93

I'm not comfortable setting this to anything else than false. This at least
fully keeps the current behaviour
@casperisfine
Copy link
Contributor Author

The suggested implementation is at #94, but depends on ruby/ruby#10355.

I suppose rb_str_chilled_p() needs a ticket and approval?

@eregon
Copy link
Member

eregon commented Mar 25, 2024

I suppose rb_str_chilled_p() needs a ticket and approval?

I'm not sure, but given the circumstances waiting for a dev meeting decision seems quite long.
I think it's fine to add this function for now, it's clear with StringIO we need such a predicate.
It seems good to file a ticket anyway in case someone has an objection regarding the name.

@casperisfine
Copy link
Contributor Author

Closing in favor of #94.

@casperisfine
Copy link
Contributor Author

#96

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.

5 participants