Skip to content

Conversation

jcreedcmu
Copy link

@jcreedcmu jcreedcmu commented Sep 25, 2025

Resolves #542.

Previously, we turned all headers into emphasized text regardless of level. With this change, we insert nested parts into the current verso document corresponding to the markdown structure.

Previously, we turned all headers into emphasized text regardless of
level. With this change, we insert parts into the current verso
document.
@jcreedcmu
Copy link
Author

This isn't right yet; it inserts consecutive parts more deeply nested than they ought to be, e.g. release note markdown

## foo
## bar
## baz

becomes rendered as if it was

## foo
### bar
#### baz

@jcreedcmu jcreedcmu marked this pull request as draft September 25, 2025 14:20
@jcreedcmu
Copy link
Author

I think I see what I'm supposed to do instead from addPartsFromMarkdown's docstring...

Now the headers are inserted at the right levels. They don't receive
permalinks, which I don't yet understand.
@jcreedcmu jcreedcmu marked this pull request as ready for review September 25, 2025 14:43
These were reported because of treating markdown sections as first-class parts.
@jcreedcmu
Copy link
Author

This is now, I think, correct, unless we expect permalink icons to appear while hovering over section headers in the release notes. I tentatively expected that, and I observe that they do not appear.

I think traversal is still correctly assigning fresh ids to sections arising from markdown in the release notes, because the sections appear in the TOC sidebar, and clicking on them still correctly goes to the right url-with-fragment. I think this is where permalinks get created.

All of these features can be disabled, and 'Goals accomplished!' icon can be configured in VS Code extension settings.
See [leanprover/vscode-lean4#585](https://github.com/leanprover/vscode-lean4/pull/585) for the details.

### Parallel Elaboration
Copy link
Author

@jcreedcmu jcreedcmu Sep 25, 2025

Choose a reason for hiding this comment

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

There was another Parallel Elaboration section in the release notes with which this one was redundant. The other one contained #7084 with identical description, and many more items.

Copy link
Collaborator

Choose a reason for hiding this comment

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

One subsection is a subsection of Highlights and the other one in the full list. Don't delete @eyihluyc's work 😅

Copy link
Author

Choose a reason for hiding this comment

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

Perish the thought! Thanks for catching my mistake. Trying to determine now why the two sections were rendered in my testing as siblings, which I now see is clearly wrong.

- Updates to `Perm` API in `Array`, `List`, and added support for `Vector`,
- Additional lemmas for `Array`/`List`/`Vector`.

### Lake
Copy link
Author

Choose a reason for hiding this comment

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

There was another Lake section in the release notes with which this one was redundant. The other one contained #7909 with almost identical description, (except it misspelled server as sever) and many more items.

Copy link
Collaborator

Choose a reason for hiding this comment

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

One subsection is a subsection of Highlights and the other one in the full list. Don't delete @eyihluyc's work 😅

@jcreedcmu
Copy link
Author

I'm interested in ignoring the prose checks for now as they are numerous and orthogonal to this change.

@jcreedcmu jcreedcmu requested a review from nomeata September 25, 2025 20:58
@david-christiansen
Copy link
Collaborator

If you stick class="no-vale" on the resulting HTML, the prose linter will ignore these.

pure #[← ``(Block.other Block.noVale #[$content,*])]
@[part_command Lean.Doc.Syntax.codeblock]
def markdown : PartCommand
| `(Lean.Doc.Syntax.codeblock| ``` markdown | $txt ``` ) => do
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 matching the literal identifier markdown, which means that it won't react correctly to the user's open statements or their own shadowing definitions. Similarly, it's better to accept arguments here, then either ignore or reject them, so that the pattern doesn't stop matching as soon as someone provides an argument.

You should instead do something like:

Suggested change
| `(Lean.Doc.Syntax.codeblock| ``` markdown | $txt ``` ) => do
| `(Lean.Doc.Syntax.codeblock| ``` $markdown:ident $args*| $txt ``` ) => do
let x <- realizeGlobalConstNoOverloadsWithInfo markdown -- This actually resolves the name
if x != ``markdown then Elab.throwUnsupportedSyntax -- If it's some other `markdown`, defer to it
-- Now we commit to our function and can reject arguments
for arg in args do
let h <- m!"Remove it:" #[""] (ref? := arg)
logErrorAt arg m!"No arguments expected{h}"

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, makes sense, will do!

@david-christiansen
Copy link
Collaborator

This is now, I think, correct, unless we expect permalink icons to appear while hovering over section headers in the release notes. I tentatively expected that, and I observe that they do not appear.

I think traversal is still correctly assigning fresh ids to sections arising from markdown in the release notes, because the sections appear in the TOC sidebar, and clicking on them still correctly goes to the right url-with-fragment. I think this is where permalinks get created.

To get a permalink, the section has to have a stable identifier. That represents our commitment to keeping it around and keeping that name. Because release notes are basically immutable, this is easy - some mangling of the title and version will suffice. You do this by assigning the tag field of each section's PartMetadata, which will also result in them appearing in the quickjump.

@jcreedcmu
Copy link
Author

To get a permalink, the section has to have a stable identifier. That represents our commitment to keeping it around and keeping that name. Because release notes are basically immutable, this is easy - some mangling of the title and version will suffice. You do this by assigning the tag field of each section's PartMetadata, which will also result in them appearing in the quickjump.

Perfect, that's exactly the information I was interested in.

@jcreedcmu
Copy link
Author

jcreedcmu commented Sep 29, 2025

This PR now depends on leanprover/verso#539 to fix markdown header processing to be actually correct. @jrr6 's original implementation was good enough for the typical error explanations markdown, but had iiuc a slightly subtle bug for repeating headers at the same depth, which my PR aims to fix.

def markdown : PartCommand
| `(Lean.Doc.Syntax.codeblock| ``` $markdown:ident $args*| $txt ``` ) => do
let x <- Lean.Elab.realizeGlobalConstNoOverloadWithInfo markdown
if x != (by exact decl_name%) then Elab.throwUnsupportedSyntax
Copy link
Author

@jcreedcmu jcreedcmu Sep 29, 2025

Choose a reason for hiding this comment

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

Is this (by exact decl_name%) tactic invocation appropriate here? As I understand it, I'm not able to literally write ``markdown because I want to refer to the very def markdown defined on line 32, right? So this is what I guessed would work instead, having seem decl_name% many places elsewhere. It does in fact seem to work, and using MessageData.hint below works to get argument deletion hints as well.

jcreedcmu added a commit to jcreedcmu/verso that referenced this pull request Oct 1, 2025
Make the type `HeaderMapping` an abbreviation instead of a definition.

This change was intended as part of
leanprover#539 and will make
leanprover/reference-manual#599 easier to
land. There's still [one place
remaining](https://github.com/leanprover/reference-manual/blob/main/Manual/Meta/ErrorExplanation.lean#L639-L640)
in `Manual/Meta/ErrorExplanation.lean` where it's relying on the
implementation of this type, which breaks if typeclass resolution
doesn't see the `ForIn` instance on `List`. Of course, we could also
expose `ForIn` on `HeaderMapping`, or expose a "close all sections for
this `HeaderMapping` but I'd rather decouple that decision from [the
present PR](leanprover/reference-manual#599).
jcreedcmu added a commit to leanprover/verso that referenced this pull request Oct 1, 2025
Make the type `HeaderMapping` an abbreviation instead of a definition.

This change was intended as part of
#539 and will make
leanprover/reference-manual#599 easier to
land. There's still [one place
remaining](https://github.com/leanprover/reference-manual/blob/main/Manual/Meta/ErrorExplanation.lean#L639-L640)
in `Manual/Meta/ErrorExplanation.lean` where it's relying on the
implementation of this type, which breaks if typeclass resolution
doesn't see the `ForIn` instance on `List`. Of course, we could also
expose `ForIn` on `HeaderMapping`, or expose a "close all sections for
this `HeaderMapping` but I'd rather decouple that decision from [the
present PR](leanprover/reference-manual#599).
jcreedcmu added a commit to jcreedcmu/verso that referenced this pull request Oct 1, 2025
Make the type `HeaderMapping` an abbreviation instead of a definition.

This change was intended as part of
leanprover#539 and will make
leanprover/reference-manual#599 easier to
land. There's still [one place
remaining](https://github.com/leanprover/reference-manual/blob/main/Manual/Meta/ErrorExplanation.lean#L639-L640)
in `Manual/Meta/ErrorExplanation.lean` where it's relying on the
implementation of this type, which breaks if typeclass resolution
doesn't see the `ForIn` instance on `List`. Of course, we could also
expose `ForIn` on `HeaderMapping`, or expose a "close all sections for
this `HeaderMapping` but I'd rather decouple that decision from [the
present PR](leanprover/reference-manual#599).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Styling: Markdown header levels
3 participants