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

Allow projections to be declared as classes. #9711

Merged
merged 1 commit into from
May 30, 2021

Conversation

mattam82
Copy link
Member

@mattam82 mattam82 commented Mar 7, 2019

Kind: feature

This fixes an arbitrary limitation of typeclasses, which didn't allow primitive projections
to be declared as class heads. They can usefully be used as proxys. In our use case:

Class UR A B := {
  ur : A -> B -> Type 
}.

We want to declare ur as a type class head, as in all instances of UR, ur is itself defined by a type-class. We can use a hint to unfold the ur projection when applied to a particular UR instance to simplify the goal to a new typeclass constraint. This allows a kind of dependent type-class resolution.

As long as users did not try to do Existing Class ur for some primitive projection ur, which had no effect previously, this should be backwards compatible.

Fix #12975, fix #8994.

  • Added / updated test-suite
  • Corresponding documentation was added / updated (including any warning and error messages added / removed / modified).

@SkySkimmer
Copy link
Contributor

@mattam82 is this PR dead?

@github-actions github-actions bot added needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. labels Aug 24, 2020
@mattam82
Copy link
Member Author

mattam82 commented Jan 7, 2021

It's been forgotten, I'll try to bring it back to life

@ppedrot
Copy link
Member

ppedrot commented May 7, 2021

@mattam82 ping. I believe this is quite high priority actually.

@mattam82
Copy link
Member Author

mattam82 commented May 8, 2021

@ppedrot I’m on holidays next week, so if you want to pick it up you’re welcome. Otherwise thanks for the reminder !

@mattam82 mattam82 force-pushed the projections-can-be-class-heads branch from 45e6392 to 7c6714f Compare May 26, 2021 09:36
@coqbot-app coqbot-app bot removed the needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. label May 26, 2021
@mattam82
Copy link
Member Author

Rebased now, let's see.

@mattam82 mattam82 marked this pull request as ready for review May 26, 2021 09:36
@mattam82 mattam82 requested a review from a team as a code owner May 26, 2021 09:36
@mattam82
Copy link
Member Author

@ppedrot this is ready now.

@mattam82 mattam82 force-pushed the projections-can-be-class-heads branch from 7c6714f to 5a0a45b Compare May 26, 2021 13:52
@@ -130,22 +138,19 @@ let rec is_class_type evd c =
match EConstr.kind evd c with
| Prod (_, _, t) -> is_class_type evd t
| Cast (t, _, _) -> is_class_type evd t
| Proj (p, c) -> GlobRef.(Map.mem (ConstRef (Projection.constant p))) !classes
Copy link
Member

Choose a reason for hiding this comment

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

This is adding a new dependency to the legacy constant binding mechanism. I know we're a bit in a hurry w.r.t. the release, but instead of doing that we should have a type of global references including projections. I can validate the PR as is, but if you have time, could you do that the right way from the start?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I don't really have time right now, but indeed that should happen next. Note that we already had code using Projection.constant here, so it's not really worse than before.

@mattam82 mattam82 force-pushed the projections-can-be-class-heads branch from 5a0a45b to 726a496 Compare May 27, 2021 12:49
@ppedrot ppedrot self-assigned this May 28, 2021
@ppedrot
Copy link
Member

ppedrot commented May 28, 2021

@mattam82 can you squash the last commit? It's useless to keep it.

@ppedrot ppedrot added the kind: feature New user-facing feature request or implementation. label May 28, 2021
@ppedrot ppedrot added this to the 8.14+rc1 milestone May 28, 2021
@mattam82 mattam82 force-pushed the projections-can-be-class-heads branch from 726a496 to 2a691de Compare May 28, 2021 08:52
@mattam82 mattam82 requested review from a team as code owners May 28, 2021 08:52
@mattam82
Copy link
Member Author

Sure. I rebased on #14392 to see where CI gets now.

@ppedrot
Copy link
Member

ppedrot commented May 28, 2021

