Repository navigation
Kernel#Integer probes in rb_convert_to_integer's order, and honours its failure arms (fixes #158) - #160
Open
jiro-by-nori[bot] wants to merge 1 commit into
Open
Kernel#Integer probes in rb_convert_to_integer's order, and honours its failure arms (fixes #158)#160jiro-by-nori[bot] wants to merge 1 commit into
Kernel#Integer probes in rb_convert_to_integer's order, and honours its failure arms (fixes #158)#160jiro-by-nori[bot] wants to merge 1 commit into
Conversation
…rs its failure arms
`__integer` asked `to_str` before everything else, in both of CRuby's forms, so an
object defining both conversions converted through its String: `Integer(Both.new)`
answered 42 against CRuby's 7, and an object whose `to_str` answers an unparseable
String raised `ArgumentError: invalid value for Integer()` where CRuby answers from
`to_int`. `rb_convert_to_integer` (object.c) has two arms: with a base it is
`rb_check_string_type` alone, and every non-String is refused ("base specified for
non string value"); without one it is `rb_check_to_int`, then `rb_check_string_type`,
then `to_i`. The probes now run in that order, and a String is still type-tested
before either.
Three more rules of that function were wrong on the same lines and are fixed with
it, because the reordering walks straight through them:
* `rb_protect(rb_check_to_int)` swallows whatever the `to_int` probe raises — of any
class, not just StandardError — and converts on. The model let it escape, so
`Integer(obj)` with a raising `to_int` and a `to_i` raised instead of answering,
and a `method_missing` that ends in `super` escaped its `NoMethodError` from the
probe. Without this the reordering would have regressed a `method_missing` that
serves only `to_str`.
* `exception: false` is `raise_exception`, not a `rescue` around the whole
conversion: a bad literal is nil, but `rb_check_string_type` and the radix check
raise either way, and in the non-raising form the `to_i` probe is protected too
(`Integer(obj_with_raising_to_i, exception: false)` is nil). Implemented by
threading the flag rather than by rescuing, so the arms cannot swallow each
other's errors. `__parse_int` and `__to_integer` take it.
* `invalid_integer` names the String being parsed, not the argument: a `to_str`
that answers `"zz"` is `invalid value for Integer(): "zz"`, which `__parse_int`
could not say because it was handed the original object.
`String.try_convert` does the `rb_check_string_type` conversion here rather than the
lenient `__check_convert`, because a `to_str` that answers a non-String is a
`TypeError` naming `to_str` — from `Integer()` as much as from `String.try_convert`.
`difftest/corpus/regressions/integer-conversion-order.{rb,json}` pins the whole
table against CRuby 4.0.5, status `fixed`: the two probe orders, both
`method_missing`-free conversion objects, the raising-`to_int` arms, the
`exception: false` matrix (nil, raise-either-way, protected `to_i`), the message
naming, and the unchanged base-0 grammar.
Validation: `make conformance` (ran 1287, agree 1087, sut_unsupported 194,
not_compared 6, disagree 0 — baseline holds; the regressions tier then held 223
cases including the new one, and tier 3 agrees), `make books` (1405 jobs),
`make metatheory`, `make soundness` (SOUNDNESS CHECK PASSED), `make gen-check`,
`make difftest-test` (201 passed, coverage baseline holds), `make feature-loading`,
`make book-checks` (72/72, plus LoopSum), `make ci-sync`, `make lint` by hand
(rubocop 1.92.0, ruff 0.17.0, shellcheck, actionlint: clean) and
`make check-not-weakened` (CHECKS NOT WEAKENED).
Not run here: `make comparator` (fetches and rebuilds leanprover/comparator; no
theorem statement changes and `make books` builds every book), `make docs` (mkdocs
is not installed and no documented behaviour changed), and `make desugar-unit` /
`make emitter-unit` (their gems sit behind `json`, whose native extension this
sandbox has no C compiler for; no desugarer or emitter file changes).
Authored by Jiro, an autonomous agent (Nori sessions), on behalf of @sam-xif.
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.
What was wrong
ruby-lean/prelude/prelude.rb'sKernel#__integeraskedto_strbefore everythingelse, in both of CRuby's forms, so an object defining both conversions converted
through its String:
Integer(Both.new)whereto_intis 7,to_stris"42"427Integer(BothBad.new)whereto_stris"zz"ArgumentError: invalid value for Integer(): #<BothBad:0x…>7Integer(BothBad.new, exception: false)nil7rb_convert_to_integer(object.c) has two arms: with a base it isrb_check_string_typealone, and a non-String is refused ("base specified for nonstring value"); without one it is
rb_check_to_int, thenrb_check_string_type,then
to_i. The model's order was "String first" in both.The change
ruby-lean/prelude/prelude.rb:rb_convert_to_integer's order. A String (including a subclassthat defines
to_int) is still type-tested first, as CRuby type-tests beforecalling anything.
Integer(StrBad.new)names the converted String (invalid value for Integer(): "zz"), not the object:__parse_intis handed the String being parsed.String.try_convertdoes therb_check_string_typeconversion instead of thelenient
__check_convert, so ato_strthat answers a non-String is CRuby'sTypeError: can't convert C to String (C#to_str gives Integer).Three more rules of the same function were wrong on the same lines, and the
reordering walks straight through them, so they are fixed in the same commit:
rb_protect(rb_check_to_int)swallows theto_intprobe's exception — of anyclass, not just
StandardError— and converts on. SoInteger(obj)with araising
to_intand ato_ianswers fromto_iinstead of raising. Withoutthis, the new order would have regressed a
method_missingthat serves onlyto_str.exception: falseisraise_exception, not arescuearound the wholeconversion. A bad literal is nil, but
rb_check_string_typeand the radix checkraise either way, and in the non-raising form the
to_iprobe is protected too(
Integer(obj_with_raising_to_i, exception: false)is nil). It is threadedthrough
__integer/__to_integer/__parse_intrather than rescued, so the armscannot swallow each other's errors.
invalid_integer's naming, as above.Integer(Float::NAN)/Integer(Float::INFINITY)still decline (FloatDomainErroris not a modeled class), except under
exception: false, where CRuby answers niland so the model now does too.
The regression case
difftest/corpus/regressions/integer-conversion-order.{rb,json}, statusfixed,pins the whole table against CRuby 4.0.5: the two probe orders, the raising-
to_intarms, the
exception: falsematrix (nil, raise-either-way, protectedto_i), themessage naming, and the unchanged base-0 grammar plus base rejection.
Test plan
Every command below was run in this session against CRuby 4.0.5 (
ruby 4.0.5p0) andLean
v4.32.2, on a clean clone ofmainwith this branch applied.make conformanceran 1287,agree 1087(= baseline),sut_unsupported 194(≤194),not_compared 6(≤6),disagree 0;CONFORMANCE OK; regressions and tier 3 agreecd difftest && uv run difftest run --tier regressions --sut leanheld 223(including the new case),gated 1(pre-existing),failures 0make booksBuild completed successfully (1405 jobs)make metatheorypropext,Classical.choice,Quot.sound; the booted heap satisfies the assumptionsmake soundnessSOUNDNESS CHECK PASSEDmake gen-checkPreludeJson.lean/Prelude.leanare in this PR)make difftest-test201 passed; coverageline 73.19% / branch 68.61%—COVERAGE OKmake feature-loadingAGREElinesmake book-checks72/72 inputs: CRuby, the model and the theorem agree; LoopSum agreesmake ci-syncOK: CI runs every target of make checkmake check-not-weakenedCHECKS NOT WEAKENEDrubocop1.92.0,ruff0.17.0,shellcheck,actionlint44 files inspected, no offenses detectedBeyond the pinned case, before/after probes of ~90 shapes through
bin/ruby-lean --compare: 34Integerprogram shapes (both probe orders, bothexception:modes, base 0/16/8/99/-2,to_int/to_str/to_ianswering non-Integer,answering nil, and raising) now print identical output and end identically; before
the change the first one disagreed on 8 of its 12 lines.
Known, pre-existing, and not addressed here (all reproduced before this change,
none made worse by it): a
method_missingthat defers withsuperstill escapesfrom
__conv_callable?(that is #152 / #157),Integer(x, nil)does not raiseCRuby's
no implicit conversion from nil to integer(the model cannot tell theargument from an omitted base), and a non-boolean
exception:is not refused.Not run here:
make comparator(fetches and rebuildsleanprover/comparator; notheorem statement changes and
make booksbuilds every book),make docs(mkdocsis not installed; no documented behaviour changed), and
make desugar-unit/make emitter-unit(their gems sit behindjson, whose native extension thissandbox has no C compiler for; no desugarer or emitter file changed).
Authored by Jiro, an autonomous coding agent, on behalf of @sam-xif. The code,
the regression case and this description were written by an agent; every result
above was produced by running the commands shown in this session.