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

feat: make Environment.mk private #2604

Merged
merged 3 commits into from
Oct 4, 2023
Merged

Conversation

digama0
Copy link
Collaborator

@digama0 digama0 commented Sep 29, 2023

As one follow-up to the discussion here and here, one easy way to make it harder for tactic-writers to accidentally modify the environment to add unchecked constants is to make the constructor private.

Against my expectations, this broke literally nothing in lean itself, and only one test (which ironically was testing private constructors); this was switched to use the API function and everything else worked. We'll see what the mathlib breakage is.

@digama0 digama0 requested a review from leodemoura September 29, 2023 23:56
@digama0 digama0 added awaiting-review Waiting for someone to review the PR awaiting-mathlib We should not merge this until we have a successful Mathlib build labels Sep 29, 2023
@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 Sep 30, 2023
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Sep 30, 2023
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the builds-mathlib CI has verified that Mathlib builds against this PR label Sep 30, 2023
@leanprover-community-mathlib4-bot
Copy link
Collaborator

leanprover-community-mathlib4-bot commented Sep 30, 2023

@SchrodingerZhu
Copy link

How can I obtain an empty Environment for mock testing?

@kim-em
Copy link
Collaborator

kim-em commented Oct 3, 2023

How can I obtain an empty Environment for mock testing?

Would using Environment.mk via open private from Std suffice for your use case?

@kim-em kim-em added awaiting-author Waiting for PR author to address issues and removed awaiting-review Waiting for someone to review the PR labels Oct 3, 2023
@kim-em kim-em self-assigned this Oct 3, 2023
@kim-em kim-em removed the awaiting-mathlib We should not merge this until we have a successful Mathlib build label Oct 3, 2023
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Oct 3, 2023
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan and removed builds-mathlib CI has verified that Mathlib builds against this PR labels Oct 3, 2023
@SchrodingerZhu
Copy link

How can I obtain an empty Environment for mock testing?

Would using Environment.mk via open private from Std suffice for your use case?

Interesting, just learned that private is not definite Lean.

@kim-em
Copy link
Collaborator

kim-em commented Oct 3, 2023

How can I obtain an empty Environment for mock testing?

@SchrodingerZhu, sorry, I gave you the wrong answer previously: you can use Lean.mkEmptyEnvironment.

@kim-em
Copy link
Collaborator

kim-em commented Oct 3, 2023

Making Environment.add private is more complicated: it currently breaks Aesop, but if fixable by using the "replay" functionality of lean4checker directly.

In order to keep things moving, I'm now going to revert the commit to this PR in which I made Environment.add private --- we'll do that later.

@kim-em
Copy link
Collaborator

kim-em commented Oct 3, 2023

@digama0, if you're happy with my proposed change to the comment, I can merge this.

leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Oct 3, 2023
@digama0
Copy link
Collaborator Author

digama0 commented Oct 4, 2023

Hm, the Environment.add part was a mistake, it should be private as well. Can we go forward with the change and calling it via extern as you did? (It would be good if extern was flagged a bit more clearly as unsafe, but it seems visible enough for review purposes. Is there a reason the same technique doesn't work for Aesop?)

@Kha
Copy link
Member

Kha commented Oct 4, 2023

Is there a reason the same technique doesn't work for Aesop?

The interpreter only knows how to handle [extern] from compiled modules

@kim-em
Copy link
Collaborator

kim-em commented Oct 4, 2023

My suggestion is that we merge as is, and then when #2617 is merged Aesop can use that, and then we can make Environment.add private as well.

@kim-em kim-em merged commit 89b65c8 into leanprover:master Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-author Waiting for PR author to address issues breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan 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.

5 participants