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

chore: collectForwardDeps returns only free variables, not metavars #6088

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

JovanGerb
Copy link
Contributor

This PR removes some unreachable code that acts as if collectForwardDeps could return metavariables. This confusing code was first added in commit 50ae170. And then to explain this, in #1625 it was explicitly documented that collectForwardDeps could return metavariables.

So I removed the metavariable handling in the functions that are only run on the result of collectForwardDeps.

It may also be a bug that no metavariables can reach this code, but in that case there should be a TODO.

@github-actions github-actions bot added the toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN label Nov 15, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Nov 15, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Nov 15, 2024
@leanprover-community-bot leanprover-community-bot added the builds-mathlib CI has verified that Mathlib builds against this PR label Nov 15, 2024
@leanprover-community-bot
Copy link
Collaborator

leanprover-community-bot commented Nov 15, 2024

Mathlib CI status (docs):

@nomeata
Copy link
Collaborator

nomeata commented Nov 22, 2024

I’ll leave others to review this (but looks good if it’s really dead code). Could you maybe leave assert! x.isVar statements in place, so that the next time someone reads the code they benefit from your findings, and we notices when they are violated?

@leanprover-bot leanprover-bot added the P-medium We may work on this issue if we find the time label Nov 22, 2024
@JovanGerb
Copy link
Contributor Author

Ok, I've added assert! toRevert.all (·.isFVar) for the result of collectForwardDeps, which is the the array passed to the other functions. (although I don't this this is reaally necessary, since otherwise there would be a panic from Expr.fvarId!)

leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Nov 24, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Nov 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builds-mathlib CI has verified that Mathlib builds against this PR P-medium We may work on this issue if we find the time toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants