Repository navigation
internal/vm: one gate and one composition for rb_warn, so the diagnostics fire, carry their position, and a redefined constant warns twice (#641, #699) - #706
Merged
Conversation
…tics fire, carry their position, and a redefined constant warns twice (#641, #699) $VERBOSE had two representations of one value and the gate read the one nobody writes. Four call sites tested vm.globals["$VERBOSE"] straight out of the map, which is unset until a program assigns it; every Ruby-level read went through verboseSlot and answered false, MRI's default. So `$VERBOSE.nil?` agreed on both engines and no Ruby-level probe could see the gate, while assigning $VERBOSE the value it already read turned the warnings on. warnEnabled (!NIL_P) and warningEnabled (RTEST) are now the only two places that decide, reading the slot through verboseSlot rather than seeding it, so a fresh VM and one handed an explicit false cannot disagree again. rbWarn is now the only place a warning is composed: error.c's warning_string -> warn_vsprintf -> err_vcatf, i.e. the current frame's "path:lineno: " then "warning: ". Nine callers passed their own "warning: " and two passed none, so two of them printed no "warning: " at all. rbCompileWarn is error.c's rb_compile_warn: the same gate and sink with an explicit position, which is what the second line of a constant redefinition needs. A redefined constant now emits both of MRI's lines, the second pointing at the original definition through the constLocs record const_source_location already keeps. Struct.new's named form keeps struct.c's own wording ("redefining constant Struct::Foo"), which a test had pinned as "already initialized". Thirteen "given block not used" warnings MRI emits were missing entirely (Array/Hash/Enumerable count, any?, all?, none?, one?, find_index), and Regexp.new's ignorecase warning was rb_warn where object.c rb_bool_expected uses rb_warning. The circular-require and autoload-did-not-define warnings wrote past Warning.warn to $stderr. Three constants the prelude re-assigned (Slim::Engine, Haml::Engine, OptParse) were already bound natively to the same objects, so with the gate fixed every rbgo run printed three warnings MRI never emits. The redundant assignments are gone and the frozen prelude regenerated. ruby/spec language+core (2206 spec files, corpus 87b1631): 23215 -> 23228. 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.
Closes #641. Addresses parts 2 and 3 of #699 (part 1, the bypassed
Warning.warn, landed in #697).The defect: one value, two representations, and the gate read the one nobody writes
rb_warn's gate was spelled four different ways. Four call sites readvm.globals["$VERBOSE"]straight out of the map, which is unset until a program assigns it; every Ruby-level read went throughverboseSlot, which answers MRI's defaultfalse. So:Assigning
$VERBOSEthe value it already read turned the warnings on. That is why it survived:$VERBOSE.nil?isfalseon both engines, so no Ruby-level probe can see the gate, and the fix is verified here only by its observable consequences — which warnings appear, on which stream, with which prefix, in which order — always A/B'd against a$VERBOSE = nilcontrol that must silence them again.C functions read (ruby/ruby, tags
v3_4_0andruby_4_0, cached under/Users/Shared/rbgo-measure/mri-c/)!NIL_P(ruby_verbose)error.c:466rb_warnRTEST(ruby_verbose)error.c:497rb_warningerror.c:450warning_string→rb_source_location_cstrfile[:line]:thenwarning:error.c:381warn_vsprintf→error.c:125err_vcatfWarning.warn, not$stderrerror.c:374rb_write_warning_str→rb_warning_warn→rb_funcallv(Warning, :warn, str)error.c:355rb_warn_category/rb_warning_warn_arityerror.c:397rb_compile_warnce->file/ce->line, and its name is the BARE onevariable.c:3694-3706const_tbl_update(ruby_4_0: 4046-4052)variable.csetup_const_entry:rb_source_location(&ce->line)at set time$VERBOSEstatesruby.c:1250-1268(-W0/-W1/-W2),ruby.c:3069verbose_setter,ruby.c:3077opt_W_getterStruct.new(name, …)words it differently and emits no second linestruct.c:273new_struct(it callsrb_mod_remove_constfirst, sorb_const_setnever sees a live entry)Regexp.new's ignorecase argument isrb_warning, notrb_warnobject.c:3493rb_bool_expected(ruby_4_0), reached fromre.c:4081enum.c315/464/1794-1798/1859/1921/2210/2271 (rb_warn),enum.c:1071(rb_warning),array.c2119/2177/6488/7792/7859/7920/7985 (rb_warn),array.c:1136(rb_warning),hash.c:4555The three
$VERBOSEstates and what each selectsrb_warnrb_warningnil-W0falseruby_verbosestarts atQfalsetrue-w/-W2, or any truthy assignment (verbose_settercoerces withRTEST)$VERBOSE = 1therefore reads back astrue, which is asserted.One rule, one place
warnEnabled()(!NIL_P) andwarningEnabled()(RTEST) are now the only two gates, and they read the slot throughverboseSlotrather than seeding the map. Seeding would have left the raw-map read alive, so any futuredeleteor a fresh VM could re-open the same hole; removing the second representation cannot.rbWarnis now the only place a warning is composed, andrbWarning,rbWarnf,rbWarningfandrbCompileWarnall route through it or its formatter. Nine of its callers passed their own literal"warning: "and two passed none, sosyswrite for buffered IOandIgnoring encoding parameter …printed with nowarning:at all. Callers now pass the text alone.Two duplicated rules were folded:
warnRedefineConst(struct.go) wrote the const_tbl_update text throughKernel#warn, with no prefix and no gate, whilewarnAlreadyInitialized(vm.go) wrote it throughrb_warn.Module#const_setused the first, the two assignment opcodes the second. They are one rule now — except forStruct.new's named form, which is genuinely different in MRI and says so at its call site.warnCircularRequireandwarnAutoloadDidNotDefinewrote tocurStderr()directly, bypassingWarning.warn, with aBooltype assertion on the unset map slot that could never be true.The brief's premise about
builtins.go:3106is wrong, and measurement says soThe task named
rbWarning1atbuiltins.go:3106as a drifted call site. It isArray#initializewith no arguments, andarray.c:1136isrb_warning("given block not used")— rbgo was right, and MRI's own split is per function, not per message:enum.c:1071(injectwith two arguments) is alsorb_warning, while the other thirteen sites arerb_warn. The change follows MRI function by function rather than making the message uniform.Per-method conformance, measured against MRI 4.0.5
Thirteen warnings MRI emits were missing entirely, all of them
given block not usedwhere an argument makes the block dead. In rbgo these methods all come fromEnumerablein the prelude (in MRI,ArrayandHashdefine their own), so they are emitted withwarn …, uplevel: 1, which isrb_warn's gate,rb_warn's sink and the caller's position:Array#count/any?/all?/none?/one?,Hash#any?,Enumerable#count/find_index/any?/all?/none?/one?, andEnumerable#inject(init, op)(that onerb_warning, so-wonly).Going the other way,
Regexp.new("a", Object.new)wasrb_warnbut MRI'srb_bool_expectedisrb_warning; it is silent at the default level again.#699's own eight-line probe, byte-for-byte on the same file:A defect this fix EXPOSED in our own prelude
With the gate working, every rbgo run printed three warnings MRI never emits:
slim.go,haml.goandoptparse.goalready bind those three names to exactly the objects the prelude then re-assigned. The redundant assignments are removed and the frozen prelude regenerated;Slim::Engine,Haml::EngineandOptParseresolve to the same classes as before. Warnings fired beforeWarningitself exists, which is also why the first build of this change panicked inwarningWarn— a useful witness that the prelude had no business warning at all.ruby/spec: 2206 spec files under
language/+core/, corpus pinned87b1631origin/main@0450238Per-file, every file that moved:
core/array/{all,any,count,none,one}_spec.rbgiven block not usedcore/enumerable/{all,any,count,find_index,none,one}_spec.rbcore/kernel/load_spec.rbWarning.warncore/kernel/require_spec.rbcore/module/autoload_spec.rbWarning.warnDownward moves: none.
core/random/bytes_spec.rbread 9 before and 8 after, and is a pre-existing flake: sampled six times per binary it gave 8, 8, 8, 9, 9, 9 onorigin/main's binary and 9, 9, 8, 9, 8, 8 on this one. One sample would have been a fabricated regression.The census figure in the task does NOT survive measurement
The wave was framed as "82 examples across 57 files behind one closed gate". It is 14 examples across 14 files, and none of them is a constant-redefinition example. Every spec that asserts one of these warnings does so through mspec's
complain, which assigns$VERBOSE— and assigning it is exactly what used to satisfy the broken gate.language/constants_spec.rb(85 passing),optional_assignments_spec.rb(74),for_spec.rb(29),core/module/const_set_spec.rb(15),const_added_spec.rb(13),const_source_location_spec.rb(39) andlanguage/assignments_spec.rb(34) are unchanged to the example, before and after. The gate was invisible to the suite for the same reason it was invisible to a Ruby probe. What the suite could see was the thirteen warnings we never emitted, and it saw twelve of them.No spec in
language/orcore/assertsprevious definition of … was hereat all, so part 3 of #699 is worth 0 examples and is in here on MRI's authority, not the corpus's.Tests
internal/vm/verbose_gate_wave45_test.go, plus corrections to eleven tests that had pinned our own divergence (wave24_lang,phase5f,env_wave16,io_modeenc_internal,wave28_modreg_internal,wave28_file_fd_open,wave31_consts_internal,getoptlong,struct_conformance,regexp_wave17_internal,regexp_bridge). One of them,TestIORbWarnSilentByDefault, documented the defect in its own comment — "rbgo's rb_warn gate still keys on the raw, unset slot, which is why nothing is printed here" — and is nowTestIORbWarnSpeaksAtTheDefaultLevel.Which new test is a witness and which a guard:
TestVerboseGateThreeStates(the UNSET rows),TestConstantRedefinitionEmitsBothLinesWithPositions,TestPreviousDefinitionPointsAtTheOriginalSite,TestStructNamedRedefineUsesStructsOwnWording,TestGivenBlockNotUsedMatchesMRIPerMethod,TestGivenBlockNotUsedCarriesTheCallersPosition,TestOverriddenWarningWarnInterceptsInternalWarnings,TestWarnGatesAgreeBetweenUnsetAndExplicitFalse.TestWarningStringAtShapes, whose two degenerate rows the single caller filters out at the call site exactly as MRI'sif (!NIL_P(ce->file) && ce->line)does, andTestNativeConstantRedefinitionHasNoSecondLine.Gates: full suite
-count=1green,go buildon native /wasip1/js,gofmt -l internal/ cmd/empty.🤖 Generated with Claude Code
Coverage — four parts, measured under Go 1.27.1 (not the host default)
The host defaults to Go 1.26.4, which counts fewer basic blocks than
setup-go@v7'sstable(1.27.1); a profile from the older toolchain is an upper bound, not a measurement. Both profiles below were produced with~/.pkgx/go.dev/v1.27.1/bin/go, each resolved bycover -funcin the tree that produced it:origin/main@0450238in a separate worktree, this branch @a885255in its own.The instrument was validated before it was trusted: the local darwin/1.27.1 below-100 list for
mainis identical, entry for entry, to CI's macos-latest list formain(16 entries). CI's ubuntu-latest list holds 3 more —internal/vm/dir.go:572cwdStillNamed,internal/vm/filestat.go:59Inspect,internal/vm/filestat.go:372registerFileStat— all filesystem-specific and untouched here.Acceptance criterion: the below-100 LIST is unchanged, not the rounded total (#704). It is:
mainbuiltins.gobootstrapshifts line 20 → 21 from the addedfmtimportFull measured function set, compared by (file, name) so line shifts cannot hide anything. Four functions removed, each with a named replacement — no orphaning:
builtins.go rbWarn1,builtins.go rbWarning1,io_class.go rbWarn,struct.go warnRedefineConstbuiltins.gorbWarn,rbWarnf,rbWarning,rbWarningf,rbCompileWarn,warningString,warningStringAt;globals.gowarnEnabled,warningEnabled;struct.gowarnStructRedefineConstAll ten new functions read 100.0% under 1.27.1, as does
warnAlreadyInitialized. No new below-100 entry appeared, so there was no candidate for the "is this even reachable?" question — the one branch pair that a single caller cannot reach,warningStringAt's empty-file and zero-line arms, iserr_vcatf's own shape and is covered by a direct unit test labelled a guard rather than a witness.-raceis not available on this host (Xcode licence), so the profiles are race-free; CI runs the raced version.Second instrument
CI's own ratchet (ubuntu, job 108497805824) reads 23230 passing, 2198 files loaded, 8 failed to load, against the local darwin sweep's 23228 with the same file counts — a two-example platform difference, of which the
core/random/bytes_spec.rbflake accounts for one.FLOORis untouched; it now sits 117 below the measurement.What the rest of the census group turned out to be
A census group formed by SYMPTOM bounds its cause's size; it does not measure it. Measured here, over the 89 files in
language/+core/that assert a warning at all (complain,suppress_warningor a$VERBOSEassignment — 186complainsites and 64suppress_warningsites):So this mechanism accounts for 12 of 263 in its own neighbourhood (14 across the whole suite, two of the moved files being outside the 89). Eleven of the twelve files that moved reached the ceiling exactly:
core/array/{all,any,count,none,one}_spec.rbandcore/enumerable/{all,any,count,find_index,none,one}_spec.rb— all now at gap 0. 34 of the 89 files are now at the ceiling.Three files moved and did not close, so here are the numbers rather than the implication:
core/module/autoload_spec.rbdefined?/autoload?/const_source_location. Owning filesinternal/vm/autoload.go+thread.gocore/kernel/require_spec.rb$LOADED_FEATUREStimingcore/kernel/load_spec.rbload(wrap: Module)andload(freeze:)And the ten largest residual gaps among the 89 are not warning defects at all — each named with its owning file, since this is the part that turns into routable work:
$!is not cleared after a successful rescue and not restored to the outer exception —expected #<StandardError: foo> to == nilthroughoutpredefined_spec.rbinternal/vm/vm.go(curExc, the rescue opcodes)$&,$`,$',$+,nil =,true =,self =), andalias-ing one then assigning raises nothing at allinternal/compiler+go-ruby-parsercore/string/chilled_string_spec.rbis 6/17. This IS a warning feature (literal string will be frozen in the future,string returned by :sym.to_s will be frozen) and is the largest warning-shaped gap left in the corpus. It is a String-representation change, not a gateinternal/object/string.go,internal/vm, the literal path ininternal/compilerModule#ruby2_keywordsdoes not actually mark the hash (expected false to == true, eight times). rbgo has the warning for declining the flag but not the flag's effectinternal/vm/modulefunc.goeval's filename/lineno arguments do not reach__FILE__,__LINE__or the SyntaxError message; rbgo reportsparse error at line 1: …where MRI reportsfile:line: …internal/vmeval path + parser error mappingconst_missingis not sent to the original scope,private_constantis not enforced under::Const. This islanguage/constants_spec.rb's residual 15, in the very file the census counted for "already initialized constant"internal/vm/vm.go-> () { … rescue … },-> (**nil),lambda { |**nil| },BEGIN { }go-ruby-parser(external)caller's precise location for a top-level constant reports[1, 3, 6, 199]where MRI reports[190, 192, 195, 199]—const_added_spec.rb. Adjacent to this PR: it is the sameframeLinemachinery thefile:lineno:prefix uses, so it is worth looking at nextinternal/vm/vm.go$_is not thread-localinternal/vm/globals.go$0does not set the process nameinternal/vm/globals.goThe two companions of #641 behave differently from each other on this axis, and that is worth stating plainly. The missing
file:lineno:prefix was worth 0 examples (every corpus assertion is a regex against the message body, never the prefix), and the missing second line was worth 0 examples (nothing inlanguage/orcore/matchesprevious definition). Both are in this PR on MRI's authority alone. What the corpus could see was the thirteen warnings we never emitted — which were not in the census's framing at all.CI verdict
All 9/9 jobs pass, including the three
-race+ coverage lanes (macos 54m58s, ubuntu 56m55s, windows 54m7s) and the ruby/spec ratchet.The acceptance criterion is confirmed on CI's own lists, not just locally. Below-100 functions, compared by (file, name) so line shifts cannot hide a substitution:
main@0450238Both lanes also print
coverage: 100.0%, but per #704 that is not what was checked — the unchanged list is.