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 dependent evars line with defined evars #18991

Merged
merged 1 commit into from May 4, 2024

Conversation

SkySkimmer
Copy link
Contributor

Not sure how much we care about this feature but as long as it's an easy fix we may as well.

Fix #18980

Not sure how much we care about this feature but as long as it's an
easy fix we may as well.

Fix coq#18980
@SkySkimmer SkySkimmer added kind: fix This fixes a bug or incorrect documentation. request: full CI Use this label when you want your next push to trigger a full CI. labels May 2, 2024
@SkySkimmer SkySkimmer requested a review from a team as a code owner May 2, 2024 12:53
@coqbot-app coqbot-app bot removed the request: full CI Use this label when you want your next push to trigger a full CI. label May 2, 2024
@SkySkimmer SkySkimmer added this to the 8.19.2 milestone May 2, 2024
@ppedrot
Copy link
Member

ppedrot commented May 2, 2024

I've been convinced for a while that this feature is an explicit breakage of invariants of the proof engine. Since the fix is local I agree with @SkySkimmer that it's fine to merge it, but future changes to the engine (e.g. a proper GC for the evar map) will likely break it again, and for good. I'd advise the maintainers of the UI relying on this to seriously start thinking about another approach.

@ppedrot ppedrot self-assigned this May 2, 2024
@hendriktews
Copy link

Thanks for working on this! The current version of the PR works on the example in #18980, however, it does not fix all problems with the dependent evar line, see #18980 for another example.

@hendriktews
Copy link

@ppedrot : I opened #19001 in order to discuss alternatives of the dependent evar line.

@ppedrot
Copy link
Member

ppedrot commented May 4, 2024

@coqbot merge now

@coqbot-app coqbot-app bot merged commit 6fdccaf into coq:master May 4, 2024
6 checks passed
@coqbot-app coqbot-app bot added this to Request 8.19.2 inclusion in Coq 8.19 May 4, 2024
@SkySkimmer SkySkimmer deleted the fix-dep-evars branch May 6, 2024 11:01
@coqbot-app coqbot-app bot moved this from Request 8.19.2 inclusion to Shipped in 8.19.2 in Coq 8.19 May 6, 2024
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
Coq 8.19
Shipped in 8.19.2
Development

Successfully merging this pull request may close these issues.

commit 5c94d0d375 breaks dependent evar line
3 participants