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: change pp.natLit to be enabled by default #6439

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

eric-wieser
Copy link
Contributor

@eric-wieser eric-wieser commented Dec 24, 2024

This PR changes the default such that raw Nat literals always print with nat_lit.

This makes the experience of hovering over 37 : Nat in the infoview less confusing, as it no longer expands to a term that contains another 37 : Nat.

In practice, raw natural literals should never appear in user code, and if they do they are indicative of a buggy tactic or simp lemma that dropped the required OfNat.ofNat wrapper that literals are supposed to have. Enabling this pretty printer option by default increases the chance of these mistakes being reported by the users affected by such bugs, and will be invisible to users who are unaffected.

@eric-wieser eric-wieser marked this pull request as ready for review December 24, 2024 00:43
@eric-wieser eric-wieser requested a review from kmill as a code owner December 24, 2024 00:43
@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 24, 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 b18f3a387772178544db849647684411886dbfd0 --onto 5240405cf4b481278db2bda3357bd2b8b8b466cd. (2024-12-24 00:58:08)

@kmill
Copy link
Collaborator

kmill commented Dec 27, 2024

This makes the experience of hovering over 37 : Nat in the infoview less confusing, as it no longer expands to a term that reads 37 : Nat.

Did you consider having only the hover show nat_lit 37? We could do that by adjusting the options set in Lean.Widget.ppExprTagged or by perhaps making the natlit delaborator respect pp.explicit, if we expand its meaning a bit.

Changing the default value for pp.natLit does make sense to me, but given that its behavior was specified in RFC #3021, I think it would be good to have this change be backed by an RFC as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

3 participants