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: upstream ToLevel from mathlib #6285

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

Conversation

alexkeizer
Copy link
Contributor

@alexkeizer alexkeizer commented Dec 2, 2024

This PR upstreams the ToLevel typeclass from mathlib and uses it to fix the existing ToExpr instances so that they are truly universe polymorphic (previously it generated malformed expressions when the universe level was nonzero). We improve on the mathlib definition of ToLevel to ensure the class always lives in Type, irrespective of the universe parameter.

This implements part one of the plan to upstream a derive handler for ToExpr, as discussed in #5906 and #5909.

@alexkeizer alexkeizer marked this pull request as draft December 2, 2024 12:47
@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 Dec 2, 2024
@leanprover-community-bot
Copy link
Collaborator

Mathlib CI status (docs):

  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase e157fcbcd1643d6440926abcc9cafab1e59cae74 --onto 3c5e612dc54733cd707becb929457d2f9d8ca6fd. (2024-12-02 12:49:46)

@alexkeizer alexkeizer marked this pull request as ready for review December 2, 2024 15:24
@alexkeizer
Copy link
Contributor Author

alexkeizer commented Dec 2, 2024

awaiting-review

@kmill

Copy link
Collaborator

@kmill kmill left a comment

Choose a reason for hiding this comment

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

Thanks! Just a few comments.

prelude
import Lean.Expr

/-! # `ToLevel` class
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/-! # `ToLevel` class
/-!
# `ToLevel` class

Comment on lines +22 to +24
/-- The universe itself. This is only here to avoid the "unused universe parameter" error.
We'll remove this field once https://github.com/leanprover/lean4/issues/2116 gets fixed.
-/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some tweaks to emphasize that it's a hack and to deemphasize any promises that it'll ever be removed (I hope it does, but I haven't heard any motion on 2116 one way or another yet)

Suggested change
/-- The universe itself. This is only here to avoid the "unused universe parameter" error.
We'll remove this field once https://github.com/leanprover/lean4/issues/2116 gets fixed.
-/
/-- A hack to avoid the "unused universe parameter" error.
We can remove this field pending issue https://github.com/leanprover/lean4/issues/2116 -/

/-- The universe itself. This is only here to avoid the "unused universe parameter" error.
We'll remove this field once https://github.com/leanprover/lean4/issues/2116 gets fixed.
-/
univ : ∃ x, x = PUnit.unit.{u} := ⟨_, rfl⟩
Copy link
Collaborator

Choose a reason for hiding this comment

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

It really doesn't matter, but some alternative encodings if you want:

Suggested change
univ : ∃ x, x = PUnit.unit.{u} := ⟨_, rfl⟩
univ : PUnit.{u} → True := fun _ => trivial
Suggested change
univ : ∃ x, x = PUnit.unit.{u} := ⟨_, rfl
univ : ∀ x : PUnit.{u}, x = x := fun _ => rfl

Or, for fun, one that doesn't make use of anything but the basic logic:

Suggested change
univ : ∃ x, x = PUnit.unit.{u} := ⟨_, rfl⟩
univ : ∀ (_ : Sort u) (p : Prop), p → p := fun _ _ h => h


namespace Lean

universe w
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that Lean core makes use of autoImplicit, could you remove this universe command and the universe u v command later on?

| none => mkApp (mkConst ``Option.none [levelZero]) type
| some a => mkApp2 (mkConst ``Option.some [levelZero]) type (toExpr a),
toTypeExpr := mkApp (mkConst ``Option [levelZero]) type }
| none => mkApp (mkConst ``Option.none [l.toLevel]) type
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the style should be using toLevel.{u} rather than naming the ToLevel instance:

Suggested change
| none => mkApp (mkConst ``Option.none [l.toLevel]) type
| none => mkApp (mkConst ``Option.none [toLevel.{u}]) type

@kim-em kim-em added the awaiting-author Waiting for PR author to address issues label Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-author Waiting for PR author to address issues changelog-other 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.

4 participants