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

Correctly classify variables as being unfoldable in dnet patterns. #12572

Merged
merged 1 commit into from Jul 2, 2020

Conversation

ppedrot
Copy link
Member

@ppedrot ppedrot commented Jun 23, 2020

Fixes #12571.

@ppedrot ppedrot added the kind: fix This fixes a bug or incorrect documentation. label Jun 23, 2020
@ppedrot ppedrot added this to the 8.12.0 milestone Jun 23, 2020
@ppedrot ppedrot requested a review from a team as a code owner June 23, 2020 09:17
@ppedrot
Copy link
Member Author

ppedrot commented Jun 23, 2020

This will need a (backwards compatible) overlay for HoTT.

@ppedrot
Copy link
Member Author

ppedrot commented Jun 23, 2020

The bug was correctly identified in many places in HoTT, there were several comments wondering why typeclass resolution was not solving the goal...

ppedrot added a commit to ppedrot/HoTT that referenced this pull request Jun 23, 2020
@maximedenes
Copy link
Member

How about removing the default case, so as to prevent similar future problems? Maybe also in constr_pat_discr_st.

@maximedenes
Copy link
Member

Could this improve the compatibility story for switching to discriminated bases (in which case it could be good to re-evaluate it), or is it a separate mechanism?

@ppedrot
Copy link
Member Author

ppedrot commented Jun 23, 2020

@maximedenes do you mean that we need to explicit all constr constructors? I don't think it'd help much, the problem here seems to have arised from the when clause instead.

@ppedrot
Copy link
Member Author

ppedrot commented Jun 23, 2020

Could this improve the compatibility story for switching to discriminated bases

I don't think it'd matter much, it's fairly uncommon to have local definitions.

@maximedenes
Copy link
Member

do you mean that we need to explicit all constr constructors?

Yes, this is what I mean.

I don't think it'd help much, the problem here seems to have arised from the when clause instead.

In general, I tend to recommend to avoid fragile pattern matchings on constr, as it makes it very hard to extend terms. The only exception is when you are really looking for a specific kind of terms.

Also, is there any drawback in listing the few remaining cases?

@ppedrot
Copy link
Member Author

ppedrot commented Jun 23, 2020

In general, I tend to recommend to avoid fragile pattern matchings on constr

As far as I am concerned, I hate when clauses with a passion, even for non-constr matchings. They're an incredible source of bugs and subtle code, when it should be straightforward to inline an if-then-else branch that ensures statically a lot of stuff. Most other people seem to like them so I don't make a fuss, but personally I never introduce them.

Also, is there any drawback in listing the few remaining cases?

It's going to make this one-line fix bigger, but if you insist...

@maximedenes
Copy link
Member

It's going to make this one-line fix bigger

I don't think it's a big problem to add 3-4 lines to this patch.

@ppedrot
Copy link
Member Author

ppedrot commented Jun 23, 2020

Your desires are orders.

Copy link
Member

@maximedenes maximedenes left a comment

Choose a reason for hiding this comment

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

Well spotted!

ppedrot added a commit to ppedrot/HoTT that referenced this pull request Jun 23, 2020
Alizter added a commit to HoTT/Coq-HoTT that referenced this pull request Jun 23, 2020
@ppedrot
Copy link
Member Author

ppedrot commented Jun 23, 2020

The HoTT overlay has been merged upstream, this should be ready now.

@maximedenes
Copy link
Member

@mattam82 do you want to be the assignee, or should I take it, as this looks like a straightforward fix?

@ppedrot
Copy link
Member Author

ppedrot commented Jun 24, 2020

@maximedenes you should probably assign it's quite a straightforward PR.

@ppedrot
Copy link
Member Author

ppedrot commented Jun 29, 2020

@maximedenes ping

@SkySkimmer SkySkimmer self-assigned this Jul 2, 2020
@coqbot coqbot added this to Request 8.12.0 inclusion in Coq 8.12 Jul 2, 2020
@SkySkimmer SkySkimmer merged commit a1835c7 into coq:master Jul 2, 2020
@ppedrot ppedrot deleted the fix-12571 branch July 2, 2020 12:42
Zimmi48 added a commit to Zimmi48/coq that referenced this pull request Jul 2, 2020
Zimmi48 added a commit to Zimmi48/coq that referenced this pull request Jul 15, 2020
Zimmi48 added a commit to Zimmi48/coq that referenced this pull request Jul 15, 2020
@coqbot coqbot moved this from Request 8.12.0 inclusion to Shipped in 8.12.0 in Coq 8.12 Jul 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: fix This fixes a bug or incorrect documentation.
Projects
No open projects
Coq 8.12
  
Shipped in 8.12.0
Development

Successfully merging this pull request may close these issues.

Typeclass resolution does not unfold local variables.
3 participants