Erm, I would gladly have merged this PR without the additional comments as soon as the original CI would have finished... This needs to be merged before the branch, so you'll have to rebase again to remove the commits once your experiment is finished.

@mattam82
Copy link
Member Author

That's fine. I'm not sure this is the end just yet.

@coqbot-app
Copy link
Contributor

coqbot-app bot commented May 28, 2021

Hey, I have detected that there were CI failures at commit 2a691de without any failure in the test-suite.
I checked that the corresponding jobs for the base commit 92f3b90 succeeded. You can ask me to try to extract a minimal test case from this so that it can be added to the test-suite.
If you tag me saying @coqbot ci minimize, I will minimize the following target: ci-coq_tools.

1 similar comment
@coqbot-app
Copy link
Contributor

coqbot-app bot commented May 28, 2021

Hey, I have detected that there were CI failures at commit 2a691de without any failure in the test-suite.
I checked that the corresponding jobs for the base commit 92f3b90 succeeded. You can ask me to try to extract a minimal test case from this so that it can be added to the test-suite.
If you tag me saying @coqbot ci minimize, I will minimize the following target: ci-coq_tools.

@ppedrot
Copy link
Member

ppedrot commented May 28, 2021

Why is coqbot repeating the very same thing twice?

@JasonGross
Copy link
Member

It'll report again every time the overall pipeline status check is finished. If a job fails and then someone restarts the job, when it finishes the second time, this will trigger a second comment. Perhaps that's what happened? (The coq-tools issue is on my side, I'll fix it shortly, sorry.)

@mattam82
Copy link
Member Author

What is coq-tools ?

@mattam82
Copy link
Member Author

Yes, I did retrigger it.

@mattam82
Copy link
Member Author

mattam82 commented May 28, 2021

Do you mean that the failure of the ci-coq_tools job is your fault?

@JasonGross
Copy link
Member

Do you mean that the failure of the ci-coq_tools job is your fault?

Yes, I pushed what I thought was an innocuous commit working around an intermittent issue with the native compiler, not realizing that it would change the output log, and I didn't enforce the discipline of running the test-suite before pushing. This broke about half of the output tests, which I'm now fixing; it should be fixed within 10 minutes, and sorry about this.

@mattam82
Copy link
Member Author

That's good news, both PRs should go green by tonight then!

@JasonGross
Copy link
Member

coq-tools should be fixed now, I've restarted it

@coqbot-app
Copy link
Contributor

coqbot-app bot commented May 28, 2021

Hey, I have detected that there were CI failures at commit 2a691de without any failure in the test-suite.
I checked that the corresponding jobs for the base commit 92f3b90 succeeded. You can ask me to try to extract a minimal test case from this so that it can be added to the test-suite.
If you tag me saying @coqbot ci minimize, I will minimize the following target: ci-coq_tools.

@ppedrot
Copy link
Member

ppedrot commented May 28, 2021

... still failing it seems.

@JasonGross
Copy link
Member

Yeah, I forgot that -native-compiler ondemand is required to work around native compiler issues on old versions of coq but produces warnings on new versions of Coq. Hopefully now fixed

@ppedrot ppedrot force-pushed the projections-can-be-class-heads branch from 2a691de to ba0125a Compare May 30, 2021 11:26
@ppedrot ppedrot force-pushed the projections-can-be-class-heads branch from ba0125a to ad3d2b4 Compare May 30, 2021 11:27
@ppedrot
Copy link
Member

ppedrot commented May 30, 2021

@mattam82 in order not to lose time I have rebased and slightly tweaked the commit message, I'll merge when the CI finishes.

@ppedrot ppedrot merged commit 495a335 into coq:master May 30, 2021
@mattam82 mattam82 deleted the projections-can-be-class-heads branch June 1, 2021 09:17
@mattam82
Copy link
Member Author

mattam82 commented Jun 1, 2021

@ppedrot thanks a whole bunch!

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.
Projects
None yet
4 participants