Skip to content

[Merged by Bors] - feat(Tactic/Linter/DirectoryDependency): add an allow-list for directories with few imports#24109

Closed
grunweg wants to merge 33 commits intomasterfrom
MR-dirdep-allowlist
Closed

[Merged by Bors] - feat(Tactic/Linter/DirectoryDependency): add an allow-list for directories with few imports#24109
grunweg wants to merge 33 commits intomasterfrom
MR-dirdep-allowlist

Conversation

@grunweg
Copy link
Contributor

@grunweg grunweg commented Apr 16, 2025

Add an allow-list mechanism to the directoryDependency linter: modules covered by that list may only have imports listed in that list --- i.e., new top-level directory imports are forbidden by default.
To prove the implementation works, we add entries for the Lean, Util and Tactic/Linter directories in mathlib.
A future PR will convert the entries for e.g. Logic, Control or Testable to this allow-list.

This PR is a follow-up to #23088. Further refactorings (such as parsing the lists of exceptions from a JSON file)
can happen in subsequent PRs.


Open question: instead of manually filtering for imports of external projects, should I instead just lint only mathlib imports?

Future question: add further tests; this mechanism is somewhat undertested. I'm happy to work on this, but am not sure if this should block this PR. The lack of tests is certainly pre-existing.

Open in Gitpod

@grunweg grunweg added the t-linter Linter label Apr 16, 2025
@github-actions
Copy link

github-actions bot commented Apr 16, 2025

PR summary 05d0a12a07

Import changes exceeding 2%

% File
+100.00% Mathlib.Tactic.Linter.DirectoryDependency

Import changes for modified files

Dependency changes

File Base Count Head Count Change
Mathlib.Tactic.Linter.DirectoryDependency 1 2 +1 (+100.00%)
Import changes for all files
Files Import difference
Mathlib.Tactic.Linter.DirectoryDependency 1

Declarations diff

+ Lean.Name.prefix?
+ allowedImportDirs
+ containsKey
+ getAllLeft

You can run this locally as follows
## summary with just the declaration names:
./scripts/declarations_diff.sh <optional_commit>

## more verbose report:
./scripts/declarations_diff.sh long <optional_commit>

The doc-module for script/declarations_diff.sh contains some details about this script.


No changes to technical debt.

You can run this locally as

./scripts/technical-debt-metrics.sh pr_summary
  • The relative value is the weighted sum of the differences with weight given by the inverse of the current value of the statistic.
  • The absolute value is the relative value divided by the total sum of the inverses of the current values (i.e. the weighted average of the differences).

grunweg added 4 commits April 16, 2025 14:01
This provides prettier output (automatically newline-separated) and
also will show all errors for forbidden imports.
Need to go through a few more files and see if extra ones need to be adjusted/
some of these should be consolidated instead.

There's a bug in the logic, where some imports with a differently nested
prefix are ignored. Add a test for this, then fix it.
@github-actions github-actions bot added the large-import Automatically added label for PRs with a significant increase in transitive imports label Apr 16, 2025
@grunweg grunweg force-pushed the MR-dirdep-allowlist branch from 55c4c1b to 685b03b Compare April 16, 2025 17:30
@grunweg grunweg changed the title feat(Tactic/Linter/DirectoryDependency): POC of using an allow-list for selected directories feat(Tactic/Linter/DirectoryDependency): use an allow-list for selected directories' imports Apr 16, 2025
@grunweg grunweg added the awaiting-CI This PR does not pass CI yet. This label is automatically removed once it does. label Apr 16, 2025
@grunweg grunweg force-pushed the MR-dirdep-allowlist branch from e0b5eac to 4b47c9f Compare April 22, 2025 12:16
@grunweg grunweg force-pushed the MR-dirdep-allowlist branch from f43eb0d to 956c814 Compare April 22, 2025 19:37
@grunweg grunweg force-pushed the MR-dirdep-allowlist branch from 956c814 to ec911f8 Compare April 22, 2025 21:02
Copy link
Contributor

@Vierkantor Vierkantor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit worried about the large amount of messages being confusing. Especially since the warning for Mathlib.Util.AssertExists subsumes the warnings for the transitive imports of that file. Do you have an idea how to deduplicate that?

Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>
@grunweg
Copy link
Contributor Author

grunweg commented Apr 23, 2025

Good point about the large number of warnings. The linter code only sees the transitive imports in the end --- so just warning about user imports is not easy (and sometimes the "bad imports" really come in transitively). I can imagine a few ways of improving this experience:

  • if syntactical imports are not allowed, error about these first
  • cap the number of warnings to e.g. five and add a note "only showing the first 5 of N warnings"
  • only list the full import chain for the first 5 imports, but still list the remaining modules: not sure if that's helpful.

If the issue is e.g. aesop being imported, I am not interested in seeing warnings about all 300 modules individually. For mathlib modules, one could try matching the bad imports to top-level directories from the allow-list...

So, there are a few options. What do you think, which of these are appropriate as opposed to overkill?

@grunweg grunweg removed the awaiting-author A reviewer has asked the author a question or requested changes. label Apr 23, 2025
@Vierkantor
Copy link
Contributor

I think I have a working approach: when describing the import path, we can check if one of the modules is already forbidden. If so, we can drop the message since another error will supersede it. I took the liberty of pushing that change so you can try it out, feel free to modify/revert if you have better ideas!

@leanprover-community-bot-assistant
Copy link
Collaborator

This pull request has conflicts, please merge master and resolve them.

@leanprover-community-bot-assistant leanprover-community-bot-assistant added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label May 3, 2025
@grunweg
Copy link
Contributor Author

grunweg commented May 16, 2025

Sorry for the delay; I have too many things at once that I want my attention. I should hopefully be able to review your commit soon. Would you like to merge master, so this can then proceed quickly after that?

@github-actions github-actions bot removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label May 16, 2025
@Vierkantor Vierkantor force-pushed the MR-dirdep-allowlist branch from 59abca9 to b00cb81 Compare May 16, 2025 13:42
@kim-em
Copy link
Contributor

kim-em commented May 28, 2025

I'd be happy with this in as is. We can keep iterating.

bors d+

@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented May 28, 2025

✌️ grunweg can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@ghost ghost added the delegated This pull request has been delegated to the PR author (or occasionally another non-maintainer). label May 28, 2025
@grunweg
Copy link
Contributor Author

grunweg commented May 28, 2025

Thanks! I just checked and agree with the last commit.
bors merge

mathlib-bors bot pushed a commit that referenced this pull request May 28, 2025
…ories with few imports (#24109)

Add an allow-list mechanism to the `directoryDependency` linter: modules covered by that list may only have imports listed in that list 

Co-authored-by: Michael Rothgang <rothgang@math.uni-bonn.de>
Co-authored-by: grunweg <rothgami@math.hu-berlin.de>
@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented May 28, 2025

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title feat(Tactic/Linter/DirectoryDependency): add an allow-list for directories with few imports [Merged by Bors] - feat(Tactic/Linter/DirectoryDependency): add an allow-list for directories with few imports May 28, 2025
@mathlib-bors mathlib-bors bot closed this May 28, 2025
@mathlib-bors mathlib-bors bot deleted the MR-dirdep-allowlist branch May 28, 2025 08:34
bwehlin pushed a commit to bwehlin/mathlib4 that referenced this pull request May 31, 2025
…ories with few imports (leanprover-community#24109)

Add an allow-list mechanism to the `directoryDependency` linter: modules covered by that list may only have imports listed in that list 

Co-authored-by: Michael Rothgang <rothgang@math.uni-bonn.de>
Co-authored-by: grunweg <rothgami@math.hu-berlin.de>
joelriou pushed a commit that referenced this pull request Jun 8, 2025
…ories with few imports (#24109)

Add an allow-list mechanism to the `directoryDependency` linter: modules covered by that list may only have imports listed in that list 

Co-authored-by: Michael Rothgang <rothgang@math.uni-bonn.de>
Co-authored-by: grunweg <rothgami@math.hu-berlin.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

delegated This pull request has been delegated to the PR author (or occasionally another non-maintainer). large-import Automatically added label for PRs with a significant increase in transitive imports t-linter Linter tech debt Tracking cross-cutting technical debt, see e.g. the "Technical debt counters" stream on zulip

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants