A tentative fix for #4413#370
Conversation
|
Hi, can someone try branch trunk+4413-evar-evar-fix at https://github.com/herbelin/github-coq.git on a type inference intensive development (@JasonGross maybe?)? On all examples I could test, it is strictly better than the evar-evar order of v8.5 and v8.6. |
|
I had a look at the problem and the proposed fix. It think the fix is going in the wrong direction. Similarly it was wrong to change the evar/evar solution of unification in 8.5. The bugreport is about "pose proof t" where t is a term like "f ?x H" where f is a constant, ?x is an implicit argument and H is an existing hypothesis of type "T ?a" and "?a" is in evar that exists before one runs pose proof. ?x is too an evar, generated while interpreting the term. The type of "f" is "forall x, T x -> P" so "f ?x H" is well type as long as ?x and ?a are identified. This way of doing is too simplistic, and too syntactic. Since ?a is now accessible-via/visible-by ?x (if you take the evars pre-existing and normalize their evar_info you find ?a in there) ?a has not to be considered as a fresh evar. The point being that is ?x was a (shelved) goal also ?a is. It is not a new goal. This is not a bug in unification but in the way tactic(s?) compute if they generate new goals or not. Having unification making choices based on what is "declared to probably be a new goal unless solved before" is making the algorithm even harder to predict. |
|
@gares: Thanks a lot for caring about the technicalities of this bug. Your analysis is very precise which is great. I agree that the new evar does not have to be considered as a fresh evar. How to proceed? It seemed to me that If I'm not mistaken, there are a few typos, with ?a and ?x sometimes swapped, but this does not change the essence of the analysis. |
|
I think the fix should be made by a tactical: it saves the original evar map, runs the tactic, tells you the list of new evars (that are new wrt the old evar map and not reachable by the assignments in the new evar map to evars that were open in the old evar map). Once you have such list you decide what to do: fail, succeed, shelve. Note that such list is stable w.r.t. the choice of solution in evar-evar unif problems. |
|
@gares: If I understand well your proposal, and if I leave aside at the current time the idea of passing via a tactical, what you are suggesting is to change the detection of what is a "derived" evar in the function What is the price that we are ready to pay with this proposal. For instance, in: Goal exists A, forall f : A -> nat, True.
eexists ?[A]; intro.
pose (x := fun l : list ?[B] => f l).do we want to consider This being said, I don't know how to make progresses. Maybe could we continue collecting examples of what we would like to have? What do you think? How would you like yourself to proceed? Otherwise, about evar-evar unification specifically, I would like to mention one my concerns which is the propagation of evar names. Consider Goal exists A, forall f : A -> nat, True.
eexists ?[A]; intro.
pose (x := fun l => f l).which we a priori want it to work. I think that we would like to see is and not, say BTW, it would be very convenient that PR #248 is integrated to work on such case studies. @maximedenes: would you like and be ready to dedicate time at merging it soon? |
|
@gares: Did you have the opportunity to think more at this PR? Do you still feel unsatisfied with the criterion and would like to propose something else? It is unfortunate that the idea I propose did not come at the time of v8.5 but it seems to me to remain the best evar-evar criterion we can propose currently (simple code, simple criterion, less examples failing than at the time of v8.5, no impact on tests). |
|
Names of evars, which evar gets assigned and which evar is considered as a new goal are all different matters. When 2 evars are unified, the one that survives should have as a name the "union" of the two names. A user-given name is higher than a generated one. An evar may also have two names if it is the result of merging two named evars, what is wrong with it? My proposal of delegating the job to a tactical was only for deciding if a new goal is created or not, and |
|
@gares: what would you then propose concretely? Would you like to propose the tactical you are taking about? Would you like to develop an evar metadata merging discipline? In any case, your ideas do not seem essentially contradictory with the very simple patch I'm proposing. So, what about already merging this without procrastinating further (*) to get the benefit of it and then you can concentrate on developing your proposal? (*) A remark which by the way applies to other PRs as well... |
|
@gares, @maximedenes: on coqdev, @maximedenes said "evar/evar resolution depending on considerations of clients unification seems to be a wrong approach". May I insist and ask what is concretely the right approach for you? Having to deal with evar/evar unification problem is intrinsic to any unification algorithm and choices have unavoidably to be made. On what considerations do you think choices should be based then? |
|
From what I understand @gares proposal avoid making user-visible choices of naming and instantiation when we don't actually have to make any, so I'd rather be in favor of pushing that idea too. The less internals of unification we expose to the user the more freedom we get. You seem to both agree on the criterion for deciding what's new and what's not, an issue I've been facing in unifall as well (for apply based on evars). If that's right then I'll take time to design that tactical. |
|
After a corridor discussion with @mattam82, it seems to me that there is a misunderstanding. I started from the regression shown by bug #4413 and I arrived to what I consider to be an improvement of the evar-evar heuristic. Improvement because a 7-line criterion is replaced by a simple one-line criterion. Improvement because not relying anymore on the evar source. If I had found this criterion early enough for v8.5, I would clearly have implemented it for v8.5 and it is a pity that I was not clever enough to having found it already for v8.5. So, even if I started from #4413, the result goes beyond the question of just fixing #4413. And a good sign is also that it e.g. fixes the regression #4095 which has nothing to do with deciding if an evar is new or not after evaluation of a tactic. So, I think we should really separate the questions:
|
This was a regression wrt v8.4 related to the change of order of resolution of evar-evar unification problems.
…ion. The new heuristic seems to subsume the previous usages without defects such as the one mentioned by J. Leivent in the discussion for rocq-prover#5219.
b283008 to
883ecdf
Compare
|
My assumption that the improved one-line evar-evar heuristic ( This actually highlights that at the current time, the situation is not symmetric in evar-evar problems wrt Class A.
Instance a:A.
Check fun H:?[x]=a :> id A => f_equal (fun x:A => x) H.
(* fun H : a = a => f_equal (fun x : A => x) H *)
Check fun H:?[x]=a :> A => f_equal (fun x:id A => x) H.
(* fun H : ?x = a => f_equal (fun x : id A => x) H *)@mattam82, @gares: Don't we want to aim a type class resolution which is independent on the order in which evar-evar are resolved? While I'm at it, @gares, you said: "When 2 evars are unified, the one that survives should have as a name the "union" of the two names". What is your idea to implement this? To attach list of names to evars? Should we make a union for the |
|
I think for resolvable indeed true should be the top value during merges. |
yes. more in general the entire "store" should be merged (i.e. it should require each data to come with a union operation or something along these lines). |
|
@gares: Yes, this looks like a good idea. Would you do it? |
|
No, I've no time now. |
|
I'll do it, after all I'm responsible for resolvable :) I'll work on the improved check for unresolved evars as well, it's badly needed in my unifall branch too. |
|
@mattam82: Test for unresolved evars: can you roughly sketch how it compares to tclWITHHOLES, if it is not taking too much time of you? |
|
@herbelin I'm looking into it right now so hopefully I'll have a PR ready soon. My first impression is that tclWITHHOLES should simply be refined actually, and I think some refactoring might be needed with check_evars_are_solved in pretyping, the two are performing very similar tests (and in this pose proof example, both are performed). |
|
Hmm, actually in your example it's not the resolvable flag which is at fault, but the fact that in one case the resulting undefined evar has type [A] and in the other [id A], and only the first is recognized by typeclass resolution as a class evar on which resolution can be launched. |
|
@mattam82: Ah, you mean that the decision to apply search for type classes instances is taken after the decision of solving the evar-evar equation. It is a bit more tricky to fix then. |
|
@mattam82: Maybe we can then take also the union of types, and an instance be searched if at least one of the types is a class? |
|
@herbelin yes, it's solved after the evar-evar solution. It's rather unfortunate but the test for an evar type to be a class or not is purely syntactic (no reduction), so we should maybe rely on a more semantic criterion (i.e. a tag on the evar, but that's not stable by instantiation of evars in the conclusion), or allow taking then whnf on the conclusion (but that will be source of incompatibilities and inefficiencies). I'm trying to figure out what to do actually. Looking at the union of types seems a bit too strong to me. |
|
What we would like eventually is that typeclass resolution is insensitive to the evar-evar solutions, so it should be insensitive to types convertible to a class. |
|
@mattam82: But a class can evaluate to another class, right? So, how to decide if a type convertible to a class is considered as related to one class or to the other. I vaguely remember that a problem of this kind happens with coercions. |
|
Indeed, the reduction should be parameterized at least not to unfold classes. |
The test is refined to handle aliases: i.e. undefined evars coming from restrictions and evar-evar unifications with an initial evar are not considered fresh unresolved evars. To check this, we generalize the restricted_evars set to an aliased_evars set in the evar map, registering evars being solved by another evar due to restriction or evar-evar unifications. This implements the proposal of PR rocq-prover#370 for testing the resolution status of evars independently of the evar-evar orientation order. This allows [apply] to refine an evar with a new one if it results from a [clear] request or an evar-evar solution only, otherwise the new evar is considered fresh and an error is raised. Also fixes bugs rocq-prover#4095 and rocq-prover#4413.
The test is refined to handle aliases: i.e. undefined evars coming from restrictions and evar-evar unifications with an initial evar are not considered fresh unresolved evars. To check this, we generalize the restricted_evars set to an aliased_evars set in the evar map, registering evars being solved by another evar due to restriction or evar-evar unifications. This implements the proposal of PR rocq-prover#370 for testing the resolution status of evars independently of the evar-evar orientation order. This allows [apply] to refine an evar with a new one if it results from a [clear] request or an evar-evar solution only, otherwise the new evar is considered fresh and an error is raised. Also fixes bugs rocq-prover#4095 and rocq-prover#4413.
The test is refined to handle aliases: i.e. undefined evars coming from restrictions and evar-evar unifications with an initial evar are not considered fresh unresolved evars. To check this, we generalize the restricted_evars set to an aliased_evars set in the evar map, registering evars being solved by another evar due to restriction or evar-evar unifications. This implements the proposal of PR rocq-prover#370 for testing the resolution status of evars independently of the evar-evar orientation order. This allows [apply] to refine an evar with a new one if it results from a [clear] request or an evar-evar solution only, otherwise the new evar is considered fresh and an error is raised. Also fixes bugs rocq-prover#4095 and rocq-prover#4413.
The test is refined to handle aliases: i.e. undefined evars coming from restrictions and evar-evar unifications with an initial evar are not considered fresh unresolved evars. To check this, we generalize the restricted_evars set to an aliased_evars set in the evar map, registering evars being solved by another evar due to restriction or evar-evar unifications. This implements the proposal of PR rocq-prover#370 for testing the resolution status of evars independently of the evar-evar orientation order. This allows [apply] to refine an evar with a new one if it results from a [clear] request or an evar-evar solution only, otherwise the new evar is considered fresh and an error is raised. Also fixes bugs rocq-prover#4095 and rocq-prover#4413.
The test is refined to handle aliases: i.e. undefined evars coming from restrictions and evar-evar unifications with an initial evar are not considered fresh unresolved evars. To check this, we generalize the restricted_evars set to an aliased_evars set in the evar map, registering evars being solved by another evar due to restriction or evar-evar unifications. This implements the proposal of PR rocq-prover#370 for testing the resolution status of evars independently of the evar-evar orientation order. This allows [apply] to refine an evar with a new one if it results from a [clear] request or an evar-evar solution only, otherwise the new evar is considered fresh and an error is raised. Also fixes bugs rocq-prover#4095 and rocq-prover#4413.
The test is refined to handle aliases: i.e. undefined evars coming from restrictions and evar-evar unifications with an initial evar are not considered fresh unresolved evars. To check this, we generalize the restricted_evars set to an aliased_evars set in the evar map, registering evars being solved by another evar due to restriction or evar-evar unifications. This implements the proposal of PR rocq-prover#370 for testing the resolution status of evars independently of the evar-evar orientation order. This allows [apply] to refine an evar with a new one if it results from a [clear] request or an evar-evar solution only, otherwise the new evar is considered fresh and an error is raised. Also fixes bugs rocq-prover#4095 and rocq-prover#4413.
The test is refined to handle aliases: i.e. undefined evars coming from restrictions and evar-evar unifications with an initial evar are not considered fresh unresolved evars. To check this, we generalize the restricted_evars set to an aliased_evars set in the evar map, registering evars being solved by another evar due to restriction or evar-evar unifications. This implements the proposal of PR rocq-prover#370 for testing the resolution status of evars independently of the evar-evar orientation order. This allows [apply] to refine an evar with a new one if it results from a [clear] request or an evar-evar solution only, otherwise the new evar is considered fresh and an error is raised. Also fixes bugs rocq-prover#4095 and rocq-prover#4413.
The test is refined to handle aliases: i.e. undefined evars coming from restrictions and evar-evar unifications with an initial evar are not considered fresh unresolved evars. To check this, we generalize the restricted_evars set to an aliased_evars set in the evar map, registering evars being solved by another evar due to restriction or evar-evar unifications. This implements the proposal of PR rocq-prover#370 for testing the resolution status of evars independently of the evar-evar orientation order. This allows [apply] to refine an evar with a new one if it results from a [clear] request or an evar-evar solution only, otherwise the new evar is considered fresh and an error is raised. Also fixes bugs rocq-prover#4095 and rocq-prover#4413.
|
This has been waiting for a loooong time. The unifall Arlésienne doesn't seem to get any closer, so I am forcefully closing this PR and let @herbelin reopen it when it finally lands. |
|
My overall analysis of the unifall Arlésienne is the following:
In my perception, one way to solve all three problems is first to get a large enough support on the following question: Are we ready, collectively, to make unifall and the question of unification algorithms a priority, meaning, are we ready to target and concentrate our force towards a unification model we are proud of, and to propose a transition model for those tactics impacted by moving to this new unification model (an option, an updating tool, ...)? Are we ready to share with @mattam82 the effort of completing the unifall program (as some of us already did)? In any case, we definitely cannot accept the current "technical debt" of |
I think we are clearly not ready, and this is what the last years have shown, and I don't think anything has changed about that. It seems that unless someone takes a strong leadership (like @ppedrot in the case of Ltac2, or @ejgallego in the Dune case) and is strongly supported by a few others which manage to take time to help (e.g. @maximedenes who integrated Ltac2 or myself who helped merge the Dune PRs), collective efforts are bound to fail. Diluted responsibility usually does not work. |
|
I'd like to see unification improved and unifall integrated. My time is short but we are many people in the team. Do you mean that we would not be able to organise ourselves to complete the effort (assuming that the importance of unifall is shared of course)? We managed to organise ourselves in moving the manual to sphinx, for instance. I don't think that collective efforts mean dilution of responsibility. We can take collective decisions about what to do and decide at the same time that someone is responsible for these decisions being brought to an end, no? |
The way that was done was pretty catastrophic. Don't get me started on that... I spent lots of time fixing the mess.
Absolutely. As long as there is someone shepherding the work (and shepherding by itself is a lot of work), collective efforts can succeed. I just meant collective works don't happen by pure magic. |
The test is refined to handle aliases: i.e. undefined evars coming from restrictions and evar-evar unifications with an initial evar are not considered fresh unresolved evars. To check this, we generalize the restricted_evars set to an aliased_evars set in the evar map, registering evars being solved by another evar due to restriction or evar-evar unifications. This implements the proposal of PR rocq-prover#370 for testing the resolution status of evars independently of the evar-evar orientation order. This allows [apply] to refine an evar with a new one if it results from a [clear] request or an evar-evar solution only, otherwise the new evar is considered fresh and an error is raised. Also fixes bugs rocq-prover#4095 and rocq-prover#4413.
The test is refined to handle aliases: i.e. undefined evars coming from restrictions and evar-evar unifications with an initial evar are not considered fresh unresolved evars. To check this, we generalize the restricted_evars set to an aliased_evars set in the evar map, registering evars being solved by another evar due to restriction or evar-evar unifications. This implements the proposal of PR rocq-prover#370 for testing the resolution status of evars independently of the evar-evar orientation order. This allows [apply] to refine an evar with a new one if it results from a [clear] request or an evar-evar solution only, otherwise the new evar is considered fresh and an error is raised. Also fixes bugs rocq-prover#4095 and rocq-prover#4413.
The test is refined to handle aliases: i.e. undefined evars coming from restrictions and evar-evar unifications with an initial evar are not considered fresh unresolved evars. To check this, we generalize the restricted_evars set to an aliased_evars set in the evar map, registering evars being solved by another evar due to restriction or evar-evar unifications. This implements the proposal of PR rocq-prover#370 for testing the resolution status of evars independently of the evar-evar orientation order. This allows [apply] to refine an evar with a new one if it results from a [clear] request or an evar-evar solution only, otherwise the new evar is considered fresh and an error is raised. Also fixes bugs rocq-prover#4095 and rocq-prover#4413.
The test is refined to handle aliases: i.e. undefined evars coming from restrictions and evar-evar unifications with an initial evar are not considered fresh unresolved evars. To check this, we generalize the restricted_evars set to an aliased_evars set in the evar map, registering evars being solved by another evar due to restriction or evar-evar unifications. This implements the proposal of PR rocq-prover#370 for testing the resolution status of evars independently of the evar-evar orientation order. This allows [apply] to refine an evar with a new one if it results from a [clear] request or an evar-evar solution only, otherwise the new evar is considered fresh and an error is raised. Also fixes bugs rocq-prover#4095 and rocq-prover#4413.
The test is refined to handle aliases: i.e. undefined evars coming from restrictions and evar-evar unifications with an initial evar are not considered fresh unresolved evars. To check this, we generalize the restricted_evars set to an aliased_evars set in the evar map, registering evars being solved by another evar due to restriction or evar-evar unifications. This implements the proposal of PR rocq-prover#370 for testing the resolution status of evars independently of the evar-evar orientation order. This allows [apply] to refine an evar with a new one if it results from a [clear] request or an evar-evar solution only, otherwise the new evar is considered fresh and an error is raised. Also fixes bugs rocq-prover#4095 and rocq-prover#4413.
The test is refined to handle aliases: i.e. undefined evars coming from restrictions and evar-evar unifications with an initial evar are not considered fresh unresolved evars. To check this, we generalize the restricted_evars set to an aliased_evars set in the evar map, registering evars being solved by another evar due to restriction or evar-evar unifications. This implements the proposal of PR rocq-prover#370 for testing the resolution status of evars independently of the evar-evar orientation order. This allows [apply] to refine an evar with a new one if it results from a [clear] request or an evar-evar solution only, otherwise the new evar is considered fresh and an error is raised. Also fixes bugs rocq-prover#4095 and rocq-prover#4413.
The test is refined to handle aliases: i.e. undefined evars coming from restrictions and evar-evar unifications with an initial evar are not considered fresh unresolved evars. To check this, we generalize the restricted_evars set to an aliased_evars set in the evar map, registering evars being solved by another evar due to restriction or evar-evar unifications. This implements the proposal of PR rocq-prover#370 for testing the resolution status of evars independently of the evar-evar orientation order. This allows [apply] to refine an evar with a new one if it results from a [clear] request or an evar-evar solution only, otherwise the new evar is considered fresh and an error is raised. Also fixes bugs rocq-prover#4095 and rocq-prover#4413.
The test is refined to handle aliases: i.e. undefined evars coming from restrictions and evar-evar unifications with an initial evar are not considered fresh unresolved evars. To check this, we generalize the restricted_evars set to an aliased_evars set in the evar map, registering evars being solved by another evar due to restriction or evar-evar unifications. This implements the proposal of PR rocq-prover#370 for testing the resolution status of evars independently of the evar-evar orientation order. This allows [apply] to refine an evar with a new one if it results from a [clear] request or an evar-evar solution only, otherwise the new evar is considered fresh and an error is raised. Also fixes bugs rocq-prover#4095 and rocq-prover#4413.
The test is refined to handle aliases: i.e. undefined evars coming from restrictions and evar-evar unifications with an initial evar are not considered fresh unresolved evars. To check this, we generalize the restricted_evars set to an aliased_evars set in the evar map, registering evars being solved by another evar due to restriction or evar-evar unifications. This implements the proposal of PR rocq-prover#370 for testing the resolution status of evars independently of the evar-evar orientation order. This allows [apply] to refine an evar with a new one if it results from a [clear] request or an evar-evar solution only, otherwise the new evar is considered fresh and an error is raised. Also fixes bugs rocq-prover#4095 and rocq-prover#4413.
The test is refined to handle aliases: i.e. undefined evars coming from restrictions and evar-evar unifications with an initial evar are not considered fresh unresolved evars. To check this, we generalize the restricted_evars set to an aliased_evars set in the evar map, registering evars being solved by another evar due to restriction or evar-evar unifications. This implements the proposal of PR rocq-prover#370 for testing the resolution status of evars independently of the evar-evar orientation order. This allows [apply] to refine an evar with a new one if it results from a [clear] request or an evar-evar solution only, otherwise the new evar is considered fresh and an error is raised. Also fixes bugs rocq-prover#4095 and rocq-prover#4413.
The test is refined to handle aliases: i.e. undefined evars coming from restrictions and evar-evar unifications with an initial evar are not considered fresh unresolved evars. To check this, we generalize the restricted_evars set to an aliased_evars set in the evar map, registering evars being solved by another evar due to restriction or evar-evar unifications. This implements the proposal of PR rocq-prover#370 for testing the resolution status of evars independently of the evar-evar orientation order. This allows [apply] to refine an evar with a new one if it results from a [clear] request or an evar-evar solution only, otherwise the new evar is considered fresh and an error is raised. Also fixes bugs rocq-prover#4095 and rocq-prover#4413.
The test is refined to handle aliases: i.e. undefined evars coming from restrictions and evar-evar unifications with an initial evar are not considered fresh unresolved evars. To check this, we generalize the restricted_evars set to an aliased_evars set in the evar map, registering evars being solved by another evar due to restriction or evar-evar unifications. This implements the proposal of PR rocq-prover#370 for testing the resolution status of evars independently of the evar-evar orientation order. This allows [apply] to refine an evar with a new one if it results from a [clear] request or an evar-evar solution only, otherwise the new evar is considered fresh and an error is raised. Also fixes bugs rocq-prover#4095 and rocq-prover#4413.
The test is refined to handle aliases: i.e. undefined evars coming from restrictions and evar-evar unifications with an initial evar are not considered fresh unresolved evars. To check this, we generalize the restricted_evars set to an aliased_evars set in the evar map, registering evars being solved by another evar due to restriction or evar-evar unifications. This implements the proposal of PR rocq-prover#370 for testing the resolution status of evars independently of the evar-evar orientation order. This allows [apply] to refine an evar with a new one if it results from a [clear] request or an evar-evar solution only, otherwise the new evar is considered fresh and an error is raised. Also fixes bugs rocq-prover#4095 and rocq-prover#4413. Co-authored-by: Maxime Dénès <maxime.denes@inria.fr>
The test is refined to handle aliases: i.e. undefined evars coming from restrictions and evar-evar unifications with an initial evar are not considered fresh unresolved evars. To check this, we generalize the restricted_evars set to an aliased_evars set in the evar map, registering evars being solved by another evar due to restriction or evar-evar unifications. This implements the proposal of PR rocq-prover#370 for testing the resolution status of evars independently of the evar-evar orientation order. This allows [apply] to refine an evar with a new one if it results from a [clear] request or an evar-evar solution only, otherwise the new evar is considered fresh and an error is raised. Also fixes bugs rocq-prover#4095 and rocq-prover#4413. Co-authored-by: Maxime Dénès <maxime.denes@inria.fr>
This is a regression wrt v8.4 related to the change of order of resolution of evar-evar unification problems.
One possible "fix" is to give priority to the instantiation of the non pre-existing evar to solve a ?x=?y problem. Note that it happens to incidentally fix also #4095!
Started a coq-contribs test but I feel this kind of "fix" should also be tested on contribs using type inference intensively [Edited afterwards: no failures on contribs here, nor for a simpler version based on trunk+4413-evar-evar-fix2 here].