-
Notifications
You must be signed in to change notification settings - Fork 691
Clarify warning about Let/Variable/Hypothesis outside sections and turn it as error by default #18880
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
Clarify warning about Let/Variable/Hypothesis outside sections and turn it as error by default #18880
Conversation
Cc @herbelin following the discussion in #13445 (comment) |
vernac/comAssumption.ml
Outdated
let warn_context_outside_section = | ||
CWarnings.create ~name:"context-outside-section" | ||
~category:Deprecation.Version.v8_20 | ||
(fun () -> Pp.strbrk "Use of \"Context\" outsdide sections is \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(fun () -> Pp.strbrk "Use of \"Context\" outsdide sections is \ | |
(fun () -> Pp.strbrk "Use of \"Context\" outside sections is \ |
This being said, the question of removing Context
outside of a section is more disputable as the replacement will be a priori more verbose (including possibly requiring an Existing
). Maybe can we find another word.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we'd ultimately like to see Variable/Hypothesis and Context converge, so we'd rather not introduce yet another subtle difference between the two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@herbelin do you think a hypothetical #[instance] Axiom
like #[canonical] Axiom
would work for the use cases you have in mind?
Having Variable/Hypothesis/Context behave more similarly seems valuable to me too, and I wonder whether the eventual desired behavior for these commands outside sections would be to act as if a section had been started in the beginning of the file/module. (I start files with Section __.
a lot...)
942562d
to
6ecf156
Compare
I see one argument against this PR: it makes harder to remove sections around some piece of code, as it forces to change all section declarations around, which is a PITA . What was the problem with the warning? It was a cheap way to allow this. |
Maybe a bit of an overstatement for a simple text replacement. Anyway, this could be considered a desirable feature. Removing sections basically changes hypotheses into asxioms, which is a serious change and you probably don't want it to get unnoticed. |
902b42d
to
31ee78a
Compare
🔴 CI failures at commit 31ee78a without any failure in the test-suite ✔️ Corresponding jobs for the base commit caf0f63 succeeded ❔ Ask me to try to extract minimal test cases that can be added to the test-suite 🏃
|
Adapt to rocq-prover/rocq#18880 This is strictly equivalent but avoids a warning / error.
@SkySkimmer what needs fixing? It seems to me this has been ready for more than three weeks now. |
4788f5d
to
1fe4525
Compare
CI looked broken https://gitlab.inria.fr/coq/coq/-/jobs/4313293 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more suggestions
doc/changelog/08-vernac-commands-and-options/18880-deprecate_let_variable_outside_section.rst
Outdated
Show resolved
Hide resolved
1fe4525
to
5ab802d
Compare
5ab802d
to
a507a9f
Compare
This follows a discussion in rocq-prover#13445 (comment) and conclusion of Coq call https://github.com/coq/coq/wiki/Coq-Call-2024-04-09#notes
a507a9f
to
c2695a8
Compare
Indeed, this has gone through way too many rebases and one of them apparently went wrong. |
I'm a bit afraid of possible complaints about the additional constraints but we will gain in clarity. I'd lean to experiment and see if it is accepted. |
@coqbot merge now |
Thanks |
* Changes for compatibility with 8.19 * Adapt files for the build process * Update README.md * Don't use opam to build and install for now * Adapt dune files to make build with dune possible * Change name field in dune file to Waterproof * Revert to dune language 3.6 * Remove line on theory Ltac2 * Adapt to rocq-prover/rocq#17836 (sort poly) * Revert "Adapt to rocq-prover/rocq#17836 (sort poly)" * Adapt to rocq-prover/rocq#18174 (Clenv.unify takes cv_pb) * Adapt to rocq-prover/rocq#17836 (sort poly) (#28) * Adapt to rocq-prover/rocq#18280 (case relevance outside case info) (#37) * Merge features of version 2.1.1 into coq-master (#46) * [build] Initial port to Dune This was done at the interest of Vincent WENDLING for jsCoq use. The setup is fairly standard, other than excluding the deprecated dir (which doesn't compile) * Update README.md * Update dune file in theories * Change importing Ltac2 modules and build only with dune * Update version numbers * Restore changes file and rename license file * Fix metadata in files * Add template tags * Add @install, minimal dune version, dev-repo to opam file * Added infrastructure for enforcing users to mention use of definitions. * Removed some unnecessary lemma's and corresponding tactics (that were apparently throwing errors anyway). * Restored old lemmas in 'SupAndInf' necessary for keeping limsup file working. To be rewritten in future. * Moved tests in SupAndInf.v file to separate test file. Small bug fix in rewrite rule sup and infimum. * 'We need to show'-tactic now also accepts equivalent goals. Added 'By ...' clause to allow for an additional lemma from which equivalence follows. * Replaced use of 'it suffices to show' by 'we need to show' for unfolding definitions. * Added testcases new behaviour tactic. * Improved feedback given new restricted proof automation. * Updated documentation. * Small change in error message. * Moved 'AutomationFailure' exception type to 'Wateprove' file. * Improved feedback 'ItHolds' for new restricted proof automation. * Replaced custom error shielded goal by standard error that can be caught using Ltac2. * Added 'Since ...' clause as alternative to 'By ...' that accepts statements instead of references. (No 'Since ... we need to show ...' because the 'By'-version is to be removed soon. ) * Removed acceptation equivalent goals from 'We need to show' tactic. * Improved feedback 'By ... we conclude ...' for new restricted automated proof finding. Added 'Since ... we conclude that ...' alternative. * Improved feedback, now it says which part of a chain of (in)equalities it failed to prove. Refactored checking whether additional lemma is actually used: now check is done in 'Waterprove.v', it throws errors with relevant information that other tactics use to display user-readable errors. * 'Contradiction' now tries to find a contradiction to the previous statement. * Moved 'Obtain' tactic to separate file. * Simplified old notation 'Obtain' tactic. Now it automatically tries to destruct previous statement and users no longer need to specify the corresponding property. * Simplified names for hypotheses not labeled by user. * Added 'change' to 'Expand definition', so putting in different dummy variables actually has effect. * Added StateGoal wrappers to subgoals of non-natural induction. * Added workaround for unexpected anomalies in restricted automation. * Strengthened workaround. Both to prevent more anomalies with hypotheses and to prevent endless searching for proof with hypothesis because it is used implicitly by the 'assumption' tactic. * Replaced axiomatic definitions with locked ones. Also strengthened shielded automation to depth 3 to be able to use definition supremum.. * Fixed dune build. * feat: add warnings * refactor: change the names of the warning functions * feat: add error function * Add files to _CoqProject.in * feat: use the warning in Conclusion.v * feat: deal with Rabs Rmax Rmin more easily by destructing them * Readded shortened 'Obtain accoriding to ...' tactic. * Reworked shielding to use Shorten database type. Includes new tactics for explicitly unfolding definitions, optimized for speed automation (i.e. they explicitly fail to prevent large growth search tree). * Improved expanding def for sup and inf. * Implemented user-level error throwing in tactics. * Fixed small error from merge. * Disabled debug mode automation. * Reverted back to old SupAndInf.v file. * Replaced wrapping after 'Expand the definition of ...' by throwing errors suggesting user to replace the line by a tactic with a similar effect as unfolding the definitions. (Also slightly simplified notation base case natural induction.) * Added tactic for unfolding that prints a message instead of throwing an errror. For internal use by Waterproof editor only. * chore: bump version number * fix: add internal unfold for general terms and tests for internal unfold * Hint fixes (#30) fix: change hint priorities * Automation debug (#31) * fix: change hint priorities * fix: change hint priorities * fix: fight line endings * feat: add switch for debugging of the automation * feat: Add description on how to enable debugging to readme * feat: use N1 instead of N in definition convergence * Improve either (#32) * fix: definition of divergence to infinity and min infinity * feat: add double_is_even to wp_integers rather than subsequences * feat: let either work with uninformative or if the goal is a prop * feat: also separate either code for prop for three statements. Refactor some decidability statements * refactor: use new either code in alternative characterization of continuity in R * feat: add alternative characterization continuity for natural numbers as subset of metric space of reals * feat: some adaptations for the continuity exercises * chore: update changelog and bump version number in opam file * Tactics for using strong induction to define index sequence (#33) * Added strong induction for defining index sequence. Warning: uses fixed letters 'n' and 'k' for index sequence and induction variable. * Removed 'Check' and 'Print's. * Removed old code. * Show version number (#34) * feat: Add tactic to show version number * fix: Add version file to _CoqProject.in * Allow testing against a folder with dune's runtest and set version number (#35) * feat: add exercises as dune test * feat: add python script for testing * feat: abstract the testing scripts and deal with errors better * fix: call the correct test script * Set Help to use default automation system. (#36) * Change required version number * Try to fix coq requirement * Fix for problems with strong induction for defining index sequence. (#38) * Fix for problems with strong induction for defining subsequence. * Updated formatting goal wrapper. * fix: some small fixes to be compatible with dev * fix: change order fold_right arguments in index sequence * fix: Small changes to the sequences and subsequences files because autorewrite no longer seems to work as before * chore: Change version number * try to allow for dev version * fix: try with version numbers * fix: try to fix version numbers * fix: Remove unnecessary import in Sequences.v * feat: add logging sentence for wp_autorewrite * feat: add logging sentence for wp_autorewrite (#43) * feat: create option to print rewrite hints (#44) * fix: Fix autorewrite (the env variable didn't come through properly) * fix: Compatibility with compilers >= 4.09.0 (#45) * fix: Exclude s390x architecture * refactor: put *.install and *.diags in gitignore --------- Co-authored-by: Emilio Jesus Gallego Arias Co-authored-by: Jelle * Adapt to rocq-prover/rocq#18327 (projection opacity) (#41) * Adapt to rocq-prover/rocq#18529 (no Dyn.anonymous) (#47) * Adapt to rocq-prover/rocq#18624 (Tac2ffi / Tac2val split) (#49) * Adapt to rocq-prover/rocq#18546. (#48) * Adapt to rocq-prover/rocq#18880 (#52) * Adapt to rocq-prover/rocq#18938 (EConstr.ERelevance) (#53) * Update README.md Update installation instructions * Testmaster (#54) Implement a better way to do a case analyse on a type Co-authored-by: DikieDick * fix: Change 'Variable' to 'local Parameter' * feat: add a test for awp_autorewrite from coq-master * Refactor: incorporate some changes from 8.17 and update version numbers (#57) * [build] Fix use of plugin aliases in findlib loading. (#58) Note that both lines where loading the same plugin, but activating different syntax rules; that's not really allowed as it leaves the system in a partial state. If something like that is needed, true two plugins are necessary (Which is not hard to support nowadays). * feat: Postponing choices in the Choose tactic (#59) * Merge 8.17 into main (#61) Main change according to (PR #58): we only declare one plugin now. * Renamed variables to prevent having actual exercise solution in repo (#64) * Specialize (#51) * feat: first version of waterproof specialize tactic * feat: Improve new specialize tactic, so it throws an error when a wrong variable is getting introduced. * Avoid doubling hypothesis, allow for multiple binders, prevent matching on functions, rename hypothesis to user-specified one --------- Co-authored-by: Jelle <jellewemmenhove@live.nl> * feat: Postponing choices in the Choose tactic * No longer rename variables in exists-statements --------- Co-authored-by: Jelle <jellewemmenhove@live.nl> --------- Co-authored-by: Gaëtan Gilbert <gaetan.gilbert@skyskimmer.net> Co-authored-by: Rodolphe Lepigre <lepigre@mpi-sws.org> Co-authored-by: Pierre Roux <pierre.roux@onera.fr> Co-authored-by: Emilio Jesús Gallego Arias <e+git@x80.org> Co-authored-by: Jelle <jellewemmenhove@live.nl>
This follows a discussion in
#13445 (comment)
Updated documented syntax by runningmake doc_gram_rsts
.Overlays (to be merged before the current PR)