Skip to content

Conversation

@herbelin
Copy link
Member

@herbelin herbelin commented Jul 20, 2024

The main objective of the PR is to replace the notation arguments of possible atomic type constr, pattern, and binders by an algebra of types based on the same atoms but composable by means of lists and tuples. It is a step towards granting #7959 as well as a way to make #18531 easier (by easily supporting a new type of notation variables).

The main commit is "A richer type system for notation variables". Two structures are provided:

  • Constrexpr.notation_arg_type: the type of instances of notation variables as known at parsing time (the atoms are: term, pattern, and binders, that is list of binders, as in forall binders, term or as in match term with pattern => term end)
  • Notation_term.notation_var_type: the type of notation variable assigned at interpretation time
    • There are three atoms again, constr, pattern and binders, but with refinements:
      • pattern carries possible constraints of the forms:
        • only ident/name
        • strict, i.e. non ident/name pattern (those patterns written `(x,y) in binders and (x,y) in match patterns)
        • patterns without restriction
      • constr carries information on whether it becomes a pattern in notations for patterns (as in (t,u) vs match term with (x,y) => term end/fun `(x,y) => term, where there is a change of type), or remain constr (as in (t,u) : typ vs fun ((x,y) : typ) => termwheretyp` is a constr even when parsed in a pattern expression)
    • Each of atom also comes with the information of how it was orginally parsed (which may differ from how it is interpreted)

Then, a number of correspondences are implemented to adjust an instance as parsed to its expected type as interpreted:

  • at definition time of the notation interpretation: by means of Metasyntax.make_interp_atom_type_rec and Metasyntax.make_interp_atom_type to inform of how parsing and interpretation types differ
  • at interpretation time: by means of functions get_term, get_pattern, ... in constrintern.ml
  • at printing time:
    • by means of functions extern_cases_pattern_notation_substitution and extern_notation_substitution in constrextern.ml to build parsing-level instances from interpretation-level instances
    • by means of functions is_term_meta, is_pattern_meta, ... in notation_ops.ml to take into account the possible constraints on the type (as well as to manage variables which denote both a term and a pattern, knowing that such notation variables used both as term and either pattern or binders are registered as patterns)

Another data type introduced in the PR is:

Note: I would not say that the invariants of the code could not be improved further and comments are welcome.

Synchronous overlays:

@herbelin herbelin added kind: cleanup Code removal, deprecation, refactorings, etc. part: notations The notation system. request: full CI Use this label when you want your next push to trigger a full CI. labels Jul 20, 2024
@herbelin herbelin added this to the 8.21+rc1 milestone Jul 20, 2024
@coqbot-app coqbot-app bot removed the request: full CI Use this label when you want your next push to trigger a full CI. label Jul 20, 2024
@herbelin herbelin added the request: full CI Use this label when you want your next push to trigger a full CI. label Jul 21, 2024
@herbelin herbelin force-pushed the master+general-recursive-notations branch from 5b0677c to 0b47dab Compare July 21, 2024 08:30
@coqbot-app coqbot-app bot removed the request: full CI Use this label when you want your next push to trigger a full CI. label Jul 21, 2024
@herbelin herbelin added the request: full CI Use this label when you want your next push to trigger a full CI. label Jul 21, 2024
@herbelin herbelin force-pushed the master+general-recursive-notations branch from 0b47dab to 0405cde Compare July 21, 2024 09:53
@coqbot-app coqbot-app bot removed the request: full CI Use this label when you want your next push to trigger a full CI. label Jul 21, 2024
@herbelin herbelin marked this pull request as ready for review July 23, 2024 07:43
@herbelin herbelin requested review from a team as code owners July 23, 2024 07:43
@ppedrot ppedrot self-assigned this Jul 23, 2024
Copy link
Contributor

@proux01 proux01 left a comment

Choose a reason for hiding this comment

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

Arg, this looks huge, I'll try to take a closer look during summer but this may take me some time.

@herbelin herbelin force-pushed the master+general-recursive-notations branch from 0405cde to 200af3e Compare July 23, 2024 14:17
@coqbot-app coqbot-app bot added the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Jul 23, 2024
@herbelin herbelin force-pushed the master+general-recursive-notations branch from 200af3e to 4988a47 Compare July 23, 2024 14:19
@herbelin
Copy link
Member Author

herbelin commented Jul 23, 2024

Note that if ever useful I can try to isolate the main commit from the auxiliary commits.

@proux01
Copy link
Contributor

proux01 commented Jul 23, 2024

Maybe not needed, let me have a look first

@github-actions github-actions bot added the needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. label Sep 3, 2024
@SkySkimmer SkySkimmer reopened this May 12, 2025
@SkySkimmer SkySkimmer added the request: full CI Use this label when you want your next push to trigger a full CI. label May 12, 2025
@SkySkimmer SkySkimmer force-pushed the master+general-recursive-notations branch from 8ff4509 to f1f2cb0 Compare May 12, 2025 12:24
@coqbot-app coqbot-app bot removed request: full CI Use this label when you want your next push to trigger a full CI. needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. stale This PR will be closed unless it is rebased. labels May 12, 2025
@SkySkimmer
Copy link
Contributor

SkySkimmer commented May 12, 2025

rebased
EDIT I guess overlays need rebase too

@SkySkimmer SkySkimmer added the needs: overlay This is breaking external developments we track in CI. label May 19, 2025
@github-actions github-actions bot added the needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. label Jul 1, 2025
@coqbot-app
Copy link
Contributor

coqbot-app bot commented Aug 27, 2025

The "needs: rebase" label was set more than 30 days ago. If the PR is not rebased in 30 days, it will be automatically closed.

@coqbot-app coqbot-app bot added the stale This PR will be closed unless it is rebased. label Aug 27, 2025
@coqbot-app
Copy link
Contributor

coqbot-app bot commented Sep 26, 2025

This PR was not rebased after 30 days despite the warning, it is now closed.

@coqbot-app coqbot-app bot closed this Sep 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind: cleanup Code removal, deprecation, refactorings, etc. needs: overlay This is breaking external developments we track in CI. needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. part: notations The notation system. stale This PR will be closed unless it is rebased.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants