Skip to content

fix_IF_NONE_EQUALS_OPTION #1482

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

ordinarymath
Copy link
Contributor

This change removes IF_NONE_EQUALS_OPTION from the srw_ss and adds if_option_eq which has the last 2
conjuncts + 2 more other useful case splits.

@ordinarymath
Copy link
Contributor Author

I'm skeptical of the usefulness of the first 2 rewrites so removing them for now.

@ordinarymath
Copy link
Contributor Author

ordinarymath commented May 4, 2025

It seems that otknl is some how not terminating. Can someone with permission to kill jobs kill it. Also to prevent this from happening again @mn200 can I please get permission to kill jobs as it seems like this is happening frequently enough.

@ordinarymath
Copy link
Contributor Author

Oh it finished running.

This change removes IF_NONE_EQUALS_OPTION from the
srw_ss and adds 2 lemmas.
if_option_eq is the last two conjuncts + 2 more other useful case splits
if_option_neq is added for backward compatiblitity and it
preserves good behaviour of IF_NONE_EQUALS_OPTION.
@ordinarymath ordinarymath force-pushed the fix_IF_NONE_EQUALS_OPTION branch 2 times, most recently from f28957d to 94b3ea9 Compare May 10, 2025 10:05
@ordinarymath ordinarymath requested a review from mn200 May 12, 2025 23:42
@ordinarymath
Copy link
Contributor Author

@mn200 ping on this?

>> Q_TAC (HNF_TAC (“M0':term”, “vs :string list”,
“y' :string”, “args':term list”)) ‘M1'’
“y''' :string”, “args':term list”)) ‘M1'’
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name y' was available here, and it's supposed to correspond to y. In my name design, y is the head variable of M0, and y' is the head variable of M0', which is perfect. Now this bad y''' jump out, because somehow y' and y'' occurred from nowhere but changed simplification rules. Shit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another slightly less obvious change is Overload subterm' = “\X M p r. FST (THE (subterm X M p r))”
does not print as often as
subterm X M p r <> NONE before would be simplified in some cases to.
subterm X M p r = SOME y
and i get
FST y appearing instead

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both NONE and SOME are primitive constants provided by optionTheory. I think it's very rude to automatically rewrite all a <> NONE to a = SOME y with a new, fixed name y introduced. I keep those _ <> NONE terms only because I don't care which SOME y it equals to.

@binghe
Copy link
Member

binghe commented May 22, 2025

I actually have a blacklist of simplification rules in front of my boehmTheory. Some are specific to the related work to lambda-calculus, some are general for all my proofs:

(* These theorems usually give unexpected results, should be applied manually *)
val _ = temp_delsimps [
   "lift_disj_eq", "lift_imp_disj",
   "IN_UNION",     (* |- !s t x. x IN s UNION t <=> x IN s \/ x IN t *)
   "APPEND_ASSOC", (* |- !l1 l2 l3. l1 ++ (l2 ++ l3) = l1 ++ l2 ++ l3 *)
   "SNOC_APPEND"   (* |- !x l. SNOC x l = l ++ [x] *)
];

If this PR were merged, I will later revert the changed proofs by putting if_option_eq and if_option_neq into this list, instead. So I give up, just do whatever you feel like.

@mn200
Copy link
Member

mn200 commented May 22, 2025

As per Zulip discussion, I think the best way forward is to avoid automatic simps that generate (existentially-quantified) names, so that (?y. x = SOME y) gets replaced with x <> NONE (in the option_neq rewrite).

@ordinarymath ordinarymath force-pushed the fix_IF_NONE_EQUALS_OPTION branch 3 times, most recently from 91157dc to 13fa486 Compare May 22, 2025 14:43
@ordinarymath ordinarymath force-pushed the fix_IF_NONE_EQUALS_OPTION branch from 13fa486 to 686f85a Compare May 23, 2025 01:41
@ordinarymath ordinarymath force-pushed the fix_IF_NONE_EQUALS_OPTION branch from 686f85a to 37ed4e8 Compare May 23, 2025 01:44
@mn200
Copy link
Member

mn200 commented May 23, 2025

Thanks for negotiating the long back-and-forth with this PR!

The total impact now looks quite minimal and the failing checks in the latest regression are not your problem so I'm happy to merge.

@mn200 mn200 merged commit 68061e1 into HOL-Theorem-Prover:develop May 23, 2025
2 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants