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

fix: generalize excessive resource usage #3575

Merged
merged 1 commit into from
Mar 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 61 additions & 10 deletions src/Lean/Meta/Tactic/Generalize.lean
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ structure GeneralizeArg where
Telescopic `generalize` tactic. It can simultaneously generalize many terms.
It uses `kabstract` to occurrences of the terms that need to be generalized.
-/
private partial def generalizeCore (mvarId : MVarId) (args : Array GeneralizeArg) : MetaM (Array FVarId × MVarId) :=
private partial def generalizeCore (mvarId : MVarId) (args : Array GeneralizeArg)
(transparency : TransparencyMode) : MetaM (Array FVarId × MVarId) :=
mvarId.withContext do
mvarId.checkNotAssigned `generalize
let tag ← mvarId.getTag
Expand All @@ -35,7 +36,8 @@ private partial def generalizeCore (mvarId : MVarId) (args : Array GeneralizeArg
let eType ← instantiateMVars (← inferType e)
let type ← go (i+1)
let xName ← if let some xName := arg.xName? then pure xName else mkFreshUserName `x
return Lean.mkForall xName BinderInfo.default eType (← kabstract type e)
return Lean.mkForall xName BinderInfo.default eType
(← withTransparency transparency <| kabstract type e)
else
return target
let targetNew ← go 0
Expand Down Expand Up @@ -71,30 +73,79 @@ private partial def generalizeCore (mvarId : MVarId) (args : Array GeneralizeArg
mvarId.assign (mkAppN (mkAppN mvarNew es) rfls.toArray)
mvarNew.mvarId!.introNP (args.size + rfls.length)

/-
Remark: we use `TransparencyMode.instances` as the default setting at `generalize`
and `generalizeHyp` to avoid excessive resource usage.

**Motivation:**
The `kabstract e p` operation is widely used, for instance, in the `generalize` tactic.
It operates by taking an expression `e` and a pattern (i.e., an expression containing metavariables)
and employs keyed-matching to identify and abstract instances of `p` within `e`.
For example, if `e` is `a + (2 * (b + c))` and `p` is `2 * ?m`, the resultant expression
would be `a + #0`, where `#0` represents a loose bound variable.

This matching process is not merely syntactic; it also considers reduction. It's impractical
to attempt matching each sub-term with `p`; therefore, only sub-terms sharing the same "root"
symbol are evaluated. For instance, with the pattern `2 * ?m`, only sub-terms with the
root `*` are considered. Matching is executed using the definitionally equality test
(i.e., `isDefEq`).

The `generalize` tactic employs `kabstract` and defaults to standard reducibility.
Hence, the `isDefEq` operations invoked by `kabstract` can become highly resource-intensive
and potentially trigger "max recursion depth reached" errors, as observed in issue #3524.
This issue was isolated by @**Scott Morrison** with the following example:
```
example (a : Nat) : ((2 ^ 7) + a) - 2 ^ 7 = 0 := by
generalize 0 - 0 = x
```
In this scenario, `kabstract` triggers a "max recursion depth reached" error while
testing whether `((2 ^ 7) + a) - 2 ^ 7` is definitionally equal to `0 - 0`.
Note that the term `((2 ^ 7) + a) - 2 ^ 7` is not ground.
We believe most users find the error message to be uninformative and unexpected.
To fix this issue, we decided to use `TransparencyMode.instances` as the default setting.

Kyle Miller has performed the following analysis on the potential impact of the
changes on Mathlib (2024-03-02).

There seem to be just 130 cases of generalize in Mathlib, and after looking through a
good number of them, they seem to come in just two types:

- Ones where it looks like reducible+instance transparency should work, where in
particular there is nothing obvious being reduced, and
- Ones that don't make use of the `kabstract` feature at all (it's being used like a
`have` that introduces an equality for rewriting).

That wasn't a systematic review of generalize though. It's possible changing the
transparency settings would break things, but in my opinion it would be better
if generalize weren't used for unfolding things.
-/

@[inherit_doc generalizeCore]
def _root_.Lean.MVarId.generalize (mvarId : MVarId) (args : Array GeneralizeArg) : MetaM (Array FVarId × MVarId) :=
generalizeCore mvarId args
def _root_.Lean.MVarId.generalize (mvarId : MVarId) (args : Array GeneralizeArg)
(transparency := TransparencyMode.instances) : MetaM (Array FVarId × MVarId) :=
generalizeCore mvarId args transparency

@[inherit_doc generalizeCore, deprecated MVarId.generalize]
def generalize (mvarId : MVarId) (args : Array GeneralizeArg) : MetaM (Array FVarId × MVarId) :=
generalizeCore mvarId args
def generalize (mvarId : MVarId) (args : Array GeneralizeArg)
(transparency := TransparencyMode.instances) : MetaM (Array FVarId × MVarId) :=
generalizeCore mvarId args transparency

/--
Extension of `generalize` to support generalizing within specified hypotheses.
The `hyps` array contains the list of hypotheses within which to look for occurrences
of the generalizing expressions.
-/
def _root_.Lean.MVarId.generalizeHyp (mvarId : MVarId) (args : Array GeneralizeArg) (hyps : Array FVarId := #[])
(fvarSubst : FVarSubst := {}) : MetaM (FVarSubst × Array FVarId × MVarId) := do
(fvarSubst : FVarSubst := {}) (transparency := TransparencyMode.instances) : MetaM (FVarSubst × Array FVarId × MVarId) := do
if hyps.isEmpty then
-- trivial case
return (fvarSubst, ← mvarId.generalize args)
return (fvarSubst, ← mvarId.generalize args transparency)
let args ← args.mapM fun arg => return { arg with expr := ← instantiateMVars arg.expr }
let hyps ← hyps.filterM fun h => do
let type ← instantiateMVars (← h.getType)
args.anyM fun arg => return (← kabstract type arg.expr).hasLooseBVars
args.anyM fun arg => return (← withTransparency transparency <| kabstract type arg.expr).hasLooseBVars
let (reverted, mvarId) ← mvarId.revert hyps true
let (newVars, mvarId) ← mvarId.generalize args
let (newVars, mvarId) ← mvarId.generalize args transparency
let (reintros, mvarId) ← mvarId.introNP reverted.size
let fvarSubst := Id.run do
let mut subst : FVarSubst := fvarSubst
Expand Down
3 changes: 3 additions & 0 deletions tests/lean/run/3524.lean
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
example (a : Nat) : ((2 ^ 7) + a) - 2 ^ 7 = 0 := by
generalize 0 - 0 = x
sorry
Loading