Skip to content
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

Add notations for patterns in [seq ... | ... ] notations #790

Merged
merged 2 commits into from Apr 6, 2023

Conversation

JasonGross
Copy link
Contributor

@JasonGross JasonGross commented Sep 26, 2021

This allows syntax such as

Check [seq '(x, y) <- List.combine (List.seq 0 5) (List.seq 0 5) | x == y ].
Check [seq x + y | '(x, y) <- List.combine (List.seq 5 5) (List.seq 5 5) ].

to parse and print.

Motivation for this change

Without this change,

Check map (fun '(x, y) => x + y) (zip (iota 0 5) (iota 0 5)).

prints as the ugly

[seq (let '(x, y) := pat in x + y) | pat <- zip (iota 0 5) (iota 0 5)]

instead of the much more understandable

[seq x + y | '(x, y) <- zip (iota 0 5) (iota 0 5)]
Things done/to do
  • added corresponding entries in CHANGELOG_UNRELEASED.md (do not edit former entries)
  • added corresponding documentation in the headers (Do I need to do this? I think it's not relevant here? Can someone confirm?)
Automatic note to reviewers

Read this Checklist and make sure there is a milestone.

@gares
Copy link
Member

gares commented Sep 26, 2021

Apparently there is a conflict in fintype

@JasonGross
Copy link
Contributor Author

I think I need @herbelin 's help in how to factor notations so that they work:

Require Import Coq.Lists.List.
Declare Scope seq_scope.
Open Scope seq_scope.
Notation "[ 'seq' E | i <- s ]" := (map (fun i => E) s)
  (at level 0, E at level 99, i binder,
   format "[ '[hv' 'seq'  E '/ '  |  i  <-  s ] ']'") : seq_scope.
Check [seq ?[F] i | i <- ?[enum] ?[P] ].
Reserved Notation "[ 'seq' F | x : T ]"
  (at level 0, F at level 99, x ident,
   format "'[hv' [ 'seq'  F '/ '  |  x  :  T ] ']'").
Check [seq ?[F] i | i <- ?[enum] ?[P] ].
(* Error: Syntax error: ':' expected after [name] (in [term]). *)

@CohenCyril CohenCyril added this to the 1.14.0 milestone Oct 25, 2021
@CohenCyril
Copy link
Member

Hi @JasonGross @herbelin any hope to make this PR go forward?

@herbelin
Copy link
Contributor

Hi,

At first view, supporting [ 'seq' E | i <- s ] with i a binder would require to change x to binder also in [ 'seq' F | x : T ] but the latter does not seem easy to generalize because the x : T has apparently a more complex interpretation than just being a binder, see the in pred_of_simpl (@pred_of_argType T) part (what would go wrong actually by replacing this part with fun x : T => true, in which case we would get the generalization easily?).

Assuming that there is no easy way to reformulate pred_of_simpl (@pred_of_argType T) under a form referring to x : T rather than to only T, what could be tried is to only add [ 'seq' E | ' i <- s ] with i pattern, rather than generalizing [ 'seq' E | i <- s ] to i binder.

Hoping that it helps.

@herbelin
Copy link
Contributor

@CohenCyril: we would need your help to know if there is a way to reformulate pred_of_simpl (@pred_of_argType T) under a form referring to x : T rather than to only T in notation [ 'seq' F | x : T ] := [seq F | x : T in pred_of_simpl (@pred_of_argType T)].

@JasonGross
Copy link
Contributor Author

reformulate pred_of_simpl (@pred_of_argType T) under a form referring to x : T rather than to only T in notation

@herbelin What about using the match trick for substitution to do (fun A (a : A) => A) _ (x : T)) in a way that does not leave a beta redex? (I can code up a solution that does this the next time I'm at a computer; I don't have in my head an accurate model of which zeta-redexs unification will unfold and in what order, when it needs to perform evar unification.)

@herbelin
Copy link
Contributor

@herbelin What about using the match trick for substitution to do (fun A (a : A) => A) _ (x : T)) in a way that does not leave a beta redex? (I can code up a solution that does this the next time I'm at a computer; I don't have in my head an accurate model of which zeta-redexs unification will unfold and in what order, when it needs to perform evar unification.)

I don't know if that'll work. We need x : T to occur in position of binder so that when substituted with something such as 'pat it continues to make sense,

@JasonGross
Copy link
Contributor Author

JasonGross commented Jan 13, 2022

We need x : T to occur in position of binder so that when substituted with something such as 'pat it continues to make sense,

What about the equivalent of (fun A (a : A -> True) => A) _ (fun x : T => I)?

@JasonGross
Copy link
Contributor Author

(Of course the match form will never be used for printing, I don't know if that is an issue)

@JasonGross
Copy link
Contributor Author

What about the equivalent of (fun A (a : A -> True) => A) _ (fun x : T => I)?

Notation "'type_of' x"
  := match _, (fun x => I) with
     | T, f
       => match match f return T -> True with f' => f' end with
          | _ => T
          end
     end
       (x binder, only parsing, at level 20).
Check type_of x : nat. (* nat *)
Check type_of '((x, y) : nat * nat). (* (nat * nat)%type *)

@JasonGross
Copy link
Contributor Author

Updated PR, seems to be working

@JasonGross
Copy link
Contributor Author

JasonGross commented Jan 14, 2022

Two issues seem to arise:

  1. This change is incompatible with Coq 8.11 and Coq 8.12. With mathcomp be dropping support for these versions on master anytime soon?
  2. multinomials fails with
File "./src/freeg.v", line 207, characters 37-38:
Error:
In environment
G : zmodType
K : choiceType
s : seq (G * K)
x : K
The term "x" has type "Choice.sort K" while it is expected to have type
 "(?A * ?B)%type".

at https://github.com/math-comp/multinomials/blob/200ef66f7f892814dc7b9254f4f23bc7f59de093/src/freeg.v#L207 which is

Definition predom s: seq K := [seq x.2 | x <- s].

with

Variable G : zmodType.
Variable K : choiceType.
Implicit Types s   : seq (G * K).
Implicit Types x y : K.

The issue is that Coq now infers x : K rather than x : _ * _. There's an easy backwards-compatible fix of changing x to v on that line; is that acceptable?

JasonGross added a commit to JasonGross/multinomials that referenced this pull request Jan 14, 2022
This will hopefully make the monomials package compatible with math-comp/math-comp#790 (see math-comp/math-comp#790 (comment)).  Due to a change in how notations are interpreted, Coq will now resolve implicit types on `x` in `[seq _ | x <- _]` before performing unification rather than after, resulting in a type error.  So we avoid using variables which have implicit types attached.
@CohenCyril
Copy link
Member

  1. This change is incompatible with Coq 8.11 and Coq 8.12. With mathcomp be dropping support for these versions on master anytime soon?

Coq 8.11 I think we can do it for the next release, for Coq 8.12 we might have to wait a few more month but eventually yes.

@CohenCyril CohenCyril modified the milestones: 1.14.0, 1.15.0 Jan 14, 2022
strub pushed a commit to math-comp/multinomials that referenced this pull request Jan 15, 2022
This will hopefully make the monomials package compatible with math-comp/math-comp#790 (see math-comp/math-comp#790 (comment)).  Due to a change in how notations are interpreted, Coq will now resolve implicit types on `x` in `[seq _ | x <- _]` before performing unification rather than after, resulting in a type error.  So we avoid using variables which have implicit types attached.
@JasonGross JasonGross closed this Jan 15, 2022
@coqbot-app coqbot-app bot removed this from the 1.15.0 milestone Jan 15, 2022
@JasonGross JasonGross reopened this Jan 15, 2022
@JasonGross
Copy link
Contributor Author

Oops, I closed and reopened to restart the CI, and coqbot removed the 1.15.0 milestone :-(

@proux01 proux01 added this to the 1.15.0 milestone Jan 15, 2022
@proux01
Copy link
Contributor

proux01 commented Jun 8, 2022

@JasonGross mathcomp now requires Coq >= 8.13, does that help here?

@gares gares modified the milestones: 1.15.0, 1.16.0 Jun 29, 2022
@affeldt-aist affeldt-aist modified the milestones: 1.16.0, 1.17.0 Jan 31, 2023
@JasonGross
Copy link
Contributor Author

Sorry, this fell off my todo list, I'll try to rebase shortly

@JasonGross JasonGross force-pushed the better-seq branch 3 times, most recently from 20a01b4 to 85ad786 Compare February 28, 2023 21:08
@JasonGross
Copy link
Contributor Author

I've rebased, but the CI seems extremely unhappy.

@JasonGross
Copy link
Contributor Author

And now there is no CI?

JasonGross added a commit to JasonGross/multinomials that referenced this pull request Mar 3, 2023
@JasonGross
Copy link
Contributor Author

Overlay for monomials: math-comp/multinomials#73

JasonGross added a commit to JasonGross/multinomials that referenced this pull request Mar 3, 2023
@JasonGross
Copy link
Contributor Author

I'm confused by the error in odd-order:
File "./theories/PFsection3.v", line 335, characters 17-19:
Error: Syntax error: ''' or [term level 99] expected after 'seq' (in [term]).

I cannot reproduce this locally. Can anyone else reproduce this?

strub pushed a commit to math-comp/multinomials that referenced this pull request Mar 6, 2023
Companion of #56, see #56 for explanation
@JasonGross JasonGross closed this Mar 6, 2023
@coqbot-app coqbot-app bot removed this from the 1.17.0 milestone Mar 6, 2023
@JasonGross JasonGross reopened this Mar 6, 2023
This allows syntax such as
```coq
Check [seq '(x, y) <- List.combine (List.seq 0 5) (List.seq 0 5) | x == y ].
Check [seq x + y | '(x, y) <- List.combine (List.seq 5 5) (List.seq 5 5) ].
```
to parse and print.
@JasonGross
Copy link
Contributor Author

JasonGross commented Apr 1, 2023

I'm confused by the error in odd-order: File "./theories/PFsection3.v", line 335, characters 17-19: Error: Syntax error: ''' or [term level 99] expected after 'seq' (in [term]).

I cannot reproduce this locally. Can anyone else reproduce this?

Ah, this only occurs with Coq 8.13. Is odd-order planning to drop support for Coq 8.13 anytime soon? (The rest of the ecosystem seems to build fine.)

JasonGross added a commit to JasonGross/odd-order that referenced this pull request Apr 1, 2023
I hope this fixes the last build issue with
math-comp/math-comp#790.  This change is only
needed in Coq 8.13; 8.14 and newer work fine without this change.
@JasonGross
Copy link
Contributor Author

Ah, maybe if someone merges math-comp/odd-order#46 then this PR will work (finally) as-is

@JasonGross JasonGross closed this Apr 2, 2023
@JasonGross JasonGross reopened this Apr 2, 2023
@affeldt-aist affeldt-aist self-assigned this Apr 5, 2023
@affeldt-aist affeldt-aist merged commit d1c40ed into math-comp:master Apr 6, 2023
383 of 384 checks passed
@JasonGross JasonGross deleted the better-seq branch April 6, 2023 11:16
proux01 pushed a commit that referenced this pull request Apr 7, 2023
* Add notations for patterns in [seq ... ] notations

This allows syntax such as
```coq
Check [seq '(x, y) <- List.combine (List.seq 0 5) (List.seq 0 5) | x == y ].
Check [seq x + y | '(x, y) <- List.combine (List.seq 5 5) (List.seq 5 5) ].
```
to parse and print.

* Fix fintype
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.

None yet

6 participants