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

default termination tactic: array_dec_dec tactic too eager, and too weak #5027

Closed
BrunoDutertre opened this issue Aug 13, 2024 · 1 comment · Fixed by #5037
Closed

default termination tactic: array_dec_dec tactic too eager, and too weak #5027

BrunoDutertre opened this issue Aug 13, 2024 · 1 comment · Fixed by #5037
Assignees
Labels
bug Something isn't working

Comments

@BrunoDutertre
Copy link

Prerequisites

Please put an X between the brackets as you perform the following steps:

Description

On this example, lean produces a confusing error.

inductive Term (α: Type): Type where
  | Composite : Array (Term α) → Term α
  | Atom: α → Term α

-- height of a term
def height (f: Term α): Nat :=
  let rec max_height (a: Array (Term α)) (i: Nat) (m: Nat): Nat :=
    if h: i < a.size then
      max_height a (i + 1) (max (height a[i]) m). -- error for `height a[I]`
    else
      m
  match f with
  | .Composite a => 1 + max_height a 0 0
  | .Atom _ => 1

The error is this:

Height.lean:10:33
unsolved goals
case h
α : Type
a : Array (Term α)
i m : Nat
h : i < a.size
⊢ False

Adding explicit termination_by clauses, as in the following, does not help:

mutual
def max_height' (a: Array (Term α)) (i: Nat) (m: Nat): Nat :=
  if h: i < a.size then
    max_height' a (i + 1) (max (height' a[i]) m)
  else
    m
termination_by (sizeOf a, a.size - i)

def height' (f: Term α): Nat :=
  match f with
  | .Composite a => 1 + max_height' a 0 0
  | .Atom _ => 1
termination_by (sizeOf f, 0)
end

Steps to Reproduce

  1. Cut and paste one of the above examples. Then run lean.

Expected behavior:

In order of preference:

  1. Have the default termination heuristics synthesize a termination ordering that works on the example
  2. If that fails, produce an error message that explains that the problem is related to termination

Actual behavior:

Obscure error: lean just produce a message about an 'unsolved goal` but this goal is unsolvable.
It's not clear at all from the message that this is related to termination.

Versions

Lean 4.10.0

Additional Information

NA

Impact

Add 👍 to issues you consider important. If others are impacted by this issue, please ask them to add 👍 to it.

@BrunoDutertre BrunoDutertre added the bug Something isn't working label Aug 13, 2024
@nomeata
Copy link
Collaborator

nomeata commented Aug 14, 2024

Thanks for the report. The issue is somewhere with array_get_dec, which forms part of decreasing_trivial. Inlining it all we get

inductive Term (α: Type): Type where
  | Composite : Array (Term α) → Term α
  | Atom: α → Term α

-- height of a term
mutual
def max_height' (a: Array (Term α)) (i: Nat) (m: Nat): Nat :=
  if h: i < a.size then
    max_height' a (i + 1) (max (height' a[i]) m)
  else
    m
termination_by (sizeOf a, a.size - i)
decreasing_by
  . decreasing_with    
      (with_reducible apply Nat.lt_trans (Array.sizeOf_getElem ..))
      -- sizeOf a < sizeOf a
      simp_arith
      
  . decreasing_tactic


def height' (f: Term α): Nat :=
  match f with
  | .Composite a => 1 + max_height' a 0 0
  | .Atom _ => 1
termination_by (sizeOf f, 0)
decreasing_by 
  . decreasing_tactic

end

And of course sizeOf a < sizeOf a is bogus.

I assume that the lemma should be phrased with Nat.lt_of_lt_of_le instead of Nat.lt_trans. Same for the corresponding list lemma.

It is a bit unfortunate that we even need these lemmas here; I wish there was a way to tell omega all it needs to know about the theory of sizeOf that a simple omega would suffice. Until then, we’ll fix these issues as they come up.

@nomeata nomeata changed the title Default termination tactic gives confusing error default termination tactic: array_dec_dec tactic too eager, and too weak Aug 14, 2024
@nomeata nomeata self-assigned this Aug 14, 2024
nomeata added a commit that referenced this issue Aug 14, 2024
Using `Nat.lt_trans` is too restrictive, and using `Nat.lt_of_lt_of_le`
should make this tactic prove more goals.

Fixes #5027
github-merge-queue bot pushed a commit that referenced this issue Aug 14, 2024
Using `Nat.lt_trans` is too restrictive, and using `Nat.lt_of_lt_of_le`
should make this tactic prove more goals.

This fixes a regression probably introduced by #3991; at least in some
cases before that `apply sizeOf_get` would have solved the goal here.
And it’s true that this is now subsumed by `simp`, but because of the
order that `macro_rules` are tried, the too restrictive variant with
`Nat.lt_trans` would be tried before `simp`, without backtracking.

Fixes #5027
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants