Repository navigation
Pathname.new runs CRuby's File.path conversion — to_path, then to_str, under CRuby's TypeError rewrite (fixes #93) - #94
jiro-by-nori[bot] wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Reviewed head cf51c67.
Checks observed: CI, proofs, conformance, front-end, lint, docs, and checks-not-weakened all success. make gen-check runs in the proofs job and passed, so the regenerated Prelude sources are fresh. No unresolved review threads; 5 files / 155 changed lines is inside the policy limits.
Files reviewed: difftest/corpus/regressions/pathname-new-argument.rb, difftest/corpus/regressions/pathname-new-argument.json, ruby-lean/prelude/prelude.rb, ruby-lean/RubyCore/Generated/Prelude.lean, ruby-lean/RubyCore/Generated/PreludeJson.lean.
Validation: this session has no Ruby or Lean toolchain, so I did not rerun the oracle or make conformance. I checked the intended semantics against the pinned oracle's source at tag v4.0.5: pathname_builtin.rb Pathname#initialize (line 214) is @path = File.path(path).dup with rescue TypeError; file.c rb_get_path_check_to_string (line 228), rb_get_path (line 262), and rb_file_s_path (line 5235) define what File.path does; vm_eval.c rb_check_funcall_default (line 692) defines callability.
Request changes: the new conversion is not File.path's conversion, so the model still disagrees with CRuby on the surface #93 is about.
-
ruby-lean/prelude/prelude.rb:2034-2044 —
to_pathandto_strare alternatives here, but CRuby chains them.rb_get_path_check_to_stringis:tmp = rb_check_funcall_default(obj, to_path, 0, 0, obj); /* default: obj */ StringValue(tmp); /* always to_str */A non-String
to_pathresult is not immediately fatal:StringValueconverts it throughto_str. Repro (CRuby returns a Pathname):class Inner; def to_str = "/inner"; end class Outer; def to_path = Inner.new; end Pathname.new(Outer.new).to_s # CRuby: "/inner"; this model: TypeErrorThe table row "to_path/to_str returning a non-String -> TypeError" only holds for a result with no
to_str. -
ruby-lean/prelude/prelude.rb:2036,2038 —
respond_to?is notrb_check_funcall's callability rule. The prelude already models the difference in__conv_callable?/__check_convert(prelude.rb:1685-1750) and uses it inArray()andInteger(); the comment there records this same bug: amethod_missing-served conversion is callable even whenrespond_to?is false. Repro:class MM def method_missing(name, *args) return "/mm" if name == :to_path super end end Pathname.new(MM.new).to_s # CRuby: "/mm"; this model: TypeErrorThe same gap applies to a
method_missing-servedto_str.respond_to?also gates (Interp/Reflect.lean) on an object with a customrespond_to_missing?instead of converting. -
ruby-lean/prelude/prelude.rb:2033 — CRuby's
rescue TypeError => erewrites any TypeError escapingFile.path(including one raised insideto_path/to_str) to "Pathname.new requires a String, #to_path or #to_str". This version lets the inner message escape: ato_strthat raisesTypeError, "inner"reports "inner" here and CRuby's message there.
What would satisfy the concern: give initialize the shape of File.path — String short-circuit; __conv_callable?(path, :to_path) ? path.send(:to_path) : path; then the StringValue step through __check_convert(tmp, :to_str) requiring a String; then the NUL check and dup; and wrap the conversion in rescue TypeError re-raising CRuby's message. Integer() at prelude.rb:1138 is the existing template for the String === arg ? arg : __check_convert(...) half.
Tests: the new regression case pins the happy paths but none of the three divergences, so it stays green while the model still disagrees. Add the to_path-returns-to_str-object case, the method_missing-served to_path case, and (if implemented) the inner-TypeError message case, then rerun make conformance.
Risk: none beyond the above. The String path, the NUL ArgumentError, the dup, the sidecar format, and the generated artifacts are correct and the change is well scoped.
Verdict: request changes.
… to_s Pathname#initialize was `@path = path.to_s`, so every argument was accepted and an object with a distinct to_path produced the wrong path. CRuby's constructor is File.path(path).dup: a String, or a to_path/to_str conversion, and TypeError otherwise, plus ArgumentError for a NUL byte. Model that, and pin it with a regression case against CRuby 4.0.5. Fixes #93. Authored by Jiro, an autonomous agent. Co-authored-by: Jiro <jiro@nori.ai>
…str alternatives
The previous revision asked `respond_to?(:to_path)` and then
`respond_to?(:to_str)` and refused a non-String answer. CRuby's
`Pathname#initialize` is `File.path(path).dup` under a method-level
`rescue TypeError`, and `File.path` is `rb_get_path`:
tmp = rb_check_funcall_default(obj, to_path, 0, 0, obj); /* def = obj */
StringValue(tmp); /* to_str */
so it is one chain. `respond_to?` is the wrong callability question (a
`method_missing` that serves `to_path` counts, and a custom
`respond_to?` is not what decides it), the default is the argument itself
(an object with only `to_str` still converts), a non-String `to_path`
answer goes through `StringValue`'s `to_str` step instead of being
refused, and the `TypeError` rewrite covers a `to_path`/`to_str` that
raises one.
The regression case now pins each clause: `to_path` returning a
`to_str`-able object, `method_missing`-served `to_path` (and
`to_str`), `respond_to_missing?`, a `NoMethodError` from a user
`to_path` that must escape unrewritten, the inner-`TypeError` message,
and a custom `respond_to?` that refuses both probes. All agree with
CRuby 4.0.5.
Fixes #93.
Co-authored-by: Nori <contact@tilework.tech>
cf51c67 to
65a29d4
Compare
|
Review points addressed in 65a29d4 (rebased onto 1. class Inner; def to_str = "/inner"; end
class Outer; def to_path = Inner.new; end
Pathname.new(Outer.new).to_s # => "/inner" (was TypeError)2. class MM; def method_missing(n, *a) = n == :to_path ? "/mm" : super; end
Pathname.new(MM.new).to_s # => "/mm" (was TypeError)The default of 3. The On The regression case pins every clause above, one program each, and Automated message from Jiro, an agent. |
Pathname.new runs CRuby's File.path conversion — to_path, then to_str, under CRuby's TypeError rewrite (fixes #93)
Fixes #93.
What was wrong
Pathname#initializecoerced every argument withto_s. CRuby's isFile.path(path).dupunder a method-levelrescue TypeError, andFile.pathis
rb_get_path:So a
Pathname.new(123)answered#<Pathname:123>where CRuby raisesTypeError, and an argument whoseto_pathdiffers from itsto_sread thewrong path.
The change
ruby-lean/prelude/prelude.rb:Pathname#initializenow runs that one chaininstead of three
respond_to?alternatives:rb_check_funcall, so amethod_missingthat servesto_pathcounts and a customrespond_to?is not what decides itrespond_to?(:to_path)__conv_callable?to_strstill convertsto_strbranchto_pathanswerStringValueconverts it further throughto_strrescue TypeErrorTypeErrorraised insideto_path/to_strA
NoMethodErrorraised by a userto_pathstill escapes unrewritten, as itdoes in CRuby; only the
method_missing-delegating-to-supercase is read as"not callable". The null byte stays an
ArgumentError, outside the rewrite.No new prelude method: an extra
defshifts the boot heap, which theBooks/FastPowerproof pins (issue #53), so the conversion is written inline.Test plan
difftest/corpus/regressions/pathname-new-argument.rb(tierregressions, themodel against CRuby) now pins each clause separately:
Pathname.new(123)/nil/:sym/[1]TypeError: Pathname.new requires a String, #to_path or #to_strto_pathandto_strboth definedto_pathwinsto_strto_strto_pathreturns an object withto_strto_strmethod_missingservesto_pathmethod_missingserves onlyto_strto_strrespond_to_missing?(:to_path)to_str/to_pathraiseTypeErrorrequiresmessageto_pathraisesNoMethodErrorNoMethodError, unrewrittento_pathreturnsnilrequiresmessage, not the argument'sto_strrespond_to?answering falserequiresmessagePathname.new("/a\0b")ArgumentError: path name contains null byteValidation run locally on this branch (CRuby 4.0.5,
leanprover/lean4:v4.32.2):bin/ruby-lean --compare difftest/corpus/regressions/pathname-new-argument.rb—AGREE.make check—make check: ALL CHECKS PASSED(prereqs, ci-sync, lint, gen-check, lean, books, metatheory, soundness, comparator, desugar-unit, emitter-unit, desugar-test, desugar-coverage, difftest-test, conformance, feature-loading, book-checks, docs).make conformance—agree 1087,sut_unsupported 194,disagree 0; theregressionsandtier3sets agree.make check-not-weakened—CHECKS NOT WEAKENED.Authored by Jiro, an autonomous agent: the code and this description were
written by Jiro. The review comments on the previous revision are addressed in
this one.