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

fix: universe inference for Prop-valued structures/inductives #2695

Closed

Conversation

arthur-adjedj
Copy link
Contributor

Closes #2690

@github-actions
Copy link
Contributor

Thanks for your contribution! Please make sure to follow our Commit Convention.

@arthur-adjedj arthur-adjedj force-pushed the StructCorrectUniv branch 2 times, most recently from 95e4f23 to c00a093 Compare October 16, 2023 14:28
@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 16, 2023
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Oct 16, 2023
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan label Oct 16, 2023
@leanprover-community-mathlib4-bot
Copy link
Collaborator

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

  • 💥 Mathlib branch lean-pr-testing-2695 build failed against this PR. (2023-10-16 16:27:52) View Log
  • ❗ Std CI can not be attempted yet, as the nightly-testing-2024-03-11 tag does not exist there yet. We will retry when you push more commits. If you rebase your branch onto nightly-with-mathlib, Std CI should run now. (2024-03-11 14:27:10)
  • 💥 Mathlib branch lean-pr-testing-2695 build failed against this PR. (2024-08-28 17:04:35) View Log
  • 💥 Mathlib branch lean-pr-testing-2695 build failed against this PR. (2024-09-03 14:12:08) View Log

@@ -1684,15 +1684,15 @@ axiom ofReduceNat (a b : Nat) (h : reduceNat a = b) : a = b
`IsAssociative op` says that `op` is an associative operation,
i.e. `(a ∘ b) ∘ c = a ∘ (b ∘ c)`. It is used by the `ac_rfl` tactic.
-/
class IsAssociative {α : Sort u} (op : α → α → α) where
class IsAssociative {α : Sort u} (op : α → α → α) : Type where
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO these should be Props, this is exactly the kind of issue we are hoping to avoid by changing the default

@@ -807,7 +807,7 @@ decidability instance instead of the proposition, which has no code).
If a proposition `p` is `Decidable`, then `(by decide : p)` will prove it by
evaluating the decidability instance to `isTrue h` and returning `h`.
-/
class inductive Decidable (p : Prop) where
class inductive Decidable (p : Prop) : Type where
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a bug, it should not infer Prop here because the inductive has two constructors.

Copy link
Contributor Author

@arthur-adjedj arthur-adjedj Oct 16, 2023

Choose a reason for hiding this comment

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

Yes, the current draft makes a hacky tweak to accLevel, which I believe isn't the right solution, as explained in the issue (This is not meant for review just yet). When tweaking mkResultUniverse, I end up getting some kernel errors regarding loose universe variables in sort-polymorphic inductives like PProd, so there's still work to do.

@kim-em kim-em added the WIP This is work in progress, and only a PR for the sake of CI or sharing. No review yet. label Oct 17, 2023
@kim-em kim-em removed the breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan label Nov 5, 2023
@arthur-adjedj arthur-adjedj force-pushed the StructCorrectUniv branch 2 times, most recently from 2fb4925 to 59ce249 Compare August 28, 2024 16:40
@arthur-adjedj arthur-adjedj changed the title fix : universe inference for Prop-valued structures/inductives fix: universe inference for Prop-valued structures/inductives Aug 28, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Aug 28, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Aug 28, 2024
@leanprover-community-bot leanprover-community-bot added the breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan label Aug 28, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Sep 3, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Sep 3, 2024
kmill added a commit to kmill/lean4 that referenced this pull request Sep 29, 2024
…c subsingletons

A `Prop`-valued inductive type is a syntactic subsingleton if it has at most one constructor and all the arguments to the constructor are in `Prop`. Such types have large elimination, so they could be defined in `Type` or `Prop` without any trouble, though users tend to expect that such types define a `Prop` and need to learn to insert `: Prop` manually.

Currently, the default universe for types is `Type`. This PR adds a heuristic: if a type is a syntactic subsingleton with exactly one constructor, and the constructor has at least one parameter, then the `inductive` command will prefer creating a `Prop` instead of a `Type`. For `structure`, we ask for at least one field.

More generally, for mutual inductives, each type needs to be a syntactic subsingleton, at least one type must have one constructor, and at least one constructor must have at least one parameter.

Thanks to @arthur-adjedj for the investigation in leanprover#2695.

Closes leanprover#2690
kmill added a commit to kmill/lean4 that referenced this pull request Oct 8, 2024
…c subsingletons

A `Prop`-valued inductive type is a syntactic subsingleton if it has at most one constructor and all the arguments to the constructor are in `Prop`. Such types have large elimination, so they could be defined in `Type` or `Prop` without any trouble, though users tend to expect that such types define a `Prop` and need to learn to insert `: Prop` manually.

Currently, the default universe for types is `Type`. This PR adds a heuristic: if a type is a syntactic subsingleton with exactly one constructor, and the constructor has at least one parameter, then the `inductive` command will prefer creating a `Prop` instead of a `Type`. For `structure`, we ask for at least one field.

More generally, for mutual inductives, each type needs to be a syntactic subsingleton, at least one type must have one constructor, and at least one constructor must have at least one parameter.

Thanks to @arthur-adjedj for the investigation in leanprover#2695.

Closes leanprover#2690
github-merge-queue bot pushed a commit that referenced this pull request Oct 8, 2024
…c subsingletons (#5517)

A `Prop`-valued inductive type is a syntactic subsingleton if it has at
most one constructor and all the arguments to the constructor are in
`Prop`. Such types have large elimination, so they could be defined in
`Type` or `Prop` without any trouble, though users tend to expect that
such types define a `Prop` and need to learn to insert `: Prop`.

Currently, the default universe for types is `Type`. This PR adds a
heuristic: if a type is a syntactic subsingleton with exactly one
constructor, and the constructor has at least one parameter, then the
`inductive` command will prefer creating a `Prop` instead of a `Type`.
For `structure`, we ask for at least one field.

More generally, for mutual inductives, each type needs to be a syntactic
subsingleton, at least one type must have one constructor, and at least
one constructor must have at least one parameter. The motivation for
this restriction is that every inductive type starts with a zero
constructors and each constructor starts with zero fields, and
stubbed-out types shouldn't be `Prop`.

Thanks to @arthur-adjedj for the investigation in #2695 and to @digama0
for formulating the heuristic.

Closes #2690
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN WIP This is work in progress, and only a PR for the sake of CI or sharing. No review yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

structure sometimes makes a Type not a Prop.
5 participants