Fix ClassCastException in ARGF.gets and readline when $stdin is not an IO - #9785

Proposal for discussion: route Kernel#readline through ARGF.readline

This PR deliberately doesn't change Kernel#readline, since it's so widely used. Here's the change that would make it match MRI exactly, for discussion. @headius, you know this area better than we do: if you think it's safe, I'll add it to this PR. If not, we'll leave it out.

diff --git a/core/src/main/java/org/jruby/RubyKernel.java b/core/src/main/java/org/jruby/RubyKernel.java
index 5d656b9ede..9c5c592622 100644
--- a/core/src/main/java/org/jruby/RubyKernel.java
+++ b/core/src/main/java/org/jruby/RubyKernel.java
@@ -833,15 +833,14 @@ public class RubyKernel {
         return context.nil;
     }
 
+    // MRI: rb_f_readline
     @JRubyMethod(optional = 1, keywords = true, checkArity = false, module = true, visibility = PRIVATE)
     public static IRubyObject readline(ThreadContext context, IRubyObject recv, IRubyObject[] args) {
-        IRubyObject line = gets(context, recv, args);
-
-        if (line.isNil()) {
-            throw context.runtime.newEOFError();
-        }
+        var ArgsFile = argsFile(context);
 
-        return line;
+        return recv == ArgsFile ?
+                RubyArgsFile.readline(context, ArgsFile, args) :
+                sites(context).readline.call(context, ArgsFile, ArgsFile, args);
     }
 
     @JRubyMethod(optional = 1, keywords = true, checkArity = false, module = true, visibility = PRIVATE)
diff --git a/core/src/main/java/org/jruby/runtime/JavaSites.java b/core/src/main/java/org/jruby/runtime/JavaSites.java
index 22c3565174..2d3238ee02 100644
--- a/core/src/main/java/org/jruby/runtime/JavaSites.java
+++ b/core/src/main/java/org/jruby/runtime/JavaSites.java
@@ -96,6 +96,7 @@ public class JavaSites {
         public final CallSite to_str = new FunctionalCachingCallSite("to_str");
         public final CallSite getc = new FunctionalCachingCallSite("getc");
         public final CallSite gets = new FunctionalCachingCallSite("gets");
+        public final CallSite readline = new FunctionalCachingCallSite("readline");
         public final CallSite putc = new FunctionalCachingCallSite("putc");
         public final CallSite puts = new FunctionalCachingCallSite("puts");
         public final CallSite initialize_copy = new FunctionalCachingCallSite("initialize_copy");

What it changes. Today Kernel#readline calls Kernel#gets, which dispatches ARGF.gets, and then raises EOFError on nil. MRI's rb_f_readline calls ARGF.readline instead: argf_readline directly when recv == argf, otherwise forward(argf, rb_intern("readline"), ...). The patch mirrors Kernel#gets (RubyKernel.java:351-357) with readline in place of gets. That means a new FunctionalCachingCallSite("readline") in KernelSites, next to the existing gets site. Nothing in JRuby calls RubyKernel.readline from Java.

Measured. Each case runs in a fresh process with file containing "a\nb\n". Columns: unmodified jruby-10.0, this PR as pushed, and this PR plus the patch.

(The EOFError message capitalization, "End of file reached", is the same in all JRuby columns and unrelated.)

With the patch, mspec ci over spec/ruby/core/{io,file,enumerator,argf,kernel} and library/stringio gives the same result as without it (2 unrelated Kernel#require failures, also on jruby-10.0).

The risk, as I see it. For reading files and real STDIN, nothing changes. The visible change is which ARGF method Kernel#readline dispatches to: ARGF.readline instead of ARGF.gets (the last two rows). That matches CRuby. But code that redefines or stubs ARGF.gets to feed a top-level readline (for example allow(ARGF).to receive(:gets) in a test) would stop working on JRuby. Code that stubs ARGF.readline would start working. I don't know how common the former is in JRuby-specific code.

Depends on b64041f. While testing this patch I found that ARGF's forwarding to a non-IO $stdin used a functional call, which can reach private methods. With the patch, a $stdin that has gets but no public readline sent ARGF.readline → private Kernel#readline → ARGF.readline → ... and overflowed the stack. That same commit also fixes overflows this PR (and, for readlines, unmodified jruby-10.0) already had with a plain Object as $stdin. ARGF's forwarding now uses NormalCachingCallSites, the equivalent of MRI's rb_funcallv_public_kw in forward_current, and raises NoMethodError like CRuby. So the patch above is only safe on top of that commit.

⚠️ Rebase note: #9784 and #9785 conflict

Whichever of #9784 and #9785 merges second needs a rebase. I trial-merged argf-gets-non-io-stdin (#9785) into argf-enumerator-keywords (#9784). There are two conflicts, and the resolution is to keep both sides in each:

1. RubyArgsFile.readlines, forwarding to a non-IO $stdin. #9784 restores callInfo so keywords reach $stdin.readlines. #9785 switches the call to a public-only call site so a plain-object $stdin doesn't recurse into the private Kernel#readlines. The resolved code needs both:

        if (!(data.currentFile instanceof RubyIO)) {
            context.callInfo = callInfo; // restore callInfo for kwargs
            return sites(context).readlines.call(context, recv, data.currentFile, args);
        }

Don't keep only #9784's callMethod(context, "readlines", args). That brings back the StackOverflowError with $stdin = Object.new. Don't keep only #9785's line either, because keywords would then be passed as a positional Hash.

2. spec/ruby/core/argf/shared/gets.rb. Both PRs add examples at the same spot. Keep both sets: #9784's chomp/separator/limit examples, then #9785's when reading $stdin and $stdin is not an IO block.

ARGF.readline auto-merges correctly. It gets #9784's optional = 2 annotation and #9785's forwarding to $stdin.readline.

Leave as is: eachLineCommon in #9784 forwards each_line with callMethod, which can reach private methods. Unlike gets, readline and readlines, that's correct. MRI's argf_each_line uses rb_block_call_kw → iterate_method → CALL_FCALL (vm_eval.c), not forward_current's public call, so it shouldn't be switched to a NormalCachingCallSite.

Verified: with this resolution, the trial merge builds, all spec/ruby/core/argf specs pass on CRuby 4.1.0dev and JRuby (186 examples), and mspec ci over core/{io,file,enumerator,argf,kernel} and library/stringio shows only the 2 unrelated Kernel#require failures that jruby-10.0 also has.

@headius thanks for the review. One open question from earlier: should I add the Kernel#readline change from this comment (routing it through ARGF.readline to match MRI)? If you'd rather not touch Kernel#readline, I'll leave it out and mark this ready as is.

@headius two questions before this merges.

1. NormalCachingCallSite deprecation warnings. CI shows three [deprecation] NormalCachingCallSite ... has been deprecated warnings at JavaSites.java:490-492. Those are the new ArgfSites gets/readline/readlines sites from this PR. The class is @Deprecated(since = "9.4-") and its javadoc says it was replaced by MonomorphicCallSite, which also uses CallType.NORMAL, so the swap doesn't change behavior (public-only dispatch).

Should I switch these three to MonomorphicCallSite in this PR? I'd lean yes. Separately, you asked about updating all of them in a new PR: a grep of origin/master under core/src/main/java finds no other users of NormalCachingCallSite besides the class itself, so this PR's three would be the only ones. A follow-up PR would only be needed to remove the deprecated class.

2. Kernel#readline. Do you want it routed through ARGF.readline to match MRI's rb_f_readline? The patch and measurements are in this comment. The tradeoff is that code stubbing ARGF.gets to feed a top-level readline would stop working, and code stubbing ARGF.readline would start working. If you'd rather not touch Kernel#readline, I'll leave it out.

Marking this ready for review now.

Let's err in the direction of keeping things moving and do the additional cleanup separately.

I support both the Kernel.readline => ARGF move and the deprecation cleanups.