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

Add option Typeclasses Default Generalize Instance Variables. #6319

Closed
wants to merge 1 commit into from

Conversation

SkySkimmer
Copy link
Contributor

Not sure the \ref in the documentation is the correct one.

Not sure I completely understood what it's doing, as I feel like the meaning of Implicit and Explicit are inverted.

Ping @RalfJung, see #6030.

Not sure the \ref in the documentation is the correct one.
@SkySkimmer SkySkimmer added kind: enhancement Enhancement to an existing user-facing feature, tactic, etc. needs: review part: typeclasses The typeclass mechanism. labels Dec 5, 2017
@Zimmi48
Copy link
Member

Zimmi48 commented Dec 5, 2017

You should probably turn this option off in the compatibility file Coq87.v.

@SkySkimmer
Copy link
Contributor Author

You should probably turn this option off in the compatibility file Coq87.v.

No, the default behaviour is the 8.7 behaviour.

@RalfJung
Copy link
Contributor

RalfJung commented Dec 5, 2017

Thanks!

IIRC 8.7 does not generalize typeclass instance variables per default, but only if Generalizable All Variables is set.

Also, what is the interaction of the new option with Generalizable All Variables?

@SkySkimmer
Copy link
Contributor Author

It only generalizes those variables which are generalizable.
Basically we have preexisting syntax Instance foo : !bar which disables generalization, new syntax

Instance foo : `bar

which enables it and without an explicit marker the option decides which is picked (with 8.7 always picking `)

Github using ` for code blocks is a pain.

@RalfJung
Copy link
Contributor

RalfJung commented Dec 5, 2017

But that changes behavior, right? The following currently passes:

Class Bar (n:nat) := {}.
Fail Instance bar : Bar n := {}.

With your PR, because it defaults to `, this will fail, no?

@SkySkimmer
Copy link
Contributor Author

It already defaults to `. This PR only adds an option to switch the default and explicit syntax to pick `.

@RalfJung
Copy link
Contributor

RalfJung commented Dec 7, 2017

Ah, so ` actually just has no effect unless Generalizable All Variables is set?

@SkySkimmer
Copy link
Contributor Author

It's probably more complicated than that. ` avoids running some implicit_application transformation on the type of the instance, but I don't quite understand what's going on.
Could use some help.

Like what is going on here 95dd730

@Zimmi48 Zimmi48 added the needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. label Jan 8, 2018
@SkySkimmer SkySkimmer closed this Mar 1, 2018
@SkySkimmer
Copy link
Contributor Author

If I come back to this issue I probably won't base my work on this PR.

@Zimmi48 Zimmi48 removed needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. needs: review labels Mar 1, 2018
@RalfJung
Copy link
Contributor

RalfJung commented Mar 2, 2018

I opened an issue at #6888 to make sure this doesn't get forgotten.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted kind: enhancement Enhancement to an existing user-facing feature, tactic, etc. part: typeclasses The typeclass mechanism.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants