leanprover / leanprover/lean4

simp still panics with appArg!/appFn! in the non-indexed rewriting path

Open Beginner friendly
#14,930 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Lean
Stars
9.2k
Forks
990
Avg merge
1d 17h
Merged PRs (30d)
175

Description

#14921 fixed the stale-candidate mechanism in simprocCore/dsimprocCore. A
second, independent instance of the same class of bug remains in the non-indexed
rewriting path, and still reproduces on current master (b0ae187bb9).

Reproducer
opaque g : Nat → Nat → Unit
@[reducible] def foo : Nat → Unit := g 0

theorem all_unit (x : Unit) : x = () := Subsingleton.elim _ _

example : foo 1 = () := by
  simp (config := { index := false }) only [all_unit]

Output on b0ae187bb9:

error: Unknown constant `_inhabitedExprDummy`
PANIC at Lean.Expr.appArg! Lean.Expr:937:15: application expected
PANIC at Lean.Expr.appFn! Lean.Expr:933:15: application expected

No Mathlib required, and the command still exits 0 because panics are logged at
info severity.

Cause

Simp.tryTheoremCore in src/Lean/Meta/Tactic/Simp/Rewrite.lean strips
numExtraArgs arguments with the partial accessors:

  let mut extraArgs := #[]
  let mut e := e
  for _ in *...numExtraArgs do
    extraArgs := extraArgs.push e.appArg!
    e := e.appFn!

Here the count and the expression disagree for a different reason than in
#14921: with index := false, getMatchLiberal computes the count from the
reduced form g 0 1, which has two arguments, while the stripping runs on the
raw foo 1, which has one. Nothing rewrites the expression mid-loop, so
#14921's fix does not apply.

As before the damage is not only log noise: panic! returns its Inhabited
fallback, so a dummy Expr reaches elaboration and produces the spurious
Unknown constant error above.

Fix

Guarding this loop is appropriate, since unlike the simproc case there is no
candidate list to re-derive; the mismatch is intrinsic to comparing a reduced
form against a raw one:

  for _ in *...numExtraArgs do
    unless e.isApp do return none
    extraArgs := extraArgs.push e.appArg!
    e := e.appFn!

I have this patched and tested locally and can open a PR with a regression test
if that is wanted. This was the second half of the now-closed #14923; the first
half was superseded by #14921, which is the better fix for that mechanism.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in src/Lean/Meta/Tactic/Simp/Rewrite.lean at Simp.tryTheoremCore and reproduce the index := false example from the issue. Ensure the non-indexed rewriting path no longer reaches partial app accessors when the argument count exceeds the raw expression, then add and run a regression test confirming the example produces no panic or spurious error.

Written by the indexing model from the issue text.

Assessment

Domain
compilers
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
82/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.