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: extract delabAppCore, define withOverApp, and make over-applied projections pretty print #3083

Merged
merged 4 commits into from
Jan 10, 2024

Conversation

kmill
Copy link
Collaborator

@kmill kmill commented Dec 17, 2023

To handle delaborating notations that are functions that can be applied to arguments, extracts the core function application delaborator as a separate function that accepts the number of arguments to process and a delaborator to apply to the "head" of the expression.

Defines withOverApp, which has the same interface as the combinator of the same name from std4, but it uses this core function application delaborator.

Uses withOverApp to improve a number of application delaborators, notably projections. This means Mathlib can stop using pp_dot for structure fields that have function types.

Incidentally fixes getParamKinds to specialize default values to use supplied arguments, which impacts how default arguments are delaborated.

@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 17, 2023
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Dec 17, 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 Dec 17, 2023
@leanprover-community-mathlib4-bot
Copy link
Collaborator

leanprover-community-mathlib4-bot commented Dec 17, 2023

  • 💥 Mathlib branch lean-pr-testing-3083 build failed against this PR. (2023-12-17 00:26:07) View Log
  • 💥 Mathlib branch lean-pr-testing-3083 build failed against this PR. (2023-12-17 00:48:10) View Log
  • 🟡 Mathlib branch lean-pr-testing-3083 build this PR didn't complete normally. (2023-12-17 15:13:02) View Log
  • ✅ Mathlib branch lean-pr-testing-3083 has successfully built against this PR. (2023-12-17 16:30:31) View Log
  • ❗ Mathlib CI will not be attempted unless you rebase your PR onto the 'nightly' branch. (2023-12-17 16:47:01)
  • ❗ Mathlib CI can not be attempted yet, as the 'nightly-testing-2023-12-24' branch does not exist there yet. We will retry when you push more commits. It may be necessary to rebase onto 'nightly' tomorrow. (2023-12-25 05:45:34)

leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Dec 17, 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 Dec 17, 2023
@@ -919,6 +919,14 @@ private def getAppNumArgsAux : Expr → Nat → Nat
def getAppNumArgs (e : Expr) : Nat :=
getAppNumArgsAux e 0

/--
If the given expression is a sequence of function applications `f a₁ .. aₙ`,
returns `f a₁ .. aₖ` where `k` is minimal such that `n - k ≤ maxArgs`.
Copy link
Member

Choose a reason for hiding this comment

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

Should it be maxRemainingArgs here for clarity?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I find my comment to be confusing. I want getBoundedAppFn m e to pop off at most m arguments, and I want it to be "dual" to getBoundedAppArgs in the sense that mkAppN (e.getBoundedAppFn m) (e.getBoundedAppArgs m) = e.

Really, the use case is extracting a function of a particular arity, but I found it easier to think about these functions in their current form, where it is up to the caller to decide how many arguments to pop off.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried to clarify the docstring.

The remaining arguments are processed depending on whether heuristics indicate that the application
should be delaborated using `@`.
-/
def delabAppCore (maxArgs : Nat) (delabHead : Delab) : Delab := do
Copy link
Member

Choose a reason for hiding this comment

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

Are there any delabs in core that should already be using delabAppCore with a specific maxArgs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, many builtin delaborators could use it. One example is delabProjectionApp, which could use it to handle over-application.

My plan was to use delabAppCore to upstream a more general version of Lean.PrettyPrinter.Delaborator.withOverApp and then use this combinator to improve builtins.

Copy link
Member

Choose a reason for hiding this comment

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

Great, SGTM

Copy link
Collaborator Author

@kmill kmill Dec 25, 2023

Choose a reason for hiding this comment

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

I went ahead and upstreamed the combinator and improved the builtins that could be improved.

By the way, I found a bug in getParamKinds where the default values can refer to temporary fvars rather than be specialized to the supplied arguments. I rewrote the function to be more efficient while fixing this -- I can split those changes into. a separate PR if you want.

@kmill kmill force-pushed the refactor_delabApp branch from 6c851fd to 0573ac1 Compare December 25, 2023 05:31
@kmill kmill changed the title refactor: extracts delabAppCore for use in delaborators feat: extracts delabAppCore, defines withOverApp, and makes over-applied projections pretty print Dec 25, 2023
@Kha Kha changed the title feat: extracts delabAppCore, defines withOverApp, and makes over-applied projections pretty print feat: extract delabAppCore, define withOverApp, and make over-applied projections pretty print Jan 10, 2024
@Kha Kha added this pull request to the merge queue Jan 10, 2024
Merged via the queue into leanprover:master with commit c394a83 Jan 10, 2024
10 checks passed
nomeata added a commit to nomeata/batteries that referenced this pull request Jan 11, 2024
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.

3 participants