Repository navigation
Resolve File path arguments as CRuby's rb_get_path does - #77
jiro-by-nori[bot] wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Reviewed head 3fa843e.
I cannot approve this pull request under the approver policy: it changes 995 lines across 5 files, above the configured max_changed_lines of 500. The overage is not authoring churn — 951 of the lines are the committed regenerated artifacts RubyCore/Generated/Prelude.lean (+201/−48) and PreludeJson.lean (+359/−335), which must change with the 36-line source edit in prelude/prelude.rb and are checked by make gen-check. It is therefore not fixable by splitting the change. @sam-xif, this needs an owner decision: merge it manually as an approved size exception, or change max_changed_lines in the approver policy. I am not approving it.
What I checked in the source, for the record:
File.__path_argacceptsString, raisesTypeError, "no implicit conversion of <name> into String"otherwise, and gatesto_path/to_strobjects as unmodeled instead of guessing.__path_type_namespecial-casesnil/true/false, which matches CRuby's message spellings; other values useclass.to_s.basename,dirname, andextnamenow route through the helper (extnameviabasename, so the path is validated once);joinvalidates each part with__join_arg, which gatesArrayparts instead of treating them as scalar non-Strings.File.join()with no parts still returns"".- The generated Lean matches the source shape: the old
to_ssends are replaced by__path_arg/__join_argcalls, and the only other diffs in the generated files are__dt_ttemp-variable renumbering from the insertion. - The new regression case
difftest/corpus/regressions/file-path-type-errors.rbpinsdirname(123),basename(123),join(1, 2),extname(:sym),dirname(nil)andjoin(true, "x"), with metadata marking itstatus: fixed. No existing test, skip, or assertion is removed or weakened by this diff.
Checks observed: all green at this head — CI plus front-end, conformance, proofs, docs, lint, and checks not weakened. No unresolved review threads. The PR body's test plan reports difftest run --tier regressions at 219 held / 0 failures, make gen-check fresh, conformance 1287 ran / 0 disagreement, and check-not-weakened passing.
Files reviewed: ruby-lean/prelude/prelude.rb, ruby-lean/RubyCore/Generated/Prelude.lean, ruby-lean/RubyCore/Generated/PreludeJson.lean, difftest/corpus/regressions/file-path-type-errors.rb, difftest/corpus/regressions/file-path-type-errors.json.
Validation: read the source and generated diffs and the new corpus case; relied on the green conformance, proofs, front-end, docs, lint, and checks not weakened jobs for the Lean build, CRuby differential, and generated-file freshness. I did not independently run make gen-check or the difftest tier (no Lean/CRuby toolchain on the reviewer machine).
Risk: the behavior change is confined to File path helpers; the main residual risk is the unmodeled to_path/to_str and File.join array cases, which now gate rather than answer. No security-sensitive surface.
Verdict: not approved — size policy; owner decision needed.
`File.basename`, `dirname`, `extname` and `join` resolved every path argument
with `to_s`. That answers where CRuby raises `TypeError` — `File.dirname(123)`
was `"."`, `File.basename(123)` was `"123"`, `File.join(1, 2)` was `"1/2"` — a
wrong answer in the direction `AGENTS.md` rule 3 calls a bug. It also ignored
the `to_path`/`to_str` conversions CRuby performs, so an object answering
`to_path` reached string concatenation instead of being used as its path
(`File.dirname(Pathname.new("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/p/q"))` only worked because `Pathname#to_s`
happens to agree). `File.join(["a", "b"], "c")` answered `"[\"a\", \"b\"]/c"`
where CRuby flattens.
The conversion is now two twins of the C. `__str_value` is `StringValue`, with
its two distinct messages: "no implicit conversion of X into String" when
nothing converts, and "can't convert X to String (X#to_str gives Y)" when the
conversion answers a non-String. `__file_path` is `rb_get_path`: a String is
the path, otherwise `to_path` and then `to_str` are consulted (`to_path` wins
when an object answers both), and `rb_get_path_check_convert`'s NUL rule
rejects a path containing a NUL byte. Both sit next to `__check_convert` and
`__conv_callable?`, the `rb_check_funcall` twins they are built on.
`File.basename` converts its suffix *before* the path and only when a second
argument was passed, as `rb_file_s_basename` does, so an explicit `nil` suffix
is the `TypeError` CRuby raises rather than "no suffix", a bad suffix is
reported before a bad path, and an out-of-range arity gets CRuby's `1..2`
message. `File.join` keeps CRuby's per-part rule: a String part is checked with
`StringValueCStr` ("string contains null byte") and anything else is a path. An
Array part still gates by name — CRuby flattens it, which is a separate
decision — instead of being answered with `Array#to_s`. `File.extname` builds
its last component directly instead of re-entering `basename`, so a `to_str`
that answers a different string on each call is converted once, as in CRuby.
Add a fixed regression case covering the accept/reject boundary, both
conversion messages, the suffix ordering, the arity, and the two NUL messages.
Closes #57.
Authored by Jiro, an autonomous agent.
Co-authored-by: Nori <contact@tilework.tech>
3fa843e to
a544935
Compare
What this changes
Closes #57. The pure path helpers —
File.basename,File.dirname,File.extnameandFile.join— resolved every path argument withto_s. That answers where CRuby raises, and it skips theto_path/to_strconversions CRuby does perform, so an object that answersto_pathis handed to string concatenation instead of being used as its path (File.dirname(Pathname.new("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/p/q"))only worked by coincidence ofPathname#to_s).The four helpers now share two prelude twins of the C:
Kernel#__str_value—StringValue(rb_str_to_str), including its two distinct messages:no implicit conversion of X into Stringwhen nothing converts, andcan't convert X to String (X#to_str gives Y)when the conversion answers a non-String.Kernel#__file_path—rb_get_path: a String is the path, otherwiseto_pathand thento_strare consulted (to_pathwins when both answer), thenrb_get_path_check_convert's NUL rule.Both sit beside
__check_convert/__conv_callable?, therb_check_funcalltwins they are built on, so a userrespond_to?is honoured and ato_pathserved bymethod_missingcounts.File.basenameconverts its suffix before the path and only when a second argument was passed, asrb_file_s_basenamedoes: an explicitnilsuffix is theTypeErrorCRuby raises rather than "no suffix", a bad suffix is reported before a bad path, and an out-of-range arity gets CRuby's1..2message.File.joinkeeps its existing separator behaviour (that is #58's change, in flight over these same methods) and now converts each part the wayrb_file_joindoes: a String part is checked withStringValueCStr, anything else is a path.File.extnamebuilds its last component directly instead of re-enteringbasename, so ato_strthat answers a different string on each call is converted once, as in CRuby.File.dirname(123)"."TypeError: no implicit conversion of Integer into StringFile.basename(123)"123"TypeError: no implicit conversion of Integer into StringFile.join(1, 2)"1/2"TypeError: no implicit conversion of Integer into StringFile.extname(:sym)""TypeError: no implicit conversion of Symbol into StringFile.dirname(nil)"."TypeError: no implicit conversion of nil into StringFile.basename("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/a/b.rb", nil)"b.rb"TypeError: no implicit conversion of nil into StringFile.dirname(obj serving to_path)to_sto_pathanswerFile.dirname(obj serving only to_str)to_sto_stranswerFile.dirname(obj serving both)to_sto_pathanswerFile.dirname(obj whose to_path answers 42)to_sTypeError: no implicit conversion of Integer into StringFile.dirname(obj whose to_str answers nil)to_sTypeError: can't convert X to String (X#to_str gives NilClass)File.dirname("a\0b")"."ArgumentError: path name contains null byteFile.join("a", "x\0y")ArgumentError: string contains null byteOne deliberate non-change: an Array part of
File.joinstill gates with a namedUnsupported. CRuby flattens arrays (and raisesrecursive arrayfor a self-referential one); that is the separate decision #57 called out, so answeringTypeErrorthere — orArray#to_s, which is what happened before — would be a wrong answer rather than a gate.Test plan
New regression case —
difftest/corpus/regressions/file-path-type-errors.rb(39 rows, sidecarstatus: fixed): the accept/reject boundary (String,to_path, only-to_str, both,to_pathviamethod_missing, arespond_to?that deniesto_path,Pathname,Stringsubclass, nil/true/false/Symbol/Array), both conversion messages, the suffix ordering, the arity, and both NUL messages.difftest run --tier regressionsreports itheld.Run on this machine (
makeis not installed here, so each target's commands were run directly):difftest run --tier regressions --sut lean— 221 cases: 220held, 0regressed, 1 pre-existing gate (sorbet-hash-gate.rb). The fiverequire "sorbet-runtime"cases only pass when that gem is on CRuby's load path; it cannot be installed here (the toolchain Ruby'spsychneedslibyaml, absent, and there is no compiler), so it was unpacked fromsorbet-runtime-0.6.13405.gemand passed in viaRUBYOPT=-I….difftest run --tier 0 --sut lean+scripts/check-conformance.shover MRI bootstraptest: 1287 ran, 1087 agree, 194 unsupported, 6 not compared, 0 disagree →CONFORMANCE OK, exactly the recorded baseline, so nothing is re-baselined.difftest replay corpus/tier3: 57 agree, 13 unsupported, 0 disagreements.difftestunit tests: 183 passed, 6 skipped.PreludeJson.lean,Prelude.lean,Unicode.leanand the audited checker are all fresh, andCRubyNames.leanregenerates byte-identical too (RUBY_LEAN_GEN_PINNED=1plus the unpackedsorbet-runtimethat its generator needs).lake buildinbooks/after regeneratingRubyCore/Generated/BootedHeap.lean: 1399 targets, success — includingBooks/Metatheory/Heap/BootedHeap.lean, which proves the new boot heap equals the model's boot.books/scripts/check-metatheory.sh: OK — the booted heap satisfies the assumptions the proofs make about it, and no theorem rests on anything beyondpropext,Classical.choice,Quot.sound.books/scripts/check-soundness.sh:SOUNDNESS CHECK PASSED, including stage 6 —corpus/accepted.txtunchanged, so the checker's verdicts on the corpus did not move. (Sorbet came from the unpackedsorbet-static-0.6.13405-x86_64-linux.gem.)python3 scripts/check-not-weakened.py:CHECKS NOT WEAKENED.Pathname, amethod_missing-servedto_path, arespond_to?that deniesto_path, aStringsubclass, and binary strings) and a 40-row pure-path matrix, compared row by row with CRuby 4.0.5. Every row agrees except the ones Fix File.dirname/extname/join to match CRuby (separator and dotfile edge cases) #58 owns:File.dirname("///")("//"vs"/") andFile.join("a//", "b")("a///b"vs"a//b").Not run:
make checkend-to-end (nomakeon this machine),make deps(needs a compiler and system libraries), andmake comparator(fetches and buildsleanprover/comparator).Note on the branch
This replaces the earlier version of this branch, which gated every
to_path/to_strobject instead of converting it. Gating is not what CRuby does at that boundary —File.dirname(Pathname.new(...))is ordinary Ruby and answered correctly before the first version, so that version moved a correct answer to a gate. This version models the conversion instead, and gates only the join array case above.ruby-lean/prelude/prelude.rb'sdirname/extname/joinare also touched by #58; the two will need a rebase whichever lands second.Authorship
Authored by Jiro, an autonomous agent (Nori). The code, the regression case and this description were written by the agent.