Skip to content

feat: importModules without loading environment extensions#6325

Merged
kim-em merged 2 commits intoleanprover:masterfrom
Kha:push-mtllrxtzuprp
Apr 2, 2025
Merged

feat: importModules without loading environment extensions#6325
kim-em merged 2 commits intoleanprover:masterfrom
Kha:push-mtllrxtzuprp

Conversation

@Kha
Copy link
Copy Markdown
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 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
ghost pushed a commit to leanprover-community/batteries that referenced this pull request Dec 6, 2024
ghost pushed a commit to leanprover-community/mathlib4 that referenced this pull request Dec 6, 2024
@ghost ghost added the builds-mathlib CI has verified that Mathlib builds against this PR label Dec 6, 2024
@ghost
Copy link
Copy Markdown

ghost 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
ghost pushed a commit to leanprover-community/batteries that referenced this pull request Dec 6, 2024
ghost pushed a commit to leanprover-community/mathlib4 that referenced this pull request Dec 6, 2024
@ghost ghost 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

Kha commented Dec 9, 2024

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

ghost pushed a commit to leanprover-community/batteries that referenced this pull request Dec 9, 2024
ghost pushed a commit to leanprover-community/mathlib4 that referenced this pull request Dec 9, 2024
@ghost ghost 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
Copy Markdown
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
Copy Markdown
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
ghost pushed a commit to leanprover-community/batteries that referenced this pull request Dec 10, 2024
ghost pushed 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
ghost pushed a commit to leanprover-community/mathlib4 that referenced this pull request Dec 10, 2024
@digama0
Copy link
Copy Markdown
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).

@eric-wieser
Copy link
Copy Markdown
Contributor

leanprover-community/mathlib4#20355 should stop the bleeding downstream of Mathlib until this lands.

mathlib-bors bot pushed a commit to leanprover-community/mathlib4 that referenced this pull request Jan 3, 2025
These are not supported unless `enableInitializersExecution` is called, and various downstream scripts do not call this.

https://leanprover.zulipchat.com/#narrow/channel/270676-lean4/topic/lean4checker.20failure/near/486191171 has some context for the underlying issue, which seems to be that Lean tries to finalize environment extensions even if initializers are disabled, leading to crashes if those extensions (such as the parser alias table) expect initializers to have run.

To prove this works, this removes `Lean.enableInitializersExecution` from the script to check the yaml files.
This is what we will want to do anyway after leanprover/lean4#6325 lands, as this script should not need to run any extensions.
@Kha Kha marked this pull request as ready for review March 27, 2025 09:43
ghost pushed a commit to leanprover-community/batteries that referenced this pull request Mar 27, 2025
ghost pushed a commit to leanprover-community/mathlib4 that referenced this pull request Mar 27, 2025
kim-em added a commit to leanprover-community/mathlib4 that referenced this pull request Mar 31, 2025
@kim-em kim-em added this pull request to the merge queue Apr 2, 2025
Merged via the queue into leanprover:master with commit 5df4e48 Apr 2, 2025
18 checks passed
@Kha Kha deleted the push-mtllrxtzuprp branch April 2, 2025 09:48
github-actions bot pushed a commit that referenced this pull request Apr 2, 2025
This PR ensures that environments can be loaded, repeatedly, without
executing arbitrary code

(cherry picked from commit 5df4e48)
kim-em pushed a commit that referenced this pull request Apr 2, 2025
This PR ensures that environments can be loaded, repeatedly, without
executing arbitrary code

(cherry picked from commit 5df4e48)
kim-em added a commit to leanprover/lean4checker that referenced this pull request Apr 3, 2025
mathlib-bors bot pushed a commit to leanprover-community/mathlib4 that referenced this pull request Oct 13, 2025
… correctly (#29513)

Parse options and load linter set extension correctly.

This fixes two issues:
- We never called the `toOptions` function that parses Lakefile options into a `Lean.Options` structure: the function we use instead is only meant for command-line options and doesn't handle the `weak.` prefix correctly.
- The way we load linter sets was incorrect: `withImportModules` stopped updating environment extensions since [lean4#6325](leanprover/lean4#6325), and that is exactly what we use to store linter sets. At the time of writing, `importModules` might be a suitable replacement (but this is not very safe, and not documented to be stable). Instead, write a small elaborator returning the environment at the point of elaboration, and use that instead.

Co-authored-by: Anne C.A. Baanen <vierkantor@vierkantor.com>
Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>
Paul-Lez pushed a commit to google-deepmind/formal-conjectures that referenced this pull request Oct 23, 2025
The `overwrite_index.lean` script failed to count attributes after a
Lean version bump. This was due to changes in how module imports handle
environment extensions, as introduced in
leanprover/lean4#6325 .

`Lean.withImportModules` no longer loads environment extensions. This
commit refactors the `runWithImports` function to use the lower-level
`importModulesCore` and `finalizeImport` functions, explicitly setting
`loadExts := true` to ensure attributes are loaded and counted
correctly.
BeibeiX0 pushed a commit to BeibeiX0/mathlib4 that referenced this pull request Nov 7, 2025
… correctly (leanprover-community#29513)

Parse options and load linter set extension correctly.

This fixes two issues:
- We never called the `toOptions` function that parses Lakefile options into a `Lean.Options` structure: the function we use instead is only meant for command-line options and doesn't handle the `weak.` prefix correctly.
- The way we load linter sets was incorrect: `withImportModules` stopped updating environment extensions since [lean4#6325](leanprover/lean4#6325), and that is exactly what we use to store linter sets. At the time of writing, `importModules` might be a suitable replacement (but this is not very safe, and not documented to be stable). Instead, write a small elaborator returning the environment at the point of elaboration, and use that instead.

Co-authored-by: Anne C.A. Baanen <vierkantor@vierkantor.com>
Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>
kim-em added a commit that referenced this pull request Feb 17, 2026
The module system draft was already covered in the v4.27.0 release
notes (PR #11637). The `importModules`/`finalizeImport` `loadExts`
change was already released in v4.20.0 (PR #6325). Neither belongs
in the upcoming release notes.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-merge-queue bot pushed a commit that referenced this pull request Feb 17, 2026
The `releases_drafts/` folder contained two entries that were already
covered in earlier releases:

- `module-system.md` — the module system was stabilized in v4.27.0
(#11637)
- `environment.md` — the `importModules`/`finalizeImport` `loadExts`
change landed in v4.20.0 (#6325)

Discovered while preparing the v4.29.0-rc1 release notes.

🤖 Prepared with Claude Code

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport releases/v4.19.0 builds-mathlib CI has verified that Mathlib builds against this PR changelog-language Language features 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.

5 participants