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

Stop implicitly adding template inductive types to Keep Equality table. #17718

Merged
merged 2 commits into from
Jun 20, 2023

Conversation

ppedrot
Copy link
Member

@ppedrot ppedrot commented Jun 10, 2023

The reason why the flag was introduced was precisely to be able to desynchronize the template status from the equality keeping behaviour. Since the Keep Equality flag has been around since 8.15 it is now time to perform this change.

@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 Jun 10, 2023
@ppedrot
Copy link
Member Author

ppedrot commented Jun 10, 2023

@coqbot run full ci

@coqbot-app coqbot-app bot removed 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 Jun 10, 2023
@coqbot-app
Copy link
Contributor

coqbot-app bot commented Jun 17, 2023

🔴 CI failure at commit 8b9a9d1 without any failure in the test-suite

✔️ Corresponding job for the base commit 037233d succeeded

❔ Ask me to try to extract a minimal test case that can be added to the test-suite

🏃 @coqbot ci minimize will minimize the following target: ci-geocoq
  • You can also pass me a specific list of targets to minimize as arguments.

@ppedrot ppedrot added the request: full CI Use this label when you want your next push to trigger a full CI. label Jun 18, 2023
@ppedrot ppedrot force-pushed the no-template-keep-inj-equality branch from 8b9a9d1 to e38d637 Compare June 18, 2023 00:09
@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 Jun 18, 2023
@ppedrot ppedrot added the kind: cleanup Code removal, deprecation, refactorings, etc. label Jun 18, 2023
@ppedrot ppedrot added this to the 8.18+rc1 milestone Jun 18, 2023
@ppedrot ppedrot marked this pull request as ready for review June 18, 2023 09:20
@ppedrot ppedrot requested review from a team as code owners June 18, 2023 09:20
@@ -3,8 +3,12 @@

(* This is also #4560 and #6273 *)

#[universes(template)]
#[warnings="-no-template-universe"]
Inductive foo := foo_1.
Copy link
Contributor

Choose a reason for hiding this comment

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

What's going on here?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is exactly the one reason why the process leading to this PR was really tricky. Basically, it is possible to have a template-poly inductive type that has no bound template poly universe. The urexample is a unit type without an explicit sort annotation (like above, and like the phantom type pattern). This flag is totally useless except for the behaviour of injection / discriminate and the like.

The warning fires as soon as the inductive has no template universes, but here we care about the inductive being effectively template, hence the attribute belly dance.

Copy link
Contributor

Choose a reason for hiding this comment

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

This flag is totally useless except for the behaviour of injection / discriminate and the like.

I thought that was only until this PR? After it does it still matter for this test?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could indeed remove it, but I think it's also a good idea to have a place to test that there is no regression for weird template corner-cases.

@SkySkimmer SkySkimmer self-assigned this Jun 19, 2023
@SkySkimmer
Copy link
Contributor

@coqbot merge now

@coqbot-app coqbot-app bot merged commit 8f100d7 into coq:master Jun 20, 2023
@ppedrot ppedrot deleted the no-template-keep-inj-equality branch June 20, 2023 19:20
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants