replay: redirect datatype constructors in match bodies too - #1089
Open
MavenRain wants to merge 1 commit into
Open
replay: redirect datatype constructors in match bodies too#1089MavenRain wants to merge 1 commit into
MavenRain wants to merge 1 commit into
Conversation
When a datatype's type is overridden while cloning, replay_tyd redirects the source constructors to the target's constructors with EcSubst.add_opdef. That map is consulted only where a constructor occurs as an operator expression, so the constructors stored as bare paths in the branches of a match body (OP_Fix) were left pointing at the source datatype. The operator-compatibility check compares those paths for equality, so it raised CoreIncompatible, which Compatible.for_operator does not convert and which therefore surfaced as an anomaly. Record the redirect in a dedicated map, sb_ctor, and consult it where opb_ctor is substituted, falling back to the previous behaviour on a miss. A sb_path entry would not do: _subst_path recurses into qualifiers, so keying it on a constructor path would also rewrite the paths that constructor qualifies, and constructor names share a namespace with sub-theories, types and modules. Adds tests/clone-datatype-match.ec. Fixes EasyCrypt#1046. Signed-off-by: Onyeka Obi <softwareengineerasaservant@isurvivable.cv>
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.
Problem
Cloning a theory while overriding a datatype's type crashes when an operator in that theory
pattern-matches on the datatype's constructors:
Both override modes (
type ATy = ...andtype ATy <- ...) are affected. Fixes #1046.Cause
When a datatype's type is overridden, the source datatype's constructors are not re-created in the
clone, so references to them have to be redirected to the target datatype's constructors. #1045
does that with
EcSubst.add_opdef, which populates the definition map.That map is consulted only where a constructor occurs as an operator expression:
subst_exprchecks
has_opdefatsrc/ecSubst.ml:335,342andsubst_format:517,524. A match body(
OP_Fix) does not store its branch constructors as expressions. It stores each one as a barepath in
opb_ctor, which is rewritten through the path map instead(
src/ecSubst.ml:934-935, viasubst_pathat:74).So for a match operator the branch constructors were left pointing at the source datatype, and
Compatible.for_opfix, which compares them withEcPath.p_equal(
src/ecTheoryReplay.ml:213), raisedCoreIncompatible.Compatible.for_operatorcatches onlyFailure _, so the exception escaped as an anomaly rather than as a clone error.Fix
Add a dedicated constructor map to
EcSubst,sb_ctor, consulted at exactly one place: where amatch branch's
opb_ctorpath is substituted.replay_tydrecords the redirect there alongsidethe existing
add_opdefentry, in the same fold. On a miss the branch falls through to theprevious
subst_pathbehaviour unchanged, so nothing outside an overridden datatype's matchbranches is affected.
A separate map rather than a
sb_pathentry, because_subst_pathrecurses into qualifiers(
src/ecSubst.ml:65-72): an entry keyed onThy.CAwould also rewrite every path thatThy.CAqualifies. Constructor names share a namespace with sub-theories, types and modules, so a sibling
that happens to be named after a constructor would be silently re-rooted into the target datatype's
prefix, changing what replayed definitions denote while their already-proved axioms are re-admitted
unchanged.
sb_ctoris keyed exactly and read only foropb_ctor, so it cannot reach any otherposition.
Testing
New
tests/clone-datatype-match.ec, covering: a match operator overridden through both overridemodes; a clone that overrides only the type, with a lemma checking that the replayed match operator
still reduces on the target's constructors; and a recursive match operator, which exercises the
recursive occurrence alongside the constructor redirect.
tests/clone-datatype-alias.ec(the Fix anomaly when alias-cloning a datatype type #1045 / Anomaly caused by algebraic datatype shenanigans #989 regression test) still passes.operator named after a constructor of the overridden datatype, plus a
renameclause matching aconstructor name) all behave exactly as on
main.Out of scope, noted for the record
Aliasing the type of a recursive datatype (
type RTy = R1.RTywhere a constructor takes anRTyargument) is rejected by the type-compatibility check withThis behaves identically with and without this patch, including when no operator is overridden at
all, so it is pre-existing and independent. The cause looks different in kind: it is the
constructor argument that mentions the datatype itself that is not redirected in alias mode,
whereas inline mode rewrites it through
add_tydef. I left it alone rather than widen the scopehere, but I am happy to look at it separately if that is useful.
Relatedly,
Compatible.for_operatorhas noCoreIncompatiblehandler, unlikefor_tydecl(
src/ecTheoryReplay.ml:208). That is why this bug presented as an anomaly instead of a cloneerror, and any other operator-body mismatch reaching
checkwould do the same. Adding| CoreIncompatible -> raise (Incompatible OpBody)would route it throughCE_OpIncompatible. Ihave not included it here since it changes unrelated error paths, but say the word and I will add
it to this PR or a follow-up.
Drafted with AI assistance; the diagnosis, the code, and every claim above were reviewed and
tested by me against a local build.