feat(Tactic/Linter): add blanketSimpArgs linter - #42056
Conversation
Welcome new contributor!Thank you for contributing to Mathlib! If you haven't done so already, please review our contribution guidelines, as well as the style guide and naming conventions. In particular, we kindly remind contributors that we have guidelines regarding the use of AI when making pull requests. We use a review queue to manage reviews. If your PR does not appear there, it is probably because it is not successfully building (i.e., it doesn't have a green checkmark), has the If you haven't already done so, please come to https://leanprover.zulipchat.com/, introduce yourself, and mention your new PR. Thank you again for joining our community. |
|
!bench |
|
Benchmark results for ded55c4 against 6c5a908 are in. There are significant results. @sgraf812
Large changes (1🟥)
Medium changes (3✅)
Small changes (3✅)
|
|
t-meta |
PR summary f2939c5ec4
|
| Files | Import difference |
|---|---|
| ../mathlib-ci/scripts/pr_summary/import_trans_difference.sh all | |
| There are 8243 files with changed transitive imports taking up over 359603 characters: this is too many to display! | |
You can run this locally from your mathlib4 directory: |
git clone https://github.com/leanprover-community/mathlib-ci.git ../mathlib-ci
Declarations diff (regex)
+ blanketSimpArgsLinter
+ cache,
+ eq_nil_of_isEmpty
+ fallback
+ isBlanketRewrite
+ myId
+ rewriteSource?
+ simpRwKind
You can run this locally as follows
## from your `mathlib4` directory:
git clone https://github.com/leanprover-community/mathlib-ci.git ../mathlib-ci
## summary with just the declaration names:
../mathlib-ci/scripts/pr_summary/declarations_diff.sh <optional_commit>
## more verbose report:
../mathlib-ci/scripts/pr_summary/declarations_diff.sh long <optional_commit>The doc-module for scripts/pr_summary/declarations_diff.sh in the mathlib-ci repository contains some details about this script.
Declarations diff (Lean)
✅ Lean-aware diff — post-build, computed from the Lean environment (commit
f2939c5).
- +2 new declarations
- −0 removed declarations
+Mathlib.Linter.BlanketSimpArgs.simpRwKind
+Mathlib.Linter.linter.blanketSimpArgsNo changes to strong technical debt.
No changes to weak technical debt.
Current commit f2939c5ec4
Reference commit 9fb10993c1
This script lives in the mathlib-ci repository. To run it locally, from your mathlib4 directory:
git clone https://github.com/leanprover-community/mathlib-ci.git ../mathlib-ci
../mathlib-ci/scripts/reporting/technical-debt-metrics.sh pr_summary
- The
relativevalue is the weighted sum of the differences with weight given by the inverse of the current value of the statistic. - The
absolutevalue is therelativevalue divided by the total sum of the inverses of the current values (i.e. the weighted average of the differences).
|
Note to reviewers: I haven't reviewed the LLM-generated linter proposed here. If the reviewers find such a linter generally useful, then I would be willing to give it a closer look and polish before seriously submitting it for review. Hence I declare the initial round of review here, specifically this comment as easy |
|
!bench |
|
Benchmark results for 7c523de against 6c5a908 are in. There are significant results. @sgraf812
Large changes (1🟥)
Medium changes (3✅)
Small changes (3✅)
|
|
Nice find; thanks for making this linter! I agree this is a potential performance footgun, so we should have such a linter. I wonder, though: can the root cause
be fixed in Lean core instead? That would be even nicer than papering over it with a linter :-) |
|
I'll relay your question to Sebastian U, who is currently investigating type class caching :) |
|
So the issue here might be partially fixed in Core, but certainly an equality like |
|
Thanks for passing that on - and the answer sounds fair. We should probably have both the linter and a core fix if viable :-) |
|
I guess the linter can be more targeted at the particular |
|
Nevermind, that's exactly what the linter checks already. That, plus whether there are any instance or proposition parameters that need discharging. It's pretty reasonable, I'd say. Do you think you'd merge it if I polish? Shall I make it ready for review next week? |
) This PR was automatically created from PR #42056 by @sgraf812 via a [review comment](#42056 (comment)) by @grunweg. Co-authored-by: sgraf812 <1151264+sgraf812@users.noreply.github.com>
|
This PR/issue depends on: |
|
Polishing this would be great. (Splitting out the other adaptations is also welcome.) |
joneugster
left a comment
There was a problem hiding this comment.
I'm a bit confused about the radar result. From the description & discussion I would have expected to see much more of a positive effect. Does that mean the linting takes up more time/instructions than the issues fixed here?
Anyways, even if this was the case, I think this linter is really useful to have, as Michael already said above! (there is always the possibility to disable some linters by default and only run then periodicall if they were ever too slow)
I'll add
awaiting-author
for now to give you the time for clean-up and thelike.
|
Regarding the benchmarking, this underscores that landing the adaptations separately would be really valuable. This way, we separate the performance side-effects (which we could also harvest by having a weekly linter) from the slow-down of the linter. |
|
I'll be happy to review the final adaptation PR, by the way. |
…anprover-community#42061) The new name is better anyway, and simplifies life for the `blanketSimpArgs` linter in leanprover-community#42056: cherry-picked from that PR. Co-authored-by: sgraf812 <1151264+sgraf812@users.noreply.github.com>
…community#42056 (leanprover-community#42062) This PR was automatically created from PR leanprover-community#42056 by @sgraf812 via a [review comment](leanprover-community#42056 (comment)) by @grunweg. Co-authored-by: sgraf812 <1151264+sgraf812@users.noreply.github.com>
|
Sorry, didn't manage to come back to it this week. Next week 🤞 |
|
Sure, no worries! |
7c523de to
042d20f
Compare
|
!bench |
|
Benchmark results for 042d20f against 9fb1099 are in. There are significant results. @sgraf812
Large changes (1🟥)
Small changes (1✅)
|
0257d64 to
73ba323
Compare
Flag simp arguments whose rewrite source is a bare variable of universally quantified type and whose side conditions simp must discharge at every match, such as `simp [Subsingleton.eq_zero]`. Such an argument is keyed as `*` in the discrimination tree, so simp retrieves it at every visited subterm and runs an instance search per attempted match. Determine the side conditions at the use site instead, by applying the lemma to a term or by fixing its implicit arguments, as in `Subsingleton.eq_zero (α := M)`. Pin the remaining such arguments in the library, all uses of `Unique.eq_default`, and drop an inert `Eq.symm` argument.
73ba323 to
f2939c5
Compare
|
!bench |
|
Benchmark results for f2939c5 against 9fb1099 are in. There are significant results. @sgraf812
Large changes (1🟥)
Small changes (1✅)
|
|
Can't make it any faster. I think I'll suggest a |
|
Thanks for trying! I agree that 0.3% overall slowdown is a mouthful, we certainly wouldn't want that as a linter enabled by default. (We could have it as a weekly linter, which is run once a week together with other diagnostic linters.) In any case, a core linter is even better - so fingers crossed! |
|
leanprover/lean4#14677 is awaiting review. The performance numbers look much better and it shows that it more than pays just for the remaining adaptations it found. |
The
blanketSimpArgslinter flags simp arguments such assimp [Subsingleton.eq_zero]whose rewrite source is a bare variable and whose side conditionssimpmust discharge at every match. Such an argument is retrieved at every visited subterm (its discrimination tree key is*), and each attempted match triggers instance searches that miss the cache under binders; #42053 measured a single such argument dominating the elaboration time of a file. The linter suggests determining the side conditions at the use site, by applying the lemma to a term or by fixing its implicit arguments, as inSubsingleton.eq_zero (α := M).The check is semantic, so it covers
Subsingleton.eq_one,Unique.eq_defaultand any other lemma of the same shape. It walks thesimpLemmaargument nodes shared by thesimp-like tactics, including theusingclause ofnontriviality, and the rewrite rules ofsimp_rw.The first commit pins the remaining such simp arguments in Mathlib, all of them uses of
Unique.eq_default, so that the library is clean under the new linter.AI usage: the linter, its tests and the call-site fixes were written with Claude Code (Claude Fable 5) under my direction; I have reviewed all of it, and the tests and affected files were verified by building them locally.