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: macro scopes in Conv.congr #1955

Merged
merged 1 commit into from
Dec 14, 2022

Conversation

digama0
Copy link
Collaborator

@digama0 digama0 commented Dec 14, 2022

@Kha Kha enabled auto-merge (squash) December 14, 2022 15:44
@Kha
Copy link
Member

Kha commented Dec 14, 2022

Missing test output?

@digama0
Copy link
Collaborator Author

digama0 commented Dec 14, 2022

it takes me 20 minutes to compile and run the test anyway... I believe the test should result in no change to the output so I'm just optimistically running CI. (If the test fails then a panic message will be produced.)

@Kha
Copy link
Member

Kha commented Dec 14, 2022

Ah, wouldn't it be nicer though to also test that the goal tag looks as expected?

@digama0
Copy link
Collaborator Author

digama0 commented Dec 14, 2022

Well it's a hygienic name, so it's hard to test that it has the right scopes. This is an edge case anyway, so I'm happy as long as it doesn't panic.

@Kha
Copy link
Member

Kha commented Dec 14, 2022

It's not hygiened anymore after appendTag!

@digama0
Copy link
Collaborator Author

digama0 commented Dec 14, 2022

it should still be hygienic, with the same macro scopes as the original case tag. The bug occurs when you have both a hygienic base tag and want to append a hygienic field name to it. With the fix, the field name is no longer hygienic but the base tag still is, so you get a hygienic result.

@Kha
Copy link
Member

Kha commented Dec 14, 2022

No, I don't think goal tags are supposed to ever be hygienic, that's why appendTag is defined that way.
image

@Kha Kha merged commit 52d00e8 into leanprover:master Dec 14, 2022
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.

2 participants