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 sigma pruning when calling ltac, reachability is not correct #581

Merged
merged 2 commits into from
Jan 29, 2024

Conversation

gares
Copy link
Contributor

@gares gares commented Jan 26, 2024

to be checked: examples/example_abs_evars.v

@gares
Copy link
Contributor Author

gares commented Jan 26, 2024

@Tragicus this new test seems more correct. Can you try it in your example? (After all I can't completely remove the pruning as we were doing this afternoon).

debug Pp.(fun () -> str "Goals: " ++ prlist_with_sep spc Evar.print declared_goals);
debug Pp.(fun () -> str "Shelved Goals: " ++ prlist_with_sep spc Evar.print shelved_goals);
Evd.fold_undefined (fun k _ sigma ->
if Evar.Set.mem k all_goals || Evd.mem sigma0 k then sigma
if Evar.Set.mem k reachable_undefined_evars then sigma
else Evd.remove sigma k
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry for hijacking this PR, but you definitely shouldn't reimplement your own pruning in a plugin. As a matter of fact, I wonder why Evd.remove is not marked as deprecated in the Coq API as you can easily wreak havoc (and definitely will if people use additional Store primitives in an evarmap).

We should instead provide a pruning function directly with Evd and ask in the API when creating user-defined evar data to provide functions explaining how the new data creates pointers to other blocks. It is more than time to realize that an evarmap is a proper heap...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm all for a heap that comes with a garbage collector, but currently it does not.
Worse, there is code that insists all your allocations are pointed to by the root, that is, that you don't leave garbage.

I'm happy to use any API you provide.

After this is merged, we can see if it also fixes the other bug affecting the TC solver.

@Tragicus
Copy link
Contributor

@Tragicus this new test seems more correct. Can you try it in your example? (After all I can't completely remove the pruning as we were doing this afternoon).

It works.

@gares gares merged commit 46f23b6 into master Jan 29, 2024
16 of 18 checks passed
@gares gares deleted the fix-evar-loss branch January 29, 2024 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants