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: the terminal refine linter #11890

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

Conversation

adomani
Copy link
Collaborator

@adomani adomani commented Apr 4, 2024

A linter that warns on usages of refine and refine' as a finishing tactic.

See this Zulip discussion.

Conclusion of the experiment

Systematic replacements of terminal refine with exact leads to an overall slow-down.


Open in Gitpod

@adomani
Copy link
Collaborator Author

adomani commented Apr 4, 2024

!bench

@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit 5f02e63.
There were significant changes against commit e545009:

  Benchmark                                                Metric         Change
  ==============================================================================
+ build                                                    linting         -5.2%
+ ~Mathlib.AlgebraicGeometry.Morphisms.RingHomProperties   instructions    -4.2%

@adomani adomani added awaiting-review The author would like community review of the PR t-linter Linter labels Apr 4, 2024
@adomani
Copy link
Collaborator Author

adomani commented Apr 4, 2024

Here are the benchmark results for commit 5f02e63. There were significant changes against commit e545009:

  Benchmark                                                Metric         Change
  ==============================================================================
+ build                                                    linting         -5.2%
+ ~Mathlib.AlgebraicGeometry.Morphisms.RingHomProperties   instructions    -4.2%

Do not be fooled by the green: the PR is largely a slowdown!

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Apr 5, 2024
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Apr 5, 2024
mathlib-bors bot pushed a commit that referenced this pull request Apr 5, 2024
See #11890 and this [Zulip discussion](https://leanprover.zulipchat.com/#narrow/stream/287929-mathlib4/topic/Usage.20of.20refine') for an in-depth explanation of why these `refine`s and not others.

The short answer is that the files in which these replacements took place were more performant after the change than before.
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Apr 5, 2024
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Apr 5, 2024
Louddy pushed a commit that referenced this pull request Apr 15, 2024
See #11890 and this [Zulip discussion](https://leanprover.zulipchat.com/#narrow/stream/287929-mathlib4/topic/Usage.20of.20refine') for an in-depth explanation of why these `refine`s and not others.

The short answer is that the files in which these replacements took place were more performant after the change than before.
atarnoam pushed a commit that referenced this pull request Apr 16, 2024
See #11890 and this [Zulip discussion](https://leanprover.zulipchat.com/#narrow/stream/287929-mathlib4/topic/Usage.20of.20refine') for an in-depth explanation of why these `refine`s and not others.

The short answer is that the files in which these replacements took place were more performant after the change than before.
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Apr 17, 2024
uniwuni pushed a commit that referenced this pull request Apr 19, 2024
See #11890 and this [Zulip discussion](https://leanprover.zulipchat.com/#narrow/stream/287929-mathlib4/topic/Usage.20of.20refine') for an in-depth explanation of why these `refine`s and not others.

The short answer is that the files in which these replacements took place were more performant after the change than before.
callesonne pushed a commit that referenced this pull request Apr 22, 2024
See #11890 and this [Zulip discussion](https://leanprover.zulipchat.com/#narrow/stream/287929-mathlib4/topic/Usage.20of.20refine') for an in-depth explanation of why these `refine`s and not others.

The short answer is that the files in which these replacements took place were more performant after the change than before.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review The author would like community review of the PR merge-conflict The PR has a merge conflict with master, and needs manual merging. t-linter Linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants