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

Fix 9663 (Miller pattern unification fails on evars) #9690

Merged
merged 1 commit into from
Mar 26, 2019

Conversation

gares
Copy link
Member

@gares gares commented Mar 4, 2019

In particular evar_eqappr_x tries to find a Miller pattern on both sides,
while the fast path for evars tries solve_simple_eqn in one direction
only. So if you have (Evar-not-Miller = Evar-Miller) the code was not
trying to solve the RHS.

Depends on: #7819 (to avoid conflicts)

@gares gares requested review from forestjulien, mattam82, ppedrot and a team as code owners March 4, 2019 14:20
@gares gares added the needs: merge of dependency This PR depends on another PR being merged first. label Mar 4, 2019
@SkySkimmer SkySkimmer changed the title Fix 9663 Fix 9663 (Miller pattern unification fails on evars) Mar 4, 2019
@gares
Copy link
Member Author

gares commented Mar 4, 2019

@mattam82 you are fresh on this code, please tell me if my fix is too aggressive.

In particular I start to doubt that the fast path for simple_eqn is just making things more buggy.
As this patch proves, it is less powerful than the code that follows, but does not always backtrack.
So, even if it is faster, it looks like a "random" optimization to me.

@gares gares added the kind: fix This fixes a bug or incorrect documentation. label Mar 4, 2019
@gares gares added this to the 8.10+beta1 milestone Mar 4, 2019
@gares gares added this to Todo ( in Enrico's work ) via automation Mar 4, 2019
@ejgallego
Copy link
Member

Would you mind submitting depending PRs as Drafts in the future?

Copy link
Contributor

@ggonthier ggonthier left a comment

Choose a reason for hiding this comment

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

This PR does fix the evar-to-Miller issue, but perhaps a related Miller-to-Miller problem should be also be addressed, e.g.:

Definition id_depfn21 {T A R} f := f : forall x y : T, A x -> R.
Definition id_depfn22 {T A R} f := f : forall x y : T, A y -> R.
Fail Definition idn2 f : nat -> nat -> nat := id_depfn21 (id_depfn22 f).

In general, all NotClean errors should check whether the offending occurrences of variables occur only in evar contexts or in arguments of applied evars, as in such cases they can be unambiguously resolved by filtering the evars or assigning them K-terms, respectively.

@@ -502,13 +502,13 @@ let rec evar_conv_x flags env evd pbty term1 term2 =
| Evar ev, _ when Evd.is_undefined evd (fst ev) && not (is_frozen flags ev) ->
(match solve_simple_eqn (conv_fun evar_conv_x) flags env evd
(position_problem true pbty,ev,term2) with
| UnifFailure (_,OccurCheck _) ->
| UnifFailure (_,(OccurCheck _ | NotClean _)) ->
(* Eta-expansion might apply *) default ()
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment on the following line need to be modified.

@ejgallego ejgallego removed the needs: merge of dependency This PR depends on another PR being merged first. label Mar 12, 2019
@gares
Copy link
Member Author

gares commented Mar 13, 2019

I think the second bug is morally from the same source, but this time is in imitate from solve_simple_eqn
The original sin is that App(Evar(k,a),b) is not systematically turned into Evar(k',a ++ b) where ?k := fun b => ?k[a,b]). It seem the code for pruning an applied evar in solve_simple_eq understands only the latter form.
Unfortunately the fix is not so trivial, so I'm not going to do it in this PR (not sure I'm going to do it, this code is really bad).

This PR is still OK I think, I'll rebase and fix the comment (I can't make sense of it even for the existing case, ideas?).

@gares
Copy link
Member Author

gares commented Mar 21, 2019

@mattam82 can you take and review this one?

@gares
Copy link
Member Author

gares commented Mar 22, 2019

I'd like this to get in 8.10 since math-comp/math-comp#294 needs it and math-comp tries to be compatible with 2 versions of Coq. So if it misses 8.10 we won't even be able to merge the other PR in 6 months...

@mattam82
Copy link
Member

Right, it's on my TODO for next week.

Copy link
Member

@mattam82 mattam82 left a comment

Choose a reason for hiding this comment

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

The comment was meant as a reminder that ?X = {| foo := ?X.(foo) |} can fail due to occur-check, while eta-expansion can solve it. IIUC, NotClean can also be raised, while pruning more would succeed, I guess the comment should reflect that.

@gares
Copy link
Member Author

gares commented Mar 26, 2019

I'm rebasing & updating the comment right away

In particular evar_eqappr_x tries to find a miller pattern on both sides,
while the fast path for evars tries solve_simple_eqn in one direction
only. So if you have (Evar-not-miller = Evar-miller) the code was not
trying to solve the RHS.
@gares
Copy link
Member Author

gares commented Mar 26, 2019

done

@mattam82
Copy link
Member

Thanks, I'll wait for a last CI just in case and merge.

@mattam82
Copy link
Member

Funny enough, this uncovers a bug in equations, unification is stronger which leads it to produce an ill-typed proof term. I'm working on a fix and will report here.

@mattam82
Copy link
Member

@gares I made the fix to equations, basically now a constraint is postponed when it was failing before, I just switched to the new unify which doesn't allow delaying. Interestingly, it is postponed exactly for the (bad) reason that you mentioned of not handling applied evars correctly. But that can be fixed at a later stage indeed. Now the CI is all green :)

@mattam82 mattam82 merged commit b4561c5 into coq:master Mar 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
No open projects
Enrico's work )
  
Done (
Development

Successfully merging this pull request may close these issues.

None yet

4 participants