-
Notifications
You must be signed in to change notification settings - Fork 327
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: apply app unexpanders for all prefixes of an application #3375
Conversation
Mathlib CI status (docs):
|
e6a7ddd
to
94c1ae2
Compare
17d4274
to
f4a5e46
Compare
tests/lean/lcnfTypes.lean
Outdated
-- Universe levels are erased, and when the delaborator sees type incorrect applications it always inserts a `@`. | ||
-- This is why one sees `@List Ty` rather than `List Ty` in the following tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anything we could do against this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a heuristic that if there was a problem computing the ParamKinds, but the universe list is []
, then it's likely that the universe list has been intentionally erased and the application has all its implicit arguments erased, so it tells explicit mode that @
is not necessary.
It's doing this in explicit mode because that avoids expanders from applying, and the logic of explicit mode is simpler to reason about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like in this case we do want the app unexpander for lcErased
to apply. But the more reasonable approach here is probably not to reuse the Lean delaborator for IR printing, so this looks satisfactory to me for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, there is an explicit pp.explicit
in the test, so I suppose this regression is expected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out that match
's missing pattern errors also lack universe levels, so this helps with that as well. (Lean.Meta.Match.Example.toMessageData
)
e9b42b1
to
62cda1c
Compare
…application hypothesis"
This is not actually safe to do -- we would need to run the tactic and check that the passed-in argument is equal.
62cda1c
to
6e55922
Compare
Before, app unexpanders would only be applied to entire applications. However, some notations produce functions, and these functions can be given additional arguments. The solution so far has been to write app unexpanders so that they can take an arbitrary number of additional arguments. However, as reported in this Zulip thread, this leads to misleading hover information in the Infoview. For example, while
HAdd.hAdd f g 1
pretty prints as(f + g) 1
, hovering overf + g
showsf
. There is no way to fix the situation from within an app unexpander; the expression position forHAdd.hAdd f g
is absent, and app unexpanders cannot register TermInfo.This commit changes the app delaborator to try running app unexpanders on every prefix of an application, from longest to shortest prefix. For efficiency, it is careful to only try this when app delaborators do in fact exist for the head constant, and it also ensures arguments are only delaborated once. Then, in
(f + g) 1
, thef + g
gets TermInfo registered for that subexpression, making it properly hoverable.The app delaborator is also refactored, and there are some bug fixes:
pp.explicit
is falseIO
-valued functions to be omitted. (IO
is a reader monad, so it's hiding a pi type)withOverApp
Furthermore, the
notation
command has been modified to generate an app unexpander that relies on the app delaborator's new behavior.The change to app unexpanders is reverse-compatible, but it's recommended to update
@[app_unexpander]
s in downstream projects so that they no longer handle overapplication themselves.