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: linter for bare open (scoped) Classical #21947

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

Conversation

grunweg
Copy link
Collaborator

@grunweg grunweg commented Feb 16, 2025

Add a syntax linter for open Classical and open scoped Classical which are not scoped to a single declaration.
Such occurrences have been tracked as technical debt, and recently been fully removed from mathlib.
This linter enforces this (and removes the corresponding technical debt metric).


TODO: does the function for finding opens already exist elsewhere?

Second version of #15454.

Open in Gitpod

Copy link

github-actions bot commented Feb 16, 2025

PR summary d0a98c7d58

Import changes for modified files

No significant changes to the import graph

Import changes for all files
Files Import difference

Declarations diff

+ bar
+ baz
+ extractOpenNames
+ openClassicalLinter

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).

/-- If `stx` is syntax describing an `open` command, `extractOpenNames stx`
returns an array of the syntax corresponding to the opened names,
omitting any renamed or hidden items. -/
def extractOpenNames : Syntax → Array Syntax
Copy link
Collaborator

Choose a reason for hiding this comment

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

I remember this function already existing, but I cannot find it. Do you know if it already exists and if maybe it can be recycled?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't remember seeing it, but have asked on zulip

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Status update: nobody responded on zulip --- perhaps it doesn't exist yet.

@jcommelin
Copy link
Member

Please update the PR description at some point 😉

@grunweg
Copy link
Collaborator Author

grunweg commented Feb 17, 2025

@adomani Do you have an idea why the tests fail? They don't seem to register the new lint at all.

I'm honest at a loss as to why this occurs.

@mathlib4-dependent-issues-bot mathlib4-dependent-issues-bot added the blocked-by-other-PR This PR depends on another PR to Mathlib (this label is automatically managed by a bot) label Feb 17, 2025
@adomani
Copy link
Collaborator

adomani commented Feb 17, 2025

I think that there is a hardcoded test exception that should be MathlibTest instead.

@mathlib4-dependent-issues-bot mathlib4-dependent-issues-bot removed the blocked-by-other-PR This PR depends on another PR to Mathlib (this label is automatically managed by a bot) label Feb 17, 2025
@grunweg
Copy link
Collaborator Author

grunweg commented Feb 17, 2025

I think that there is a hardcoded test exception that should be MathlibTest instead.

🤦 Oh wow, thanks for pointing out the tomatoes on my eyes! Upon a closer look, I decided to remove those exclusions wholesale: downstream projects can decide to activate the linter in their lakefile.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t-linter Linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants