[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#21478coqbot-app[bot] merged 11 commits intorocq-prover:masterfrom
Conversation
|
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.
|
Following last Rocq call, this is now split in two:
@gares CI green this now seems in a mergeable state |
gares
left a comment
There was a problem hiding this comment.
I think that the PR now only contains what was discussed at the rocq-call, @SkySkimmer @andres-erbsen please amend your requests for changes.
I'm a little worried about the warning. I don't think it is urgent, since the use case this PR makes possible is to load SSR/MC inside non SSR/MC context. You don't really care if the the MC code uses rewrite instead of rw. The warning is there, correct me if I'm wrong, to push MC users to port their scripts. IMO this can wait. Also, the warning should really come with a quickfix, so that the porting can be done automatically.
|
It's the common policy here to emit deprecation warning as soon as the replacement is available, since warnings can easily be silenced. No strong opinion but it would be strange to make an exception here. |
SkySkimmer
left a comment
There was a problem hiding this comment.
remove the test suite makefile change
I'm also not very convinced about allowing DOUBLE LOW LINE in idents
Using double-low-line rather than underscore, i.e., `‗foo‗` instead of `_foo_`, will cause less conflicts and is not an issue since, by definition, we never type these identifiers by hand.
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.
More generally, all ssreflect users.
No strong opinion, but we usually introduce warnings with the replacement to let user know, they can then silence it until they can handle it. Doing it later means we have to remember introducing it latter, users may discover the change it later (probably not an issue here), the difference between the versions the replacement was introduced and the version the deprecation warning was introduced may make it harder to track/handle for users (particularly if this one differs whereas others usually don't).
done
@SkySkimmer done |
Note that this is for ssreflect reserved identifiers (typically the ones introduced by |
|
@gares CI green |
|
@andres-erbsen please check the updated PR. If I hear nothing I'll merge tomorrow. |
|
@coqbot merge now |
Since this was the major cause of conflict with legacy tactics, ssreflect could now be loaded in the Prelude (in a further PR, c.f. #21776 ). 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.