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: enforce that Algebra.Notation imports no other files inside algebra #22725

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

grunweg
Copy link
Collaborator

@grunweg grunweg commented Mar 9, 2025

by extending the broadImports check of the header linter accordingly.
This enforces some of the rules set in #22640.


Open in Gitpod

inside Algebra

by extending the broadImports check of the header linter accordingly.
@grunweg grunweg added awaiting-CI t-linter Linter bench-after-CI Once the PR passes CI, comment `!bench` on the PR labels Mar 9, 2025
Copy link

github-actions bot commented Mar 9, 2025

PR summary a01470fa27

Import changes for modified files

No significant changes to the import graph

Import changes for all files
Files Import difference

Declarations diff

No declarations were harmed in the making of this PR! 🐙

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 grunweg requested review from YaelDillies and adomani March 9, 2025 08:05
@grunweg
Copy link
Collaborator Author

grunweg commented Mar 9, 2025

About the review requests:

  • @YaelDillies Can you check I got the logic exactly right, please?
  • @adomani Can you double-check the implementation?
    Both are hopefully quick, and not urgent from my side.

Copy link
Collaborator

@adomani adomani left a comment

Choose a reason for hiding this comment

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

Does this prevent also transitive importing? That is, a direct import will get flagged, but what if there is no direct import, but there is a transitive one?

I wonder whether this should be regulated by an extension of the assert_not_imported machinery that is directory-aware.

@leanprover-community-mathlib4-bot
Copy link
Collaborator

!bench

Copy link
Collaborator

@YaelDillies YaelDillies left a comment

Choose a reason for hiding this comment

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

Thanks

maintainer delegate

Copy link

github-actions bot commented Mar 9, 2025

🚀 Pull request has been placed on the maintainer queue by YaelDillies.

@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit f982972.
There were no significant changes against commit 2186c5f.

Copy link

github-actions bot commented Mar 9, 2025

File Instructions %
build +1.504⬝10⁹ (+0.00%)
CI run

@github-actions github-actions bot removed the bench-after-CI Once the PR passes CI, comment `!bench` on the PR label Mar 9, 2025
@grunweg grunweg changed the title feat: enforce that Algebra.Notation imports no other files inside algbera feat: enforce that Algebra.Notation imports no other files inside algebra Mar 9, 2025
@grunweg
Copy link
Collaborator Author

grunweg commented Mar 9, 2025

Does this prevent also transitive importing? That is, a direct import will get flagged, but what if there is not direct import, but there is a transitive one?

I wonder whether this should be regulated by an extension of the assert_not_inported machinery that is directory-aware.

Very good question! This only lints direct imports. I agree transitive imports should also be caught.

If you have a pointer to how to extend assert_not_imported, I'm happy to try. (This is not my highest priority though, so might take some time.) In the mean-time, we could land this with an issue about the generalisation - or wait for the better fix instead. Either way is fine with me.

@adomani
Copy link
Collaborator

adomani commented Mar 9, 2025

What if, instead of checking that there is an import ... syntax, you actually inspect docs#Lean.Environment.allImportedModuleNames, filter out the ones ones that should not be there and assert that the rest is empty?

This check should only run on the appropriate set of modules, so that, on most files, just the prefix check on the name will run.

@grunweg
Copy link
Collaborator Author

grunweg commented Mar 9, 2025

That's a great idea, thanks!

@grunweg grunweg added the awaiting-author A reviewer has asked the author a question or requested changes label Mar 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-author A reviewer has asked the author a question or requested changes maintainer-merge t-linter Linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants