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

Controlling typing flags with commands (no attribute) #10291

Merged
merged 9 commits into from Aug 20, 2019

Conversation

SimonBoulier
Copy link
Contributor

@SimonBoulier SimonBoulier commented Jun 3, 2019

This PR allows to enable and disable:

  • the guard checking
  • the positivity checking
  • the check of universes (providing "type in type")

with some Set/Unset commands.

For this, the check_guarded typing flag is split into check_guarded and check_positive.

The syntax of commands is: Set Guard Checking, Unset Guard Checking, Set/Unset Positivity Checking, Set/Unset Universe Checking.

This PR also adds a Print Typing Flags command which prints the status of those three flags, and improves a bit the output of Print Assumptions.

This PR is #9004 without the attributes part which is trickier.

Kind: feature

  • Added / updated test-suite

@coqbot coqbot added the needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. label Jun 3, 2019
@Zimmi48
Copy link
Member

Zimmi48 commented Jun 3, 2019

Note that you based this PR on an outdated branch.

@Zimmi48 Zimmi48 added the kind: feature New user-facing feature request or implementation. label Jun 3, 2019
@coqbot coqbot removed the needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. label Jun 4, 2019
@SimonBoulier
Copy link
Contributor Author

@Zimmi48 Thanks, this is corrected

Copy link
Contributor

@SkySkimmer SkySkimmer left a comment

Choose a reason for hiding this comment

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

Seems OK barring the Print Assumptions related comment.

doc/sphinx/language/gallina-specification-language.rst Outdated Show resolved Hide resolved
doc/sphinx/language/gallina-specification-language.rst Outdated Show resolved Hide resolved
doc/sphinx/language/gallina-specification-language.rst Outdated Show resolved Hide resolved
doc/sphinx/proof-engine/vernacular-commands.rst Outdated Show resolved Hide resolved
doc/sphinx/proof-engine/vernacular-commands.rst Outdated Show resolved Hide resolved
doc/sphinx/proof-engine/vernacular-commands.rst Outdated Show resolved Hide resolved
doc/sphinx/proof-engine/vernacular-commands.rst Outdated Show resolved Hide resolved
printing/printer.ml Outdated Show resolved Hide resolved
@Zimmi48 Zimmi48 added the needs: changelog entry This should be documented in doc/changelog. label Jun 4, 2019
@SimonBoulier
Copy link
Contributor Author

SimonBoulier commented Jun 6, 2019

Should be ok now.

@SkySkimmer
Copy link
Contributor

SkySkimmer commented Jun 6, 2019

The test suite fails as the checker doesn't take into account the typing flags associated to a constant/inductive.

@SkySkimmer SkySkimmer added the needs: fixing The proposed code change is broken. label Jun 6, 2019
@SimonBoulier
Copy link
Contributor Author

Should be fixed :-)

checker/check_stat.ml Outdated Show resolved Hide resolved
@SimonBoulier
Copy link
Contributor Author

@ppedrot @SkySkimmer Can we merge this ?

@SkySkimmer
Copy link
Contributor

The checker using local typing flags means it also tries to use native compile / VM (and fails in native compile in the test suite)
We don't want the checker using either AFAIK

@SimonBoulier
Copy link
Contributor Author

Should be ok!

@Zimmi48
Copy link
Member

Zimmi48 commented Jun 20, 2019

Note that this PR is still missing a changelog entry (see https://github.com/coq/coq/tree/master/doc/changelog#unreleased-changelog for instructions).

@SimonBoulier
Copy link
Contributor Author

I added the changelog, thanks for your advice @Zimmi48

@SkySkimmer SkySkimmer added part: kernel part: vernac High level command interpretation. labels Jul 2, 2019
(* Locally set typing flags for further typechecking *)
let orig_flags = env.env_typing_flags in
let cb_flags = cb.const_typing_flags in
let env = Environ.set_typing_flags {orig_flags with check_guarded = cb_flags.check_guarded;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be in the let env', such that you don't need to reset typing flags.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is still todo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. Sorry for having missing that.

In fact I don't understand the lines 27-29:

  let poly, env' =
    match cb.const_universes with
    | Monomorphic ctx -> false, env

Why is the env not enriched in the monomorphic case? Are the universes declared in the definition supposed to be yet here?
If so, why do we take env' instead of env in line 61?

  let env =
    if poly then add_constant kn cb env
    else add_constant kn cb env'

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be in the let env', such that you don't need to reset typing flags.

Actually never mind, we already do this resetting manipulation before the PR for the oracle so I'll clean it up in a follow up.

Copy link
Contributor

@SkySkimmer SkySkimmer left a comment

Choose a reason for hiding this comment

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

Needs rebasing due to conflict

@coqbot coqbot removed the needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. label Aug 16, 2019
@SimonBoulier
Copy link
Contributor Author

@SkySkimmer I'm back and I rebased!

(* Locally set typing flags for further typechecking *)
let orig_flags = env.env_typing_flags in
let cb_flags = cb.const_typing_flags in
let env = Environ.set_typing_flags {orig_flags with check_guarded = cb_flags.check_guarded;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is still todo

@SkySkimmer SkySkimmer self-assigned this Aug 20, 2019
@SkySkimmer SkySkimmer added this to the 8.11+beta1 milestone Aug 20, 2019
Copy link
Contributor

@SkySkimmer SkySkimmer left a comment

Choose a reason for hiding this comment

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

Considering this has been open for months and the changes don't look especially dangerous I'm going to assume other kernel/checker maintainers have nothing to say and merge without further delay.

(* Locally set typing flags for further typechecking *)
let orig_flags = env.env_typing_flags in
let cb_flags = cb.const_typing_flags in
let env = Environ.set_typing_flags {orig_flags with check_guarded = cb_flags.check_guarded;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be in the let env', such that you don't need to reset typing flags.

Actually never mind, we already do this resetting manipulation before the PR for the oracle so I'll clean it up in a follow up.

SkySkimmer added a commit to SkySkimmer/coq that referenced this pull request Aug 20, 2019
…ute)

Reviewed-by: SkySkimmer
Reviewed-by: Zimmi48
@SkySkimmer SkySkimmer merged commit d6d8229 into coq:master Aug 20, 2019
ionathanch pushed a commit to ionathanch/coq that referenced this pull request Aug 28, 2019
…d sized_typing plugin that did the same thing.
wilbowma pushed a commit to wilbowma/coq that referenced this pull request Aug 28, 2019
…d sized_typing plugin that did the same thing.
ionathanch pushed a commit to ionathanch/coq that referenced this pull request Aug 30, 2019
…d sized_typing plugin that did the same thing.
ionathanch pushed a commit to ionathanch/coq that referenced this pull request Sep 6, 2019
…d sized_typing plugin that did the same thing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: feature New user-facing feature request or implementation. part: kernel part: vernac High level command interpretation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants