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

Revert "Notations: Trying using a notation with or w/o removal of coe… #11053

Closed
wants to merge 2 commits into from

Conversation

gares
Copy link
Member

@gares gares commented Nov 5, 2019

Fix #11033

Or course any other fix is fine to me

@gares gares requested a review from herbelin as a code owner November 5, 2019 14:14
@gares gares added this to the 8.11+beta1 milestone Nov 5, 2019
@gares
Copy link
Member Author

gares commented Nov 5, 2019

This bug affects 8.10 too, if a new point release is to be made @vbgl can consider backporting this

@vbgl vbgl modified the milestones: 8.11+beta1, 8.10.2 Nov 5, 2019
@vbgl
Copy link
Contributor

vbgl commented Nov 5, 2019

Can you please add a test-case corresponding to the bug report? Thanks.

@gares
Copy link
Member Author

gares commented Nov 5, 2019

There you are, this is the test I've used for bisecting (plus grep COERCION, that should not succeed, since coercions should not be printed)

@gares
Copy link
Member Author

gares commented Nov 5, 2019

Also 8.11 is affected, CC @ppedrot @mattam82 .

I don't know if a better fix can be provided in time for the release, I see this PR for the worst case scenario.

@herbelin
Copy link
Member

herbelin commented Nov 5, 2019

@gares, thanks for the quick finding of the issue! If it is clear that #8890 is a change of semantics, I would not say that it is bug though, especially regarding this other expectation.

Since this is already released in 8.10, I'd be tempted to test the following cheap-to-implement improvement of the criterion used in #8890: look for notations which span the full application of the coercion to a term, rather than looking for a notation which applies to any partial prefix of the term as it is with #8890. By doing so, in the #11033 example, Notation COERCION x := (c x). would use the notation but Notation COERCION := c. would not. This looks like a reasonable compromise on guessing what the intent of the user is in defining a notation involving a coercion.

@JasonGross
Copy link
Member

Since this is already released in 8.10, I'd be tempted to test the following cheap-to-implement improvement of the criterion used in #8890: look for notations which span the full application of the coercion to a term, rather than looking for a notation which applies to any partial prefix of the term as it is with #8890. By doing so, in the #11033 example, Notation COERCION x := (c x). would use the notation but Notation COERCION := c. would not. This looks like a reasonable compromise on guessing what the intent of the user is in defining a notation involving a coercion.

I would prefer this solution over simply reverting #8890

@Zimmi48 Zimmi48 added the kind: fix This fixes a bug or incorrect documentation. label Nov 6, 2019
@herbelin
Copy link
Member

See #11090 for the variant of #8890 which looks only for notations matching the fully applied coercion.

@ejgallego
Copy link
Member

Superseeded by #11090

@ejgallego ejgallego closed this Nov 26, 2019
@coqbot coqbot removed this from the 8.10.2 milestone Nov 26, 2019
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
None yet
Development

Successfully merging this pull request may close these issues.

Notations for coercions make coercions always visible
7 participants