-
Notifications
You must be signed in to change notification settings - Fork 27
feat: add Cslib.Init module and checkInitImports linter #131
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
Conversation
This commit implements Cslib.Init following Mathlib's pattern, along with a checkInitImports linter to ensure modules are connected to the library's import graph. Changes: - Created Cslib/Init.lean importing Mathlib.Init and Cslib.Foundations.Lint.Basic - Added checkInitImports environment linter to Cslib/Foundations/Lint/Basic.lean - Created scripts/check-init-imports.lean for text-based CI validation - Updated lakefile.toml comments - Added import Cslib.Init to 10 foundational modules (3 Syntax files + 7 others) Files excluded from import due to technical constraints: - Cslib.Foundations.Lint.Basic: Circular dependency (imported by Cslib.Init) - Cslib.Foundations.Data.FinFun: Notation conflict with Mathlib.Finsupp (both use →₀) - Cslib.Foundations.Semantics.LTS.Basic: Causes type elaboration issues in downstream files - Cslib.Logics.LinearLogic.CLL.Basic: Syntax elaboration conflicts TODO comments have been added to document these issues for future investigation.
4966ada to
ece20ae
Compare
This commit addresses feedback from @chenson2018 on PR leanprover#131: 1. Removed the environment linter approach - Environment linters fire on every declaration, causing spam - Import connectivity is a whole-graph property, not per-declaration 2. Improved the script to use Mathlib's findImports - Now uses Lean.parseImports' internally (robust parser) - Replaced brittle text parsing with proper Lean parser 3. Updated lakefile.toml comments - Clarified that we use a script-based approach, not env linter - Matches Mathlib's pattern (script-based, not environment linter) The script works correctly and identifies the 4 modules that don't import any Cslib module (as documented in the PR description).
Add an allowlist of 4 modules that cannot import Cslib.Init due to technical constraints: - Cslib.Foundations.Lint.Basic: Circular dependency (imported by Init) - Cslib.Foundations.Data.FinFun: Notation conflict with Mathlib (→₀) - Cslib.Foundations.Semantics.LTS.Basic: Type elaboration issues - Cslib.Logics.LinearLogic.CLL.Basic: Syntax elaboration conflicts This follows Mathlib's pattern, which also maintains an allowlist of modules excluded from their missingInitImports check. The script now passes in CI while still catching new orphaned modules. Tested by creating a test orphan module - correctly detected.
Refactor to match Mathlib's exact pattern of using chained .erase calls instead of an upfront allowedExclusions array. This mirrors how Mathlib's missingInitImports handles exceptions. Tested: script still passes and correctly detects new orphan modules.
|
Thanks! I think I've addressed the feedback. Can you take another look? |
Remove `Elab Command` opens that were added for the environment linter but are no longer needed after its removal.
Revert comment-only changes to simplify PR review. The configuration itself is unchanged.
Move copyright header before the import statement to follow standard Lean file convention.
|
Thanks for the work, I think this is moving in the right direction! A couple of follow up questions. Regarding the logic for checking imports, let me make sure I have it straight:
With the exceptions existing though, do you need to change I also don't think you've made It would also be nice to go ahead and address #125 here since it affects the logic of checking |
Replace pattern matching on name components with the more idiomatic Lean.Name.getRoot method.
- Refactor CheckInitImports with clearer exception handling - Add CheckCslibComplete to verify all modules in Cslib.lean (leanprover#125) - Create RunTests driver to execute both checks via `lake test` - Rename scripts to PascalCase for lakefile.toml compatibility
Thanks! I've refactored the import checking logic to filter out exceptions upfront, and added that to |
- Use filter instead of imperative loops - Add Cslib itself to exceptions list - Remove issue reference from documentation
|
I just pushed a commit that made some alterations to I will take a look at |
|
@jessealama Okay, I think we are nearly done here. I would appreciate you taking a look and testing. Here are the highlights of what I just pushed:
I am pretty happy with everything here. My only hesitation is being unsure about the setup of the test runner, which feels a bit nonstandard in how it includes a |
|
One final consideration: do we actually remove the
|
|
@fmontesi Can you please review this before merging anything else? Because a change to testing merged at the same time as some other PRs, there are some failures on |
Closes #122
This PR implements
Cslib.Initfollowing Mathlib's pattern, along with acheckInitImportslinter to ensure modules are connected to the library's import graph.Changes
Cslib/Init.leanimportingMathlib.InitandCslib.Foundations.Lint.BasiccheckInitImportsenvironment linterlake testvia new test scripts inscripts/import Cslib.Initto 10 foundational modulesFiles excluded from import
Some files could not import
Cslib.Initdue to technical constraints:Cslib.Foundations.Lint.Basic: Circular dependency (imported byCslib.Init)Cslib.Foundations.Data.FinFun: Notation conflict withMathlib.Finsupp(both use→₀)Cslib.Foundations.Semantics.LTS.Basic: Causes type elaboration issues in downstream Automata filesCslib.Logics.LinearLogic.CLL.Basic: Syntax elaboration conflictsTODO comments have been added to document these issues for future investigation.