Stop REAL_RAT_REDUCE_CONV diverging on rational literals - #2036
Merged
mn200 merged 2 commits intoAug 11, 2026
Conversation
REAL_RAT_DIV_CONV rewrites x / y to x * inv y and back, so on a literal already in lowest terms it returns |- &1 / &3 = &1 / &3: success, no change. computeLib re-evaluates whatever a registered conversion hands back, so CBV_CONV went round that term forever, and REAL_RAT_REDUCE_CONV did not terminate on any term containing a non-integral rational literal. The compset already guarded one entry, REAL_RAT_NEG_CONV, with CHANGED_CONV. The guard belongs on every entry rather than on whichever operator's no-op was noticed first, so real_rat_compset now adds them through one function that applies it. A registered conversion can report no progress in two ways: return a reflexive theorem, or raise UNCHANGED (as ALL_CONV, hence TRY_CONV, does). Both are fatal -- the first loops, the second escapes CBV_CONV entirely, because reduce_cst catches only HOL_ERR when deciding to move on to the next rule. Conv.CHANGED_CONV covers both, so it needs no QCHANGED_CONV around it, and QCHANGED_CONV in its place would be a regression: that one catches only the raise and lets the diverging mode through. A comment at the site records this so the question is not reopened. Found via a normaliser that runs REAL_RAT_REDUCE_CONV after REAL_POLY_CONV: the latter turns x / 3 into 1 / 3 * x, so any real goal containing a division by a non-unit literal hung outright. SOSLib composes the same two conversions in that order, at src/real/SOSLib.sml:2133. src/real/selftest.sml gains a test that the conversion terminates on &1 / &3 * x, and a companion pinning that &1 / &3 + &1 / &6 still reduces to &1 / &2, so the guard cannot be satisfied by disabling reduction. This is the instance half of HOL-Theorem-Prover#2034. The underlying computeLib mechanism -- reduce_cst counting a reflexive result as progress -- and the add_conv documentation that never stated the contract are deliberately left alone here.
lukaszcz
force-pushed
the
real-rat-reduce-termination
branch
from
August 10, 2026 09:36
4dbc756 to
7f856fb
Compare
Member
|
Both new selftests should use |
Contributor
Author
Fixed |
Member
|
Thanks! |
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.
The bug
REAL_RAT_DIV_CONVrewritesx / ytox * inv yand back, so on aliteral already in lowest terms it returns
|- &1 / &3 = &1 / &3—success, no change.
computeLibre-evaluates whatever a registeredconversion hands back, so
CBV_CONVwent round that term forever:REAL_RAT_REDUCE_CONVdid not terminate on any term containing anon-integral rational literal.
The fix
real_rat_compsetalready guarded one entry —REAL_RAT_NEG_CONV—with
CHANGED_CONV. Several of the others are no-ops on analready-reduced argument in the same way, so the guard belongs on every
entry rather than on whichever operator's no-op was noticed first. The
compset now adds all of them through a single
add_reduce_convthatapplies it.
A registered conversion can report no progress in two ways: return a
reflexive theorem, or raise
UNCHANGED(asALL_CONV, henceTRY_CONV, does). Both are fatal — the first loops, the second escapesCBV_CONVentirely, becausereduce_cstcatches onlyHOL_ERRwhendeciding to move on to the next rule.
Conv.CHANGED_CONVcovers both,so it needs no
QCHANGED_CONVaround it;QCHANGED_CONVin its placewould be a regression, since it catches only the raise and lets the
diverging mode through. A comment at the site records this so the
question is not reopened.
Tests
src/real/selftest.smlgains a test that the conversion terminates on&1 / &3 * x, plus a companion pinning that&1 / &3 + &1 / &6stillreduces to
&1 / &2— so the termination guard cannot be satisfied bysimply disabling reduction.
Scope
This is the instance half of #2034. The underlying
computeLibmechanism —
reduce_cstcounting a reflexive result as progress — andthe
add_convdocumentation that never stated the contract aredeliberately left alone here.