[ssreflect] Use rw instead of rewrite to avoid conflicts with legacy tactic#21478
[ssreflect] Use rw instead of rewrite to avoid conflicts with legacy tactic#21478proux01 wants to merge 3 commits intorocq-prover:masterfrom
Conversation
|
Based on the overlays, this change is doing more than renaming ssreflect |
|
Being able to use ssreflect without conflicts. The major source of conflict was the |
|
While, on the long run, I strongly believe tactics should be indexed through Ltac2, it is far away, and in the meantime it would be indeed great if ssreflect could be loaded by default. A few questions after a quick review of the code and docs:
|
SkySkimmer
left a comment
There was a problem hiding this comment.
This PR seems to be doing multiple separate things (as indicated by having 3 changelogs) and should probably be split.
also whatever made the stdlib overlay needed is not described in the changelog and doesn't even seem to be desired, the overlay should be reverted and the change fixed.
Not sure I understand the question(s)?
C.f. the related changelog entry and top message comment about what could be done in the future. So currently, this is only adding the
According to #21425 (comment) it should be done "the next time it breaks", so either in the current PR or any other that get merged before it.
It could relatively easily be split in three nonindependent PRs (one per changelog entry), but not sure it's worth it (no strong opinion, I'd rather let the assignee decide).
In some earlier development stage of the current PR, I considered forbidding the use of |
|
Please do remove the unneeded changes from overlays (or at least a few representative ones) so that we can have illustrative examples of what the actually necessary downstream changes are. I don't have opinions on what ssreflect rewrite is called, but making |
andres-erbsen
left a comment
There was a problem hiding this comment.
Finally realizing the true extent of the change here, I am deeply skeptical of where this is going. Over years, ssreflect code has earned a reputation of being dense and essentially incomprehensible to outsiders and brittle outside the narrow niche of coding style used in mathcomp. While there are certainly valuable bits to be adopted from there, the way towards it would be targeted feature-by-feature design review with user-experience considerations under special focus.
Even adding a global 2-letter shorthand for ssreflect's rewrite seems premature given that unifall did not land, and I ran into serious issues within the first hour of trying ssr rewrite out in a relatively undemanding proof. I am still in favor of resolving the naming collision, but ssrewrite or something similarly evocative of the use case of the tactic would be more appropriate.
| Require Export Corelib.Init.Tactics. | ||
| Require Export Corelib.Init.Tauto. | ||
| Require Export Corelib.Init.Sumbool. | ||
| Require Export ssreflect_rw. |
There was a problem hiding this comment.
The referenced file defines a number of global notations, Ltacs, definitions, lemmas, and hints, not just ssreflect rewrite. We should not force these upon all corelib users without much more extensive review, even if we can get CI to pass with them.
|
I guess the point here is to provide the ability to load at the same time ssreflect and libraries which did not load it, and in order to do that, at least the grammar from ssreflect must be taken into account in order not to create conflicts during a late |
|
Let me take a step back: I think indeed this is not a regular PR but a strategic one. Let's start with some facts.
Stuff that have been tried before and are not a good thing IMO.
In my opinion, we should integrate ssreflect as such to resolve compatibility issues right now, with minimal impact to users: I do not think here is the time and place to change its syntax or semantics as an answer to tastes and distastes of the participants to this discussion (this is a more general and valid question, but solves a distinct issue). The fact that this PR does not pass CI and calls for overlay reflects the Require time conflicts, so that we can solve them here once and for all, for everyone wanting to mix mathcomp or other ssreflect based developments with almost anything. |
|
I agree that the split of the ecosystem is not ideal and we ought to do something about it. I would be happy to discuss strategy if one was proposed. I am not sure what you mean by Require-time conflicts -- are there any? I've been able to Require and wrap ssreflect in coqutil, for example, without needing the overlays here. The change I highlighted above adds controversial ssreflect constructs to by-default-Imported prelude. This does not seem necessary for fixing the compatibility issues, but is interesting as an experiment for investigating what the incompatibilities are -- or is that what you meant by "strategic PR"? I would like to improve compatibility, but the path towards that must take into account desires and constraints of all users, mathcomp and otherwise. Obviously porting large swaths of code is costly, and shipping barely-tested constructs is not right. But I don't think we'd make any friends by forcing ssreflect as-is upon the supermajority of users who have not chosen it -- at the very least we should leave them the option whether to Import it or not. At a higher level, my preferred path would be to
|
|
I globally agree with you @andres-erbsen. By the way I think I got mistaken when I said As for ssreflect evaluation, I fully agree that it should be tested on non ssr contexts to see what might be missing and improved in a backward compatible way. I'm sorry to see your issues with ssr rewrite taking an awful amount of time (I usually have the exact opposite experience so I was surprised) and I am eager to know where this slowdown comes from. However I do think ssreflect syntax should not be "evaluated" but that there should be a second one, less perl-ish available to all, and that ideally it should resemble current syntax e.g. |
|
PS:
What I would like to reach is the point where |
|
To be discussed in a future Call: https://github.com/rocq-prover/rocq/wiki/Rocq-Call-2026-03-17 |
SkySkimmer
left a comment
There was a problem hiding this comment.
not convinced we should be adding this stuff to the prelude
also even if we decide we should, it doesn't match the PR title "[ssreflect] use rw instead of rewrite"
| mk_lookup_table_from_unicode_tables_for IdentSep | ||
| [ | ||
| single 0x005F; (* Underscore. *) | ||
| single 0x2017; (* Double low line. *) |
There was a problem hiding this comment.
Ssreflect was using _foo_ for reserved identifiers (the ones introduced by move=> ?) which forbids explicitly using any such identifier. To avoid conflicts this changes it to ‗foo‗.
|
|
||
| Axiom proof_admitted : False. | ||
| #[export] Hint Extern 0 => case proof_admitted : unused. | ||
| #[export] Hint Extern 0 => (case proof_admitted) : unused. |
There was a problem hiding this comment.
case E: foo is valid ssreflect syntax (c.f., https://rocq-prover.org/doc/V9.1.0/refman/proof-engine/ssreflect-proof-language.html#generation-of-equations ), the parenthesis fixes the conflict.
According to rocq-prover#21425 (comment)
Since this was the major cause of conflict with legacy tactics, ssreflect can now be loaded in the Prelude. For backward compatibility From Corelib Require Import ssreflect. still loads a 'rewrite' wrapper to 'rw'. This compatibility layer also includes the `if _ is _ then _ else _` and the `if _ isn't _ then _ else _` notations.
Since this was the major cause of conflict with legacy tactics, ssreflect can now be loaded in the Prelude. For backward compatibility
still loads a
rewritewrapper torw.The
theories/Corelib/ssrdir still contains:ssrfun.vandssrbool.vthat should go back in mathcomp (in a further PR)ssreflect.vwithrewritetactic as alias torw(to be removed in a few releases)theof T & ... & Tnotation for anonymous binders (c.f., Add "& T" syntax for anonymous binder "(_ : T)" and "of _ & _ & _" syntax for constructors #21611 ) (merged)if <term> is <pattern> then <term> else <term>andif <term> isn't <pattern> then <term> else <term>notations (c.f., Add "if <term> is <pattern> then _ else _" syntax #21609 )if ... then ... elsenotations: in the long run we should maybe deprecate the specificif ... then ... elsefor inductives with two constructors (that is error prone, since it depends from the order of the constructors) with a quickfix toward the newif g is first_constructor then ... elsevariant and restrictif ... then ... elseto booleans (or anything coercible to bool)Documented any new / changed user messages.make doc_gram_rsts.Overlays (to be merged before the current PR)
Most overlays are handling the new
ofandiskeywords and are only a couple lines long.rw->rew)//->/ /)