feat: generalize below and brecOn on subsingletons#12718
feat: generalize below and brecOn on subsingletons#12718arthur-adjedj wants to merge 7 commits intoleanprover:masterfrom
below and brecOn on subsingletons#12718Conversation
|
Reference manual CI status:
|
|
Mathlib CI status (docs):
|
|
!bench |
|
Benchmark results for f03c018 against c595413 are in! @arthur-adjedj
Small changes (4🟥)
|
|
BTW, if you need the stage0 update only to get the test suite to pass, but not because of actual bootstrapping issues, you don't need the stage0 update on this branch. It suffices to leave a comment in stage0/src/stdlib_flags.h and the CI will run the test suite with stage2. |
4a1dcf4 to
cf38184
Compare
|
changelog-language |
src/Lean/Meta/Basic.lean
Outdated
| That is, `inductive` type in `Prop` and cannot eliminate to any `Sort u`. | ||
| -/ | ||
| def isInductivePredicateNotSubsingleton? (declName : Name) : MetaM Bool := do | ||
| match (← getEnv).find? (declName ++ `rec) with |
There was a problem hiding this comment.
I think ++ can do the wrong thing for names with macro scopes, or something. Do we not have a mkRecName function somewhere?
There was a problem hiding this comment.
We do. ++ 'rec is already used in a few other places, it might be worth fixing those too.
There was a problem hiding this comment.
Let's use it here to not make things worse, but leave that cleanup elsewhere for another PR. Fixing inductives in macros maybe a larger task.
src/Lean/Meta/Basic.lean
Outdated
| Return `some info` if `declName` is an non-subsingleton inductive predicate where `info : InductiveVal`. | ||
| That is, `inductive` type in `Prop` and cannot eliminate to any `Sort u`. | ||
| -/ | ||
| def isInductivePredicateNotSubsingleton? (declName : Name) : MetaM Bool := do |
There was a problem hiding this comment.
Is this different from ”rec eliminates not into Sort”?
There was a problem hiding this comment.
It's not. Is there an existing function for this ? If not, should this function be named something else ? it definitely should not end with a ?, that's a typo. Since it replaces isInductivePredicate?, I thought I would extend the naming to specifically mention non-singleton predicates, but something better is probably more sensible
There was a problem hiding this comment.
Since this construction is really based off the recursor, and simply eliminates into whatever the recursor supports, that seems the more natural phrasing. Imagine people generating “recursors” in libraries at some point. Then this construction would still work and only care about the shape of the recursor.
In other places in the Lean code we do this check simply by checking if the recursor has one level parameter more than its inductive, IIRC. Not sure which check is better or it it makes a difference. It would be nice to unify these checks in one place. I'm not expecting you to do that in this PR, but if you feel like it I'll happily review that.
There was a problem hiding this comment.
In other places in the Lean code we do this check simply by checking if the recursor has one level parameter more than its inductive.
I will adapt this function to do the same then.
It would be nice to unify these checks in one place
Would adding a "does its recursor eliminate to sort" field to RecursorVal be worth anything for this ? Since this is already computed when the recursor is generated initially, the changes would be minimal (similarly to your past numNested refactoring), and with that implemented, fixing #3213 would now be a one-line change .
There was a problem hiding this comment.
I'm wary of changing RecursorVal, it may break meta code downstream, and things like the export format. Not worth it given that it is easy to recalculate and not on any hot path.
There was a problem hiding this comment.
Can you test that eq_def and induct_unfolding work properly here?
|
Mathlib CI status (docs):
|
|
As discussed: This change is welcome in principle, but runs into existing issues in |
This PR makes it so that (recursive) inductive predicate that are subsingletons, namely
Acc, havebelowandbrecOnbe generated like any other non-predicate inductive type rather than treating them usingIndPredBelow. This allowsbrecOnto eliminate to sorts instead of only Prop.Closes #12739