[3.4 backport] Prism ^D fix #14358
[3.4 backport] Prism ^D fix #14358
Conversation
The purpose of this commit is to fix Bug #21188. We need to detect when
stdin has run in to an EOF case. Unfortunately we can't _call_ the eof
function on IO because it will block.
Here is a short script to demonstrate the issue:
```ruby
x = STDIN.gets
puts x
puts x.eof?
```
If you run the script, then type some characters (but _NOT_ a newline),
then hit Ctrl-D twice, it will print the input string. Unfortunately,
calling `eof?` will try to read from STDIN again causing us to need a
3rd Ctrl-D to exit the program.
Before introducing the EOF callback to Prism, the input loop looked
kind of like this:
```ruby
loop do
str = STDIN.gets
process(str)
if str.nil?
p :DONE
end
end
```
Which required 3 Ctrl-D to exit. If we naively changed it to something
like this:
```ruby
loop do
str = STDIN.gets
process(str)
if STDIN.eof?
p :DONE
end
end
```
It would still require 3 Ctrl-D because `eof?` would block. In this
patch, we're wrapping the IO object, checking the buffer for a newline
and length, and then using that to simulate a non-blocking eof? method.
This commit wraps STDIN and emulates a non-blocking `eof` function.
[Backport #21188]
|
A couple of things:
|
This is the general expectations for default gems when they cannot be upgraded to the latest version of the gem in Ruby's stable branches. If we introduced a feature/syntax to Prism that Ruby 3.4 shouldn't have, ruby/prism probably should cut a ruby_3_4 branch and make releases separately from the I'll merge this now, but can you either release such a version and backport the version bump to this branch now, or make me an owner of the gem so that I can do it myself now or when I release Ruby 3.4.6? |
I don't understand what we're trying to accomplish here. Are we backporting the Prism fix just because it's a bugfix? As such, we need to ship a new version of Prism? Bumping the prism version in |
Cool, that's a valid option, to bump to the latest version. We'd still need to coordinate a release to keep the version number unique and meaningful though. Looks like no current release contains the API change in this diff yet. Though a bump like that might be risky for the branch maintainer, it's useful to know that it's an option.
The problem is that making any changes at all will make the default gem version 1.2.0 of Prism different from the 1.2.0 one can get on rubygems.org. Making changes to support both versions still runs into this problem. PS. we may want to pick a better version name than I did. I picked 1.3.1 because this is a breaking change, but this version is more closely related to 1.2.0 than 1.3.0... Maybe we need a separate versioning scheme for these backport Prism versions. |
Sorry I guess I overlooked this discussion.
As a branch maintainer, I'm generally open to bumping gem versions to the latest, which may include new features, as long as they are backward-compatible. We backport rubygems and bundler which tend to have way riskier changes :p So which one do you all want in the |
|
I vote for cutting a new Prism gem release and pointing to it. It's been a while since the last Prism release anyways: ruby/prism#3553 and also, doing that will also do the backport for https://bugs.ruby-lang.org/issues/21337 Will take a little bit of integration work to set the syntax version correctly so we don't accidentally pull in https://bugs.ruby-lang.org/issues/20925 and ruby/prism#3337 which Matz just approved but this seems like the best long term play. |
|
@kddnewton WDYT about making @tenderlove and @eileencodes gem owners of prism.gem? I, as a release manager of Ruby 3.4, think we need at least one person who has full-time attention to Ruby and can cut a release of Prism as needed. |
|
v1.5.0 is released. Please let me know if there's something I should do with this specific ticket or any backport. |
|
Thanks! I'll work on sync-ing Prism to that version on Have you also invited them to be gem owners? |
|
It doesn't compile 😞 https://github.com/ruby/ruby/actions/runs/17684937914/job/50267562914?pr=14532#step:9:176 |
|
It builds correctly on my local Linux environment, but every macOS-based CI seems to be failing with it. |
|
It works fine on my local macOS environment too 🤔 |
|
It seems like we also can't use Prism 1.5.0 for Ruby 3.4 anyway? Do we need to create a Prism branch and version for 1) Failure:
TestSyntax#test_methoddef_endless_command [/home/runner/work/ruby/ruby/src/test/ruby/test_syntax.rb:1800]:
/home/runner/work/ruby/ruby/src/test/ruby/test_syntax.rb:1800:in 'TestSyntax#test_methoddef_endless_command'.
```
private def foo = puts "Hello"
```
no-newline.
SyntaxError expected but nothing was raised.
2) Failure:
Prism::TestCompilePrism#test_concatenated_StringNode [/home/runner/work/ruby/ruby/src/test/ruby/test_compile_prism.rb:624]:
@/home/runner/work/ruby/ruby/src/test/ruby/test_compile_prism.rb:624.
expected but was
.
Finished tests in 221.929572s, 147.4747 tests/s, 29655.7955 assertions/s.
32729 tests, 6581498 assertions, 2 failures, 0 errors, 364 skips
https://github.com/ruby/ruby/actions/runs/17684937953/job/50267562966?pr=14532 |
|
It contains 3.5 specific syntax now and currently ruby always uses the latest version that prism knows about. You have to explicitly request 3.4 to get the correct behaviour like #14523 |
|
I see. Thanks for working on that. I hope backporting it will fix the rest of the problems too. |
|
@k0kubun would you be able to make it so that the ruby 3.4 branch requests ruby 3.4 syntax? |
|
There is this commit: 7dbd9c2 I lack the context but it looks like it was never reverse synced (or mentioned in https://bugs.ruby-lang.org/issues/21187 for that matter) |
|
Hm. So I guess this needs to be fixed (or just reverted) on Prism's end first? |
|
So on July 21st at ruby-lang Slack (#general-ja), hsbt reported (probably non-GitHub Actions) CI failures because of the reverted commit and shared that he was going to revert it. Aaron acknowledged it and said he would fix Prism and Ruby together, but I guess we've never got to fix it. For now, I'd like to suggest reverse sync-ing the revert to Prism, make another Prism release, and re-open the Redmine ticket if necessary. I'll just forward-fix the problem by merging the "almost Prism 1.5.0" (with the revert) to ruby_3_4, but it'd be nice to have a release with the revert soon. Otherwise I'll (revert backports and) postpone Prism backports to Ruby 3.4.7. |
|
The problem that this change intended to fix still seems to be fixed even with the revert. There is more context in #13966 for those that understand but I tested the repro from https://bugs.ruby-lang.org/issues/21187 and it is as expected on current master. 3.4.5 still fails since it never got backported. So reverting seems ok? Although I cannot say why @tenderlove made that change to begin with. I assume there were reasons. |
|
Huh, ok. Let's start from reverting it then. Unfortunately Aaron seems to be out next week (as well as this week), so I guess I'll need Kevin to make another release. I'll prepare the reverse sync PR. |
|
So @kddnewton Could you merge ruby/prism#3643 and make another release of Prism? @tenderlove is out until Sep 21st, so I don't think he has time to attempt to fix his patch before the Ruby 3.4.6 release (next Monday). |
|
v1.5.1 is out with the revert. |
6 participants
Because this bugfix touches the Prism gem, there is
a problem of having two versions of the gem sharing
the same version number. With that users would get
different copies depending on the version they
install the gem from, or get a checksum error from
RubyGems or Bundler, which is undesirable.
I don't think we can bump to the latest Prism gem
because that would also bring in syntax changes
that are only in Ruby 3.5.
I've picked a version number that ruby/prism hasn't
used yet. Maybe as an optional step, we should also
release this version of Prism to RubyGems when
releasing the next 3.4 version. Otherwise, Prism
needs to remember to avoid this version number
in the future to make sure versions are uniquely
identifiable.
CC @kddnewton