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

Make Decidable a subtype of Bool #2038

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

gebner
Copy link
Member

@gebner gebner commented Jan 15, 2023

This resolves the diamond between Decidable and BEq, which is starting to cause lots of headaches in mathlib.

-/
return false
/-
TODO: remove this hack after we refactor `Decidable` as suggested by Gabriel.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this TODO can now be removed, right? Or is the return false still a hack?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I understand, the return false here is about instances that have more than one exit point, i.e., if c then { a := 42 } else { a := 1 }. We still allow that and I didn't remove class inductive either.

@leodemoura leodemoura added the dev meeting It will be discussed at the (next) dev meeting label Jan 15, 2023
@gebner
Copy link
Member Author

gebner commented Jan 17, 2023

This PR is blocked by what I can only assume to be a miscompilation. Running stage1 immediately segfaults in the Syntax.identComponents function where we call lean_inc on a freed object that was allocated in splitNameLit.

Diffing the IR code is hard because we generate lots of block y := ..; case x of false-> jmp block 0; true -> jmp block 1 instead of a direct case distinction.

@Kha
Copy link
Member

Kha commented Jan 20, 2023

Regarding the explosion of join points (which perhaps is triggering the bug), I think the old compiler is very reliant on ite being a direct recursor application . At the LCNF level we expand macro_inline but no matchers. Unfortunately

@[macro_inline] def dite {α : Sort u} (c : Prop) [h : Decidable c] (t : c → α) (e : Not c → α) : α :=
  h.decide.casesOn
    (fun h => e (nomatch h.2 ·))
    (fun h => t (h.1 (.refl _)))
    h.decide_iff

@[macro_inline] def ite {α : Sort u} (c : Prop) [Decidable c] (t e : α) : α :=
  dite c (fun _ => t) (fun _ => e)

doesn't really help because the overapplication of Bool.casesOn seems to trigger a similar join point mess.

@gebner gebner added depends on new code generator We are currently working on a new compiler (code generator) for Lean. This issue/PR is blocked by it and removed dev meeting It will be discussed at the (next) dev meeting labels Jan 23, 2023
@gebner gebner removed the depends on new code generator We are currently working on a new compiler (code generator) for Lean. This issue/PR is blocked by it label Jan 28, 2023
@gebner
Copy link
Member Author

gebner commented Jan 28, 2023

Rebasing onto #2060 seemed to fix the compilation errors.

!bench

@gebner gebner marked this pull request as ready for review January 28, 2023 02:21
@gebner gebner added the dev meeting It will be discussed at the (next) dev meeting label Jan 28, 2023
@Kha
Copy link
Member

Kha commented Jan 31, 2023

!bench

@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit 6cdf707.
There were significant changes against commit 345aa6f:

  Benchmark          Metric         Change
  ===================================================
- binarytrees        task-clock       3.9%   (10.5 σ)
- stdlib             instructions     2.1% (1236.2 σ)
- stdlib             maxrss           1.4%  (140.2 σ)
- stdlib             task-clock       1.5%   (19.4 σ)
- stdlib             wall-clock       1.3%   (67.3 σ)
- stdlib size        bytes .olean     2.2%
- workspaceSymbols   maxrss           1.4%   (15.2 σ)
+ workspaceSymbols   task-clock      -4.0%  (-20.8 σ)
+ workspaceSymbols   wall-clock      -4.0%  (-20.8 σ)

@fgdorais
Copy link
Contributor

Just a ping in eager anticipation of progress on this PR!

@kmill
Copy link
Collaborator

kmill commented Nov 21, 2023

Something that might be a less invasive change is this:

class Decidable (p : Prop) where
  /-- The truth value of the proposition `p` as a `Bool`.
  If `true` then `p` is true, and if `false` then `p` is false. -/
  decide : Bool
  /-- `decide p` evaluates to the Boolean `true` if and only if `p` is a true proposition.  -/
  cond_decide : cond decide p (Not p)

/-- Prove that `p` is decidable by supplying a proof of `p` -/
@[match_pattern] def Decidable.isTrue {p : Prop} (h : p) : Decidable p where
  decide := true
  cond_decide := h

/-- Prove that `p` is decidable by supplying a proof of `¬p` -/
@[match_pattern] def Decidable.isFalse {p : Prop} (h : Not p) : Decidable p where
  decide := false
  cond_decide := h

In my limited testing, you don't need to touch pre-existing match patterns, which would be great if that meant downstream projects don't need to be updated.

However, this does need a modification to erase_irrelevant.cpp and perhaps other parts of the compiler, since properties of Decidable are hard-coded into it, and changing Decidable out from under it causes assertion violations.

@urkud
Copy link

urkud commented Jul 2, 2024

@kmill I tried your definition on current version of Lean4, then

@[macro_inline] def dite {α : Sort u} (c : Prop) [h : Decidable c] (t : c → α) (e : Not c → α) : α :=
  match h with
  | .isTrue h => t h
  | .isFalse h => e h

says "'unreachable' code was reached". UPD: I see that the assertion in decidable_to_bool_cases fails, I'll fix it tonight after day job.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev meeting It will be discussed at the (next) dev meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants