Repository navigation
Warnings reach stderr, not stdout (#667); Tempfile ORs its mode: with RDWR|CREAT|EXCL (#670) - #675
Merged
Merged
Conversation
vm.New set errOut = out, and cmd/rbgo passed os.Stdout for both, so every
rb_warn, Kernel#warn, $stderr.puts and STDERR.puts landed on stdout:
$ rbgo warntest.rb 2>/dev/null
warntest.rb:1: warning: given block not used # should be silent
MRI separates the two at the descriptor. io.c v3_4_0 Init_IO builds rb_stdout
from rb_io_prep_stdout() (the C stdout FILE*) and rb_stderr from
rb_io_prep_stderr() (the C stderr FILE*, FMODE_WRITABLE|FMODE_SYNC), and every
warning reaches the second one: error.c rb_warn -> rb_write_warning_str ->
Warning.warn -> rb_write_error_str, which io.c writes to rb_ractor_stderr(),
falling back to fwrite(..., stderr) while $stderr is still the original.
NewWithStderr(out, errOut) splits them; New(out) keeps merging them, which is
what an embedder capturing a program's whole output in one buffer wants, and
leaves every existing caller (run.go, cmd/wasm, the tests) unchanged. The CLI
passes os.Stdout and os.Stderr.
errOut is only the fallback sink: every diagnostic already resolves through
vm.curStderr(), which prefers the Ruby-level $stderr global exactly as
rb_write_error_str prefers rb_ractor_stderr(). Reassigning $stderr therefore
still captures warnings, so mspec's `complain` matcher keeps working -- which is
also why the conformance suite never observed this defect.
Verified against MRI 4.0.5 on both streams separately (2>/dev/null and
2>&1 >/dev/null), byte for byte, for rb_warn, Kernel#warn, $stderr.puts,
STDERR.puts, Kernel#abort's message, the deprecated-constant path, and the
uncaught-exception report.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Tempfile#initialize passed mode: straight to File.open, so
Tempfile.new("x", encoding: Encoding::BINARY, mode: File::RDONLY)
opened flag 0 -- read-only -- on a path that does not exist yet and raised
Errno::ENOENT. That call is the `before :each` of the "to a Tempfile" block of
core/io/copy_stream_spec.rb, so eight examples never ran.
mode: is an INTEGER flag set, not an fopen-style String. lib/tempfile.rb v3_4_0
Tempfile#initialize defaults it to 0 and does
@mode = mode|File::RDWR|File::CREAT|File::EXCL
before the open, so RDONLY (which is 0) still creates a readable-and-writable
file. Tempfile.create does the same in create_with_filename. Following the
default from "w+" to 0 also makes a String mode raise NoMethodError on #|, which
is what library/tempfile/create_spec.rb asserts:
-> { Tempfile.create(mode: "wb") }.should.raise(NoMethodError, /undefined method ['`]|' for .+String/)
The other keywords now reach File.open instead of being swallowed, as in MRI:
Tempfile hands them to Dir::Tmpname.create, which consumes max_try: and yields
the rest back for the open with perm: forced to 0600. That is how the spec's
encoding: Encoding::BINARY reaches the file. anonymous: is Tempfile.create's own
keyword and is consumed there rather than forwarded.
Measured through scripts/conformance/rubyspec/spec_helper.rb against MRI 4.0.5:
core/io/copy_stream_spec.rb 54 -> 59 (ceiling 62)
library/tempfile/create_spec.rb 9 -> 13 (ceiling 18)
library/tempfile/initialize_spec.rb 0 -> 2 (ceiling 2)
library/tempfile/open_spec.rb 5 -> 6 (ceiling 9)
prelude_frozen_gen.go regenerated with `go run ./cmd/freeze-prelude`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
With #670's mode fix the "to a Tempfile" block of core/io/copy_stream_spec.rb ran, and 3 of its 8 examples still failed: #pos stayed 0 after a copy, a preceding #write was ignored, and the handle's own buffer overwrote the copied bytes on the next flush. copyStreamRead/copyStreamWrite took the String/#to_path branch for a Tempfile, writing the file behind the handle's back. io.c v3_4_0 copy_stream_body checks #to_io FIRST, on both arguments: VALUE tmp_io = rb_io_check_io(dst_io); if (!NIL_P(tmp_io)) dst_io = GetWriteIO(tmp_io); else if (!RB_TYPE_P(dst_io, T_FILE)) { ... rb_class_new_instance(..., rb_cFile) } An object that is not an IO but answers #to_io IS that IO, and copy_stream works through the handle -- at its current position, leaving the position at the last write. A Tempfile answers both #to_io and #to_path (both delegate to the File it wraps), so the order decides which one wins. The new check stays inside the String/#to_path branch because that is where MRI puts it: an object answering #to_io but NOT #to_path gets dst_fptr == NULL and goes to copy_stream_fallback's duck-typed #read/#write instead. core/io/copy_stream_spec.rb 59 -> 62 (ceiling 62, MRI 4.0.5) Nothing else in core/io, core/array, core/hash or core/kernel moved (422 files measured before and after). Note for the reviewer: io_class.go was outside this lane's granted file set, which named internal/vm/io.go -- copy_stream lives next door. No sibling branch or open PR touches it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every expectation was taken from MRI 4.0.5 running the same program, and each
stream was read separately (`2>/dev/null` and `2>&1 >/dev/null`) rather than
merged -- under the old wiring the warning text was present either way, only on
descriptor 1, which is exactly how this defect stayed invisible.
internal/vm/diagnostic_stream_test.go
Six diagnostic kinds on a split pair of writers: rb_warn (given block not
used), Kernel#warn, $stderr/STDERR writes, rb_warning under $VERBOSE=true, the
deprecated-constant path, and Kernel#abort's message. Plus: New still merges
them (the embedder contract), $stderr reassignment still captures everything
(mspec's `complain`), and the two writers share nothing.
cmd/rbgo/stream_test.go
The descriptor-level half: real os.Pipe pairs swapped in for os.Stdout and
os.Stderr around run(), so the assertion is about actual file descriptors and
not about a pair of buffers being distinct. Ungated, so CI runs it.
internal/vm/tempfile_mode_test.go
Tempfile mode: as an integer flag set (RDONLY still creates a writable file,
APPEND survives the OR, a String raises NoMethodError on #|), the forwarded
options (encoding, perm 0600 under a runtime.GOOS guard, max_try consumed,
anonymous consumed by .create), and IO.copy_stream through #to_io -- including
the two negative cases that must NOT change: an object with #to_io but no
#to_path still takes the duck-typed #write fallback, and a String destination
is still opened and truncated.
Each witness was checked by A/B against the old code, not assumed:
- errOut forced back to out: all 6 diagnostic cases + the share-nothing test
fail; the CLI descriptor test fails on both streams.
- to_io check removed: 3 of the 5 copy_stream cases fail (pos 0, empty file,
lost prelude); the 2 negative cases pass both ways, as intended.
- prelude reverted: 3 of the Tempfile cases fail (2 with Errno::ENOENT) and
the permission test fails.
TestReassignedStderrCapturesWarnings passes both ways -- it is a guard on the
$stderr routing, not a witness of #667, and its comment now says so.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The coverage gate found it: copyStreamRead sat at 96.9% because every test
exercised the DESTINATION side only. The one uncovered block was the
rb_io_check_io step on the source.
It is not just a coverage hole, it is observable behaviour. Read through the
handle, the copy starts at the source's current position and leaves it advanced,
where the old path branch re-read from byte 0:
src.write("HEADbody"); src.flush; src.rewind; src.read(4)
IO.copy_stream(src, dst) # => 4, "body", src.pos == 8
Byte-identical to MRI 4.0.5.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two measured defects, plus the third the measurement turned up while verifying
the second. Closes #667 and #670.
Every expectation below was taken from MRI 4.0.5 running the same program, and
each stream was read separately (
2>/dev/nulland2>&1 >/dev/null) ratherthan merged.
#667 — warnings reached stdout, the program's data stream
vm.NewseterrOut = outandcmd/rbgo/main.gopassedos.Stdoutfor both,so every diagnostic landed on descriptor 1:
MRI separates the two at the descriptor.
io.cv3_4_0Init_IObuildsrb_stdoutfromrb_io_prep_stdout()(the CstdoutFILE*) andrb_stderrfrom
rb_io_prep_stderr()(the CstderrFILE*,FMODE_WRITABLE|FMODE_SYNC),and every warning reaches the second one:
error.crb_warn→rb_write_warning_str→Warning.warn→rb_write_error_str, whichio.cwrites to
rb_ractor_stderr(), falling back tofwrite(..., stderr)while$stderris still the original stream.NewWithStderr(out, errOut)splits them;New(out)keeps merging them, which iswhat an embedder capturing a program's whole output in one buffer wants, and
leaves every existing caller (
run.go,cmd/wasm, the tests) unchanged. The CLIpasses
os.Stdoutandos.Stderr.errOutis only the fallback sink: every diagnostic already resolves throughvm.curStderr(), which prefers the Ruby-level$stderrglobal exactly asrb_write_error_strprefersrb_ractor_stderr(). Reassigning$stderrtherefore still captures warnings, so mspec's
complainmatcher keeps working —which is also why the conformance suite never observed this defect.
Where each kind of diagnostic now goes (verified byte for byte against MRI on
each stream in isolation):
$stderrrb_warn(given block not used, …)rb_warning(-wonly)Kernel#warn(viaWarning.warn)$stderr.puts/STDERR.putsSTDERRis the original, as in MRI)Kernel#abort's messageputs/print/pOnly the stream is asserted there. The circular-require warning's text still
diverges from MRI (MRI prefixes it with the requiring frame and, under
-w, addsa backtrace); that is a pre-existing text defect, not a stream one, and is left
alone here.
#670 —
Tempfileignored itsmode:Tempfile#initializepassedmode:straight toFile.open, soopened flag
0— read-only — on a path that does not exist yet and raisedErrno::ENOENT. That call is thebefore :eachof the"to a Tempfile"block ofcore/io/copy_stream_spec.rb, so eight examples never ran.mode:is an integer flag set, not an fopen-style String.lib/tempfile.rbv3_4_0
Tempfile#initializedefaults it to0and does@mode = mode|File::RDWR|File::CREAT|File::EXCLbefore the open, soRDONLY(which is
0) still creates a readable-and-writable file;create_with_filenamedoes the same. Following the default from"w+"to0also makes a String mode raise
NoMethodErroron#|, which is whatlibrary/tempfile/create_spec.rbasserts.The other keywords now reach
File.openinstead of being swallowed, as in MRI:Tempfilehands them toDir::Tmpname.create, which consumesmax_try:andyields the rest back for the open with
perm:forced to0600. That is how thespec's
encoding: Encoding::BINARYreaches the file.anonymous:isTempfile.create's own keyword and is consumed there.prelude_frozen_gen.goregenerated withgo run ./cmd/freeze-preludefrom therepo root (91 insertions / 49 deletions — not byte-identical).
IO.copy_streamhonours#to_iobefore#to_pathWith the mode fix the
"to a Tempfile"block ran, and 3 of its 8 examplesstill failed:
#posstayed0after a copy, a preceding#writewas lost, andthe handle's own buffer overwrote the copied bytes on the next flush.
copyStreamRead/copyStreamWritetook the String/#to_pathbranch for aTempfile and wrote the file behind the handle's back.
io.cv3_4_0copy_stream_bodychecks#to_iofirst, on both arguments:An object that is not an IO but answers
#to_iois that IO, and the copygoes through the handle — at its current position, leaving the position at the
last write. A Tempfile answers both
#to_ioand#to_path(both delegate to theFile it wraps), so the order decides. The new check stays inside the
String/
#to_pathbranch because that is where MRI puts it: an object answering#to_iobut not#to_pathgetsdst_fptr == NULLand takescopy_stream_fallback's duck-typed#read/#writeinstead.Measured
Through
scripts/conformance/rubyspec/spec_helper.rb, serially (2-way), againstMRI 4.0.5 run through the same shim for the per-file ceiling. 422 files of
core/io core/array core/hash core/kernelplus the 10 oflibrary/tempfile,before and after.
core/io/copy_stream_spec.rblibrary/tempfile/create_spec.rblibrary/tempfile/initialize_spec.rblibrary/tempfile/open_spec.rbTotals:
core/{io,array,hash,kernel}6537 → 6545 (ceiling 6725);library/tempfile17 → 24 (ceiling 36).A stream change is global, so four directories are not a sufficient
regression check. Widened to the whole of
core+language— 2206 files,22485 → 22493 passing — and exactly one file moved:
copy_stream_spec.rb+8. Nothing regressed. (
library/,optional/andsecurity/were left out ofthat widening: they are dominated by network specs that hit the per-file timeout
in both runs, so their FILEFAIL set is not reproducible — and a sibling session
was running its own test suite on this machine at the time.)
The briefed payoff for #667 did not exist, and measuring is what showed it.
The stream fix moves zero conformance examples: the
given block not usedfamily across
core/arrayandcore/hashwas said to be blocked by it, butthose examples already passed, because
rbWarn1has gone throughvm.curStderr()— and therefore through the reassigned$stderrthatcomplaininstalls — since #666. #667 is a real defect about piping and aboutrbgo gen.rb > out.json, and it is invisible to this corpus by construction.The 8 examples #670 promised did arrive, but only 5 of them came from the mode
fix; the other 3 needed the
#to_ioordering above.Left, with the reason
Kernel#abortwritesvm.errOutdirectly (internal/vm/kernel_introspect.go),not
vm.curStderr(). It now reaches the right descriptor, but a program thatreassigns
$stderrwill not capture it, where MRI'srb_f_abortcallsrb_io_puts(1, args, rb_ractor_stderr()). One line, in a file outside thislane's set.
closed_main.go/closed_main_wasm.gostill callvm.New(os.Stdout), soa
rbgo build --closedbinary keeps the merged streams. Same one-line changeas
main.go; both files are outside this lane's set.SystemExit's status never reaches the process.rbgo -e 'exit 3'exits 0where MRI exits 3, and
abortexits 0 where MRI exits 1. Found while checkingabort's stream; it is a separate defect from Warnings go to stdout:rbgo prog.rb 2>/dev/nullstill prints them #667 and needs its own issue.Tempfile.createreturns aTempfile, not aFile. MRI'screate_with_filenamehands back the realFile(library/tempfile/create_spec.rbsays so explicitly), which is most of that file's remaining 5-example gap.
Tempfile.newwith a block also yields, where MRI warns"Tempfile.new doesn't call the given block."and does not. Both areTempfilesurface rather than themode:defect Tempfile#initialize does not OR mode with RDWR|CREAT|EXCL, so mode: File::RDONLY raises ENOENT (8 ruby/spec examples) #670 names.File::CREATis 64 andFile::EXCLis 128 in rbgo (the Linux values) wheremacOS has 512 and 2048. Self-consistent, so nothing here depends on it, but a
program that hard-codes a platform's
O_*numbers would disagree. Out of scopeand a constants question, not a Tempfile one.
internal/vm/io_class.gowas outside this lane's granted file set, whichnamed
internal/vm/io.go—copy_streamlives next door. Flagging it ratherthan hiding it; no sibling branch or open PR touches that file.
Gates
CGO_ENABLED=0 go build ./...— clean.CGO_ENABLED=0 GOWORK=off go test ./...— full suite green, no test neededupdating: the existing tests all construct the VM with
vm.New, whose mergedbehaviour is unchanged by design.
origin/main's — 17 functions, measured by running the same-coverpkg=./internal/...profile on a detached worktree atorigin/mainrather than trusting the rounded total (both report 99.9% by this measurement).
The gate caught a real hole on the way:
copyStreamReadsat at 96.9% becauseevery
copy_streamtest exercised the destination side only, so therb_io_check_iostep on the SOURCE was never reached. Covering it is the lastcommit, and it pins observable behaviour, not just a line.
gofmt -l internal/ cmd/— empty.CGO_ENABLED=0 GOOS=wasip1 GOARCH=wasm go build ./...— clean.runtime.GOOSguard on the only OS-specific assertion (the0600permissionbits).
Each new test was checked by A/B against the old code, not assumed to be a
witness: forcing
errOutback tooutfails all 6 diagnostic cases and theCLI descriptor test; removing the
#to_iocheck fails 3 of the 5copy_streamcases; reverting the prelude fails 3 Tempfile cases (2 with
Errno::ENOENT) andthe permission test.
TestReassignedStderrCapturesWarningspasses both ways —it is a guard on the
$stderrrouting, not a witness of #667, and its commentsays so.
🤖 Generated with Claude Code