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: importModules without loading environment extensions #6325

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

Conversation

Kha
Copy link
Member

@Kha Kha commented Dec 6, 2024

This PR ensures that environments can be loaded, repeatedly, without executing arbitrary code

@Kha Kha requested a review from tydeu as a code owner December 6, 2024 09:35
@Kha Kha added the changelog-language Language features, tactics, and metaprograms label Dec 6, 2024
@Kha Kha marked this pull request as draft December 6, 2024 09:37
@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 Dec 6, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Dec 6, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Dec 6, 2024
@leanprover-community-bot leanprover-community-bot added the builds-mathlib CI has verified that Mathlib builds against this PR label Dec 6, 2024
@leanprover-community-bot
Copy link
Collaborator

leanprover-community-bot commented Dec 6, 2024

Mathlib CI status (docs):

@Kha Kha force-pushed the push-mtllrxtzuprp branch from 570f669 to 81447dd Compare December 6, 2024 12:21
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Dec 6, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Dec 6, 2024
@leanprover-community-bot leanprover-community-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 Dec 6, 2024
@tydeu
Copy link
Member

tydeu commented Dec 6, 2024

@Kha mkInitialExtensionStates also runs arbitrary I/O. Thus, the entire setup of environment extensions would need to be disabled to avoid this.

Also, what is the reason for privatizing finalizeImport? Downstream users may need to reimplement importModules manually and have it public is key to that. For instance, loading plugins (and then configuring any related settings) needs to be done in the withImporting block.

EDIT: Personally, I would suggest splitting finalizeImport into two separate definitions: importConstants and initializeExtensions.

@Kha
Copy link
Member Author

Kha commented Dec 9, 2024

mkInitialExtensionStates also runs arbitrary I/O

But we're not loading any non-builtin extensions when initializers are disabled, as discussed before.

For instance, loading plugins (and then configuring any related settings) needs to be done in the withImporting block.

So --plugin is broken?

@digama0
Copy link
Collaborator

digama0 commented Dec 9, 2024

Also, what is the reason for privatizing finalizeImport? Downstream users may need to reimplement importModules manually and have it public is key to that. For instance, loading plugins (and then configuring any related settings) needs to be done in the withImporting block.

Agreed, this would be really annoying. I am calling that function in lean4lean, this will need me to copy paste a lot of code.

@Kha Kha force-pushed the push-mtllrxtzuprp branch from 81447dd to 0c4532f Compare December 9, 2024 11:26
@Kha
Copy link
Member Author

Kha commented Dec 9, 2024

I didn't reply to that part because I was already reverting it :)

leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Dec 9, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Dec 9, 2024
@leanprover-community-bot leanprover-community-bot added builds-mathlib CI has verified that Mathlib builds against this PR and removed breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan labels Dec 9, 2024
@tydeu
Copy link
Member

tydeu commented Dec 9, 2024

@Kha

mkInitialExtensionStates also runs arbitrary I/O

But we're not loading any non-builtin extensions when initializers are disabled, as discussed before.

True, so it is currently safe. It does mean that we have to continually be careful to not introduce any opportunity for arbitrary I/O in a mkInitial going forward. In my opinion, it seems easier (and more principled) to just avoid initializing extensions altogether in sensitive cases (thus avoiding future footguns).

For instance, loading plugins (and then configuring any related settings) needs to be done in the withImporting block.

So --plugin is broken?

Sorry, I should have been more specific. Only loading plugins in Lean code (via Lean.loadPlugin) needs to be done in a withImporting block (to enable Environment-based initializers that require Lean.initializing). --plugin works because it is run during IO.initializing (which also implies Lean.initializing).

@Kha
Copy link
Member Author

Kha commented Dec 9, 2024

@digama0 Actually could you explain what importModulesCore is used for in lean4{lean,checker} that couldn't be done with importModules mod.imports?

@Kha Kha force-pushed the push-mtllrxtzuprp branch from 0c4532f to 542c80d Compare December 10, 2024 10:55
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Dec 10, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Dec 10, 2024
@Kha Kha force-pushed the push-mtllrxtzuprp branch 2 times, most recently from cb6d977 to 8e6f0cc Compare December 10, 2024 12:55
@Kha Kha force-pushed the push-mtllrxtzuprp branch from 8e6f0cc to e5b8ce6 Compare December 10, 2024 12:59
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Dec 10, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Dec 10, 2024
@digama0
Copy link
Collaborator

digama0 commented Dec 10, 2024

I forget all the details, but looking at it now it seems to be a one-step unrolling of importModulesCore #[{module}] . That is, it is creating an environment that uses the target file as its only import, rather than creating an environment prepared for the imports of the target file and doing the target file via the normal elaboration pathway. But for lean4lean I think this is still suboptimal and I want a version of this function that I can prove properties about, so I think (1) importModules mod.imports (loadExts := false) is close enough to what I want for now and (2) eventually this will be replaced by a manual reimplementation that moves everything into pure code so I can prove theorems about it, and the main thing lean core can do to help here is to factor things out into pure and impure parts where sensible (so I eagerly await your Lean.Kernel.Environment PR #5145).

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 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.

4 participants