Skip to content

[declare] Allow custom typing flags when declaring constants. - #12586

Merged
coqbot-app[bot] merged 8 commits into
rocq-prover:masterfrom
ejgallego:declare+typing_flags
Nov 27, 2020
Merged

[declare] Allow custom typing flags when declaring constants.#12586
coqbot-app[bot] merged 8 commits into
rocq-prover:masterfrom
ejgallego:declare+typing_flags

Conversation

@ejgallego

@ejgallego ejgallego commented Jun 25, 2020

Copy link
Copy Markdown
Contributor

We use the new Declare.Info structure to uniformly add properties to
the handling of constants. In this case, per-constant typing flags.

The internal code may want to see some further refactoring, including
pushing the flags down to Safe_typing.add_constant , but the changes
in the interface should be definitive.

By request, we also implement attributes to specify the typing flags, they have the form
#[typing($ATT=$ENABLE)] where:

  • ATT is one of guarded, universes, positive
  • ENABLE is one of on / off

This replaces #12539 and finishes #9004 .

@ejgallego ejgallego added needs: merge of dependency This PR depends on another PR being merged first. kind: internal API, ML documentation... labels Jun 25, 2020
@ejgallego
ejgallego requested review from a team June 25, 2020 14:26
@ejgallego
ejgallego force-pushed the declare+typing_flags branch from ec6fa01 to cabb2a5 Compare June 25, 2020 14:27
Comment thread kernel/safe_typing.ml Outdated
@ejgallego
ejgallego force-pushed the declare+typing_flags branch from 3af36b3 to 5ee6fd9 Compare June 29, 2020 09:04
@ejgallego
ejgallego marked this pull request as ready for review June 29, 2020 09:06
@ejgallego
ejgallego requested a review from a team as a code owner June 29, 2020 09:06
@ejgallego
ejgallego force-pushed the declare+typing_flags branch from 5ee6fd9 to fe9ef42 Compare June 29, 2020 09:06
@ejgallego

Copy link
Copy Markdown
Contributor Author

Not sure I'm happy with the current setup, but requesting review.

@coqbot

coqbot commented Jun 29, 2020

Copy link
Copy Markdown
Contributor

For your complete information, the following job in allow failure mode has failed: test-suite:4.12+trunk+dune

@ejgallego ejgallego removed the needs: merge of dependency This PR depends on another PR being merged first. label Jun 30, 2020
@ejgallego
ejgallego requested review from a team and SkySkimmer June 30, 2020 10:52
@ejgallego
ejgallego force-pushed the declare+typing_flags branch from d946646 to 3870d45 Compare June 30, 2020 10:59
@coqbot

coqbot commented Jun 30, 2020

Copy link
Copy Markdown
Contributor

For your complete information, the following job in allow failure mode has failed: test-suite:4.12+trunk+dune

@SkySkimmer SkySkimmer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This isn't hooked up to anything.

Comment thread doc/changelog/12-misc/12586-declare+typing_flags.rst Outdated
@SkySkimmer SkySkimmer added the needs: progress Work in progress: awaiting action from the author. label Jul 1, 2020
@ejgallego

Copy link
Copy Markdown
Contributor Author

This isn't hooked up to anything

@SkySkimmer indeed I provide here the core API and defer to #12539 and #9004 the implementation using attributes + the test cases. Do you think it is a bad idea?

How should @lthms proceed?

@SkySkimmer

Copy link
Copy Markdown
Contributor

I think having an unused API is a bad idea, we should just merge this PR and its first user.

@ejgallego

Copy link
Copy Markdown
Contributor Author

Ok , @lthms , please include this PR in your branch then.

@lthms

lthms commented Jul 1, 2020

Copy link
Copy Markdown
Contributor

Ack. I will try to give it a try later this week.

@ejgallego
ejgallego force-pushed the declare+typing_flags branch from 6724742 to ca93ab8 Compare July 2, 2020 16:30
@ejgallego

Copy link
Copy Markdown
Contributor Author

I did some progress, but still the parsing is broken.

@lthms

lthms commented Jul 6, 2020

Copy link
Copy Markdown
Contributor

Am I correct to guess your three last commits implements what I tried to do in my PR, @ejgallego? (it’s fine by me, I just want to be sure I understood correctly)

@ejgallego

Copy link
Copy Markdown
Contributor Author

Am I correct to guess your three last commits implements what I tried to do in my PR, @ejgallego? (it’s fine by me, I just want to be sure I understood correctly)

They are just a try, I didn't finish them; feel free to ignore them / update them.

@ejgallego

Copy link
Copy Markdown
Contributor Author

@lthms actually only the last commit has overlap with your PR, the rest are the base infrastructure that couldn't be implemented before #12372

@gares

gares commented Nov 23, 2020

Copy link
Copy Markdown
Member

More concretely, in the objective of a discussion at the next call, is there a summary of all propositions, together with an indication of which are the closest to be adopted and why?

Given all the valid (as in correct in English) options, I'd say we pick the ones we like the most and find less confusing for a user.
I don't think involving the community as a whole is needed in this case, but more than a few devs and users may be beneficial.
The weekly call seemed to fit both the time frame I hope for (that is very soon) and the number of participants (about 10 usually).

I can sum up all the proposals given here, as well as all the preferences, if you like.

@gares

gares commented Nov 23, 2020

Copy link
Copy Markdown
Member

terminaison(admit)

If there were many ways to pass the termination check, I'd probably like termination(structutal)...termination(lexicographic).
But we already have a syntax, for both toplevel and inner fixpoints, to express that. Here it seems we talk about a bunch of booleans (see also the other spin off PR), so I would not flip the object with the action.

ejgallego and others added 7 commits November 26, 2020 21:21
We use the new `Declare.Info` structure to uniformly add properties to
the handling of constants. In this case, per-constant typing flags.

The internal code may want to see some further refactoring, including
pushing the flags down to `Safe_typing.add_constant` , but the changes
in the interface should be definitive.

This will allow rocq-prover#12539 and rocq-prover#9004 using attributes.
This is just an experiment, but makes the uses of the API easier as we
don't mess with the global state anymore.
The syntax is the one of boolean attributes, that is to say
`#[typing($flag={yes,no}]` where `$flag` is one of `guarded`,
`universes`, `positive`.

We had to instrument the pretyper in a few places, it is interesting
that it is doing so many checks.
Most cases should be accounted in proof code, however be wary of paths
where `Global.env ()` is used.
Co-authored-by: <Théo Zimmermann <theo.zimmermann@inria.fr>
@ejgallego

Copy link
Copy Markdown
Contributor Author

Updated to bypass_check which is IMHO clear and scary enough; for the naming of the flags themselves, I've chosen to keep the same name used in the Set global options, for consistency. Renaming / deprecation is much welcome if you feel like doing it, but IMHO they should be kept in sync.

@ejgallego

Copy link
Copy Markdown
Contributor Author

Actually after looking a bit more to the global Set flags, it could make sense to just use #[check(guarded=no)] , etc... instead of bypass_check. WDYT folks?

@herbelin

Copy link
Copy Markdown
Member

As far as I'm concerned, both bypass_check(guard) and check(guarded=no) are ok to me.

@gares

gares commented Nov 27, 2020

Copy link
Copy Markdown
Member

The the sea of comments, there were these two, by myself and @mattam82:

W.r.t. the terminology, both in the attributes and refman, I'd frankly pick termination and productivity rather than guardedness (by destructor/constructor), since at least the most common one (termination) is clear even without a PhD in TT.

I agree guard_check is a bit technical.

@Zimmi48

Zimmi48 commented Nov 27, 2020

Copy link
Copy Markdown
Member

@gares: This was answered:

for the naming of the flags themselves, I've chosen to keep the same name used in the Set global options, for consistency. Renaming / deprecation is much welcome if you feel like doing it, but IMHO they should be kept in sync.

@Zimmi48

Zimmi48 commented Nov 27, 2020

Copy link
Copy Markdown
Member

I like bypass_check. Quite scary indeed 👻

@gares

gares commented Nov 27, 2020

Copy link
Copy Markdown
Member

This was answered:

for the naming of the flags themselves, I've chosen to keep the same name used in the Set global options

fair enough, but I'm not so sure the deprecation plan makes sense. I mean, if we agree that termination is better than guarded, maybe we can deprecate it right now, and provide the new name as an attribute from the start. (in another PR)

@Zimmi48

Zimmi48 commented Nov 27, 2020

Copy link
Copy Markdown
Member

Agreed. Making the change in a separate (but 8.13) PR is also a good idea to make the process more efficient.

@gares

gares commented Nov 27, 2020

Copy link
Copy Markdown
Member

@herbelin this seems all good to me. If you see no other problems, the sooner its merged the better

@herbelin

Copy link
Copy Markdown
Member

So, if I understand correctly, the current proposal is:

bypass_check(positivity)
bypass_check(universe)
bypass_check(guard)

Are the plans clear regarding guard vs termination?

Concretely, my question is whether there is a risk of having to backtrack if I merge now?

@ejgallego

Copy link
Copy Markdown
Contributor Author

Concretely, my question is whether there is a risk of having to backtrack if I merge now?

I think we could refine more; my point is indeed what @Zimmi48 said, IMHO we should keep the attributes and the Set flags in sync, otherwise seems a bit inconsistent (that's also a point for moving towards check vs bypass_check IMO)

@Zimmi48

Zimmi48 commented Nov 27, 2020

Copy link
Copy Markdown
Member

Concretely, my question is whether there is a risk of having to backtrack if I merge now?

The plan is indeed to change the guard into termination but as part of a separate PR (that would still go into 8.13).

@herbelin

Copy link
Copy Markdown
Member

OK, you then think that enough feedback has been given and we could now safely merge?

If so, will do around 8PM Paris time.

@Zimmi48

Zimmi48 commented Nov 27, 2020

Copy link
Copy Markdown
Member

Yes, because this was discussed during the Coq Call and it corresponds more or less to the conclusion.

@herbelin

Copy link
Copy Markdown
Member

@coqbot: merge now

@ejgallego

Copy link
Copy Markdown
Contributor Author

Thanks to all!

@jfehrle

jfehrle commented Nov 27, 2020

Copy link
Copy Markdown
Member

Though the train has left the station, skip_check might be a good alternative to bypass_check.

@ejgallego

Copy link
Copy Markdown
Contributor Author

Though the train has left the station, skip_check might be a good alternative to bypass_check.

There is still time until the final release. I still like just check more, see:

Set Positivity Checking.
#[check(positivity=no)] Inductive foo := ...
#[skip_check(positivity=yes] Inductive foo := ...
#[bypass_check(positivity=no)] ...

I don't like the double negation in the last example. That alone, should be a reason to go with check; if on top, we add the current semantics for Set, IMHO the case is strong. WDYT @jfehrle ?

@jfehrle

jfehrle commented Nov 28, 2020

Copy link
Copy Markdown
Member

I've not followed the details of the PR. I don't care for the double negative in the last example, either.

check(positivity=yes) and skip_check(positivity=no) seem pretty clear. check(positivity=no) is a little vague--does it mean to actively check that positivity is false or does it mean not to check that positivity is true? One would have to know/learn that. check(positivity) and skip_check(positivity) would be pretty clear, but I don't if that's sufficient for your use case.

On the other hand, check or skip_check seem to be rather general terms that might be used for other attributes, for example, hypothetically, check(transitivity)--would attributes allow/support that?

No wording is perfect and no code lasts forever. We can change this relatively easily if we come up with a better idea in the future. We may come up with better ideas as more attributes are created over time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind: enhancement Enhancement to an existing user-facing feature, tactic, etc. kind: feature New user-facing feature request or implementation. kind: internal API, ML documentation...

Projects

None yet

Development

Successfully merging this pull request may close these issues.