-
Notifications
You must be signed in to change notification settings - Fork 715
fix: hovers and docstrings for (co)inductive types #10738
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: hovers and docstrings for (co)inductive types #10738
Conversation
This PR fixes a regression introduced by leanprover#10307, where hovering the name of an inductive type or constructor in its own declaration didn't show the docstring. In the process, a bug in docstring handling for coinductive types was discovered and also fixed. Tests are added to prevent the regression from repeating in the future.
|
|
||
| private def elabInductiveViewsPostprocessingCoinductive (views : Array InductiveView) | ||
| : CommandElabM Unit := do | ||
| let views := views.map updateViewRemovingFunctorName |
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.
@wkrozowski Could you please confirm that this is correct?
This line was removing the namespaces from the constructor names. With it, the constructors' docstrings weren't being placed on the correct names and thus didn't appear in hovers anywhere. Additionally, adding the terminfo below led to an error, because the constant that the constructor's declId represents was not present in the environment (because the name had the namespace removed).
Removing this line fixes docstrings for coinductive predicates' constructors more generally, and it also allows the overall fix for in-declaration hovers in this PR to work without error. All tests still pass, but I don't have much of a feel for how good our test coverage is for coinductive predicates.
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.
Tldr; that looks good to me. The updateViewRemovingFunctorName is an artifact from my older attempts, that is not really needed.
In detail, in the case of coinductive predicates inelabInductives we look at the InductiveView and add a _functor postfix to it.
if isCoinductive then
runTermElabM fun vars => do
checkNoInductiveNameConflicts elabs (isCoinductive := true)
let flatElabs := elabs.map fun e => {e with view := updateViewWithFunctorName e.view}
We elaborate this as a normal inductive, and then use it in for generating the coinductive predicate with the original name. The updateViewRemovingFunctorName was originally doing that. However, if you inspect elabInductives you will observe that elabInductiveViewsPostprocessingCoinductive is called with the original view, so there is no need to remove the postfix.
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.
Thanks! That was how I'd understood it as well, but without deeply understanding this process I wanted you to check before a merge.
Thanks again!
|
@mhuisi This touches a language server test, so I'll wait for your approval. I don't expect it to be at all controversial, though, as it merely improves test coverage a bit. |
|
Mathlib CI status (docs):
|
|
Reference manual CI status:
|
wkrozowski
left a comment
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.
Could you please remove updateViewRemovingFunctorName from Coinductive.lean before proceeding?
|
|
||
| private def elabInductiveViewsPostprocessingCoinductive (views : Array InductiveView) | ||
| : CommandElabM Unit := do | ||
| let views := views.map updateViewRemovingFunctorName |
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.
Tldr; that looks good to me. The updateViewRemovingFunctorName is an artifact from my older attempts, that is not really needed.
In detail, in the case of coinductive predicates inelabInductives we look at the InductiveView and add a _functor postfix to it.
if isCoinductive then
runTermElabM fun vars => do
checkNoInductiveNameConflicts elabs (isCoinductive := true)
let flatElabs := elabs.map fun e => {e with view := updateViewWithFunctorName e.view}
We elaborate this as a normal inductive, and then use it in for generating the coinductive predicate with the original name. The updateViewRemovingFunctorName was originally doing that. However, if you inspect elabInductives you will observe that elabInductiveViewsPostprocessingCoinductive is called with the original view, so there is no need to remove the postfix.
This is based on feedback from the PR.
Done! Thanks! |
mhuisi
left a comment
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.
The test looks good :-)
This PR fixes a regression introduced by #10307, where hovering the name of an inductive type or constructor in its own declaration didn't show the docstring. In the process, a bug in docstring handling for coinductive types was discovered and also fixed. Tests are added to prevent the regression from repeating in the future.