ruby / stringio

Pseudo `IO` class from/to `String`.
BSD 2-Clause "Simplified" License
33 stars 26 forks source link

Eagerly defrost chilled strings #93

Closed casperisfine closed 5 months ago

casperisfine commented 6 months ago

Ref: https://bugs.ruby-lang.org/issues/20205 Fix: https://github.com/ruby/stringio/pull/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 commented 6 months ago

Why not add a new predicate function for chilled strings?

casperisfine commented 6 months ago

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?.

eregon commented 6 months ago

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: https://github.com/ruby/stringio/blob/779f71359d4760df49b70cfbcf908bc012e7e4ba/ext/stringio/extconf.rb#L3-L7

It might be possible to still require the stdlib stringio but that's a bit messy: https://github.com/ruby/strscan/pull/35/commits/3efd592ece043f4e8913096073539718e74c3cbb#diff-8629a2ff706a5015ed1623142dd173041183ccaf2aff0802f4f33493279347cbR7-R8

Alternatives I can think of:

eregon commented 6 months ago

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 commented 6 months ago

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

nobu commented 6 months ago

Or rb_str_defrost()?

casperisfine commented 5 months ago

The suggested implementation is at https://github.com/ruby/stringio/pull/94, but depends on https://github.com/ruby/ruby/pull/10355.

I suppose rb_str_chilled_p() needs a ticket and approval?

eregon commented 5 months ago

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 commented 5 months ago

Closing in favor of https://github.com/ruby/stringio/pull/94.

casperisfine commented 3 months ago

https://github.com/ruby/stringio/pull/96