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: allow dot idents to resolve to local names #6602

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cppio
Copy link
Contributor

@cppio cppio commented Jan 10, 2025

This PR allows the dot ident notation to resolve to the current definition, or to any of the other definitions in the same mutual block. Existing code that uses dot ident notation may need to have nonrec added if the ident has the same name as the definition.

Closes #6601

@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 Jan 10, 2025
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Jan 10, 2025
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jan 10, 2025
@cppio cppio closed this Jan 10, 2025
@leanprover-community-bot
Copy link
Collaborator

leanprover-community-bot commented Jan 10, 2025

Mathlib CI status (docs):

@leanprover-community-bot leanprover-community-bot added the breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan label Jan 10, 2025
nomeata
nomeata previously approved these changes Jan 10, 2025
Copy link
Collaborator

@nomeata nomeata left a comment

Choose a reason for hiding this comment

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

(nevermind, looks like there is more work to be done)

@nomeata nomeata self-requested a review January 10, 2025 21:08
@cppio
Copy link
Contributor Author

cppio commented Jan 10, 2025

This causes breakage in Batteries and Mathlib due to code such as https://github.com/leanprover-community/batteries/blob/66225aab4f6bd1687053b03916105f7cab140507/Batteries/Data/UnionFind/Lemmas.lean#L100-L101 which rely on the dot ident not resolving to the current definition. This can easily be fixed by using nonrec, which the linked code already does for rfl.

@cppio cppio reopened this Jan 10, 2025
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Jan 10, 2025
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jan 10, 2025
@nomeata
Copy link
Collaborator

nomeata commented Jan 10, 2025

Thanks for the investigation. Can you add this analysis to #6601, ideally with a self-contained example of the idiom that this fix would be breaking (so that we can use it for the test suite)? Under which circumstaces can .foo refer to both an existing constant and the current definitions?

Also, if possible, contrast the behavior with the a.foo notation for A.foo (a : A); ideally .foo and a.foo resolution behave the same with regard to recursive definitions.

@nomeata nomeata dismissed their stale review January 10, 2025 22:03

(invalidated by breakage)

@nomeata nomeata removed their request for review January 10, 2025 22:04
cppio added a commit to leanprover-community/mathlib4 that referenced this pull request Jan 11, 2025
@nomeata nomeata added the awaiting-mathlib We should not merge this until we have a successful Mathlib build label Jan 11, 2025
@cppio
Copy link
Contributor Author

cppio commented Jan 11, 2025

changelog-language

@github-actions github-actions bot added the changelog-language Language features, tactics, and metaprograms label Jan 11, 2025
@cppio cppio marked this pull request as ready for review January 11, 2025 12:39
cppio added a commit to cppio/batteries that referenced this pull request Jan 11, 2025
@nomeata
Copy link
Collaborator

nomeata commented Jan 11, 2025

Please ping me here once mathlib is green (this likely requires pinning the mathlib adaption branch to the fixed batteries branch that you created, but should be possible)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-mathlib We should not merge this until we have a successful Mathlib build breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan changelog-language Language features, tactics, and metaprograms 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.

dot ident notation cannot refer to recursive call
3 participants