feat(Tactic/Linter/Header): make header linter work downstream#38396
feat(Tactic/Linter/Header): make header linter work downstream#38396kim-em wants to merge 2 commits intoleanprover-community:masterfrom
Conversation
The header linter hardcoded `Mathlib.lean` as the aggregator file to consult when deciding whether to inspect a file, so it silently exited early on any project depending on Mathlib (reported for CSLib in https://leanprover.zulipchat.com/#narrow/channel/287929-mathlib4/topic/usage.20of.20linter.2Estyle.2Eheader.20downstream). Replace `isInMathlib` with `isInLibraryRoot`, which takes the root of the current `mainModule` and looks for `<root>.lean`. This matches the standard Lean convention where a library named `Foo` has its aggregator at `Foo.lean`, so e.g. `Cslib.Foo.Bar` now consults `Cslib.lean`. Also generalise the "skip the aggregator itself" check from `mainModule == \`Mathlib` to `mainModule == mainModule.getRoot`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR summary c1e30e172cImport changes for modified filesNo significant changes to the import graph Import changes for all files
Declarations diff
You can run this locally as follows## summary with just the declaration names:
./scripts/pr_summary/declarations_diff.sh <optional_commit>
## more verbose report:
./scripts/pr_summary/declarations_diff.sh long <optional_commit>The doc-module for No changes to technical debt.This script lives in the
|
Per code review: the `mainModule == mainModule.getRoot` check is already covered by the earlier `inLibraryRoot?` gate in practice. Keep the check as defensive code for the edge case of a root module appearing in `headerTestFiles`, but update the comment to reflect that. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
This looks good to me at first glance. Once the cache is ready I'll try it out with CSLib as additional confirmation. Edit: This appears to be working as expected. |
|
It looks the build now fails on a header lint in |
| -- Defensive: skip linting the library root file itself. In practice the | ||
| -- `inLibraryRoot?` check above already covers this (a well-formed `<root>.lean` | ||
| -- does not import itself), but a root module could appear in `headerTestFiles`. |
There was a problem hiding this comment.
I don't find the wording "Defensive: " clear at all... let me double-check the comment being outdated: #38405
There was a problem hiding this comment.
Check successful: how rephrasing the comment as follows?
| -- Defensive: skip linting the library root file itself. In practice the | |
| -- `inLibraryRoot?` check above already covers this (a well-formed `<root>.lean` | |
| -- does not import itself), but a root module could appear in `headerTestFiles`. | |
| -- Skip linting the library root file itself. | |
| -- In practice, the `inLibraryRoot?` check above already covers this (a well-formed `<root>.lean` | |
| -- does not import itself), but a root module could appear in `headerTestFiles`. |
|
LGTM, with the one comment rephrased and the build failure fixed (i.e., that archive file getting a valid copyright header). Thanks for doing this! |
|
✌️ kim-em can now approve this pull request. To approve and merge a pull request, simply reply with |
This PR makes
linter.style.headerusable in projects downstream of Mathlib. PreviouslyisInMathlibhardcoded a lookup ofMathlib.lean, so the linter silently exited early in (e.g.) CSLib even when enabled vialinter.mathlibStandardSet.The fix uses the root of the current
mainModuleto find the aggregator file, soCslib.Foo.Barnow checksCslib.lean, matching the standard Lean convention that a library namedFoohas its root file atFoo.lean. The "skip the aggregator itself" check is generalised frommainModule == \MathlibtomainModule == mainModule.getRoot`.Reported by Chris Henson on Zulip; cc @chenson2018 @grunweg.
🤖 Prepared with Claude Code