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

feat: Array.mem: Avoid DecidableEq, set up decreasing_trivial #2774

Merged
merged 1 commit into from
Oct 30, 2023

Conversation

nomeata
Copy link
Collaborator

@nomeata nomeata commented Oct 26, 2023

The notation a ∈ as for Arrays was previously only defined with
DecidableEq on the elements, for (apparently) no good reason. This
drops this requirements (by using a ∈ as.data), and simplifies a bunch
of proofs by simply lifting the corresponding proof from lists.

Also, sizeOf_lt_of_mem was defined, but not set up to be picked up by
decreasing_trivial in the same way that the corresponding List lemma
was set up, so this adds the tactic setup.

The definition for a ∈ as is intentionally not defeq to a ∈ as.data
so that the termination tactics for Arrays don’t spuriously apply when
recursing through lists.

@nomeata
Copy link
Collaborator Author

nomeata commented Oct 26, 2023

awaiting-review

@github-actions github-actions bot added the awaiting-review Waiting for someone to review the PR label Oct 26, 2023
@nomeata
Copy link
Collaborator Author

nomeata commented Oct 26, 2023

A first attempt failed tests like

inductive Term where
  | app (f : String) (args : List Term)

def printFns : Term → IO Unit
  | Term.app f args => do
     IO.println f
     for h : arg in args do
       have : sizeOf arg < 1 + sizeOf f + sizeOf args := Nat.lt_trans (List.sizeOf_lt_of_mem h) (by simp_arith)
       printFns arg

where there is not even any Array around.

It seemed that apply Nat.lt_trans (Array.sizeOf_lt_of_mem ?h) could fire due to unwanted defeq between the element-relatio for Array and List. Fixed by making Array.Mem an inductive.

@github-actions github-actions bot added the toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN label Oct 26, 2023
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Oct 26, 2023
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the builds-mathlib CI has verified that Mathlib builds against this PR label Oct 26, 2023
@leanprover-community-mathlib4-bot
Copy link
Collaborator

leanprover-community-mathlib4-bot commented Oct 26, 2023

@digama0
Copy link
Collaborator

digama0 commented Oct 27, 2023

You should make it a structure instead of an inductive, assuming def doesn't work (it should work though).

@nomeata
Copy link
Collaborator Author

nomeata commented Oct 27, 2023

Will do. Is there a difference between a one field structure and a one field one constructor inductive? Ive seen both patterns so far.

@digama0
Copy link
Collaborator

digama0 commented Oct 27, 2023

Structures have various additional niceties, I don't see why you would ever not use them unless you don't meet the restrictions. Do you have an example? (The field projection is the main important nicety in this case.)

@nomeata
Copy link
Collaborator Author

nomeata commented Oct 27, 2023

I've seen it mostly with rose trees recently:

inductive PrefixTreeNode (α : Type u) (β : Type v) where
(and similar data structures)

Maybe the benefit of an inductive is that you don't have to think of a name for the projection :-)

@nomeata
Copy link
Collaborator Author

nomeata commented Oct 27, 2023

Fixed in 9a7d350, PTAL.

leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Oct 27, 2023
@leodemoura leodemoura added needs-squash and removed awaiting-review Waiting for someone to review the PR labels Oct 28, 2023
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Oct 28, 2023
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan and removed builds-mathlib CI has verified that Mathlib builds against this PR labels Oct 28, 2023
@nomeata
Copy link
Collaborator Author

nomeata commented Oct 29, 2023

Squashed.

(And because Github is very strange in that squashing is just one click away when merging, but basically impossible without a laptop and a command line for the PR authors, in the process I wrote half a Github App that squashes PRs… :-)

@nomeata nomeata force-pushed the Array.mem branch 3 times, most recently from e1a1fc3 to bf19459 Compare October 29, 2023 10:03
@digama0
Copy link
Collaborator

digama0 commented Oct 29, 2023

Maybe the benefit of an inductive is that you don't have to think of a name for the projection :-)

My suggestion would be to just use val for one-field structures, rather than coming up with bespoke names that have to be remembered.

@nomeata nomeata force-pushed the Array.mem branch 2 times, most recently from 977f2e6 to 3864fa4 Compare October 29, 2023 10:12
@nomeata
Copy link
Collaborator Author

nomeata commented Oct 29, 2023

My suggestion would be to just use val for one-field structures, rather than coming up with bespoke names that have to be remembered.

done

The notation `a ∈ as` for Arrays was previously only defined with
`DecidableEq` on the elements, for (apparently) no good reason. This
drops this requirements (by using `a ∈ as.data`), and simplifies a bunch
of proofs by simply lifting the corresponding proof from lists.

Also, `sizeOf_lt_of_mem` was defined, but not set up to be picked up by
`decreasing_trivial` in the same way that the corresponding List lemma
was set up, so this adds the tactic setup.

The definition for `a ∈ as` is intentionally not defeq to `a ∈ as.data`
so that the termination tactics for Arrays don’t spuriously apply when
recursing through lists.
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Oct 29, 2023
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added builds-mathlib CI has verified that Mathlib builds against this PR and removed breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan labels Oct 29, 2023
@kim-em kim-em merged commit f74ae5f into leanprover:master Oct 30, 2023
@nomeata nomeata deleted the Array.mem branch October 30, 2023 07:11
Komyyy pushed a commit to Komyyy/lean4 that referenced this pull request Nov 2, 2023
…over#2774)

The notation `a ∈ as` for Arrays was previously only defined with
`DecidableEq` on the elements, for (apparently) no good reason. This
drops this requirements (by using `a ∈ as.data`), and simplifies a bunch
of proofs by simply lifting the corresponding proof from lists.

Also, `sizeOf_lt_of_mem` was defined, but not set up to be picked up by
`decreasing_trivial` in the same way that the corresponding List lemma
was set up, so this adds the tactic setup.

The definition for `a ∈ as` is intentionally not defeq to `a ∈ as.data`
so that the termination tactics for Arrays don’t spuriously apply when
recursing through lists.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builds-mathlib CI has verified that Mathlib builds against this PR toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants