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

refactor: hyperlink more notations such as field notations #261

Closed
wants to merge 3 commits into from

Conversation

Komyyy
Copy link
Contributor

@Komyyy Komyyy commented Jan 10, 2025

Screenshot from 2025-01-10 14-18-35

Current DocGen hyperlink system is as following:

(Lean.)Expr
  -(DocGen4.Process.)prettyPrintTerm→
(Lean.Widget.)CodeWithInfos
  -(DocGen4.Output.)docInfoToHtml→
(DocGen4.)Html

CodeWithInfos is text sent to the infoview to display terms with additional datas when hovered.

For example, let's see what CodeWithInfos sent to the infoview when it display List.insert 0 [1, 2].

.tag
  { expr := (q(List.insert 0 [1, 2]) : Expr), subexprPos := Pos.fromString! "/", .. }
  (.append
    #[.text "List.insert ",
      .tag
        { expr := (q(0) : Expr), subexprPos := Pos.fromString! "/0/1", .. }
        (.text "0"),
      .text " ",
      .tag
        { expr := (q([1, 2]) : Expr), subexprPos := Pos.fromString! "/1", .. }
        (.append
          #[.text "[",
            .tag
              { expr := (q(1) : Expr), subexprPos := Pos.fromString! "/1/0/1", .. }
              (.text "1"),
            .text ", ",
            .tag
              { expr := (q(2) : Expr), subexprPos := Pos.fromString! "/1/1/0/1", .. }
              (.text "2"),
            .text "]"])])

prettyPrintTerm does the same process when display terms on the infoview, but DocGen runs prettyPrintTerm with set_option pp.tagAppFns true; this option makes constant application tagged with its constant. So, in the last example, #[.text "List.insert", ..] becomes #[L.tag { info := ⟨{ info := { expr := (q(@List.insert) : Expr), .. }, .. }⟩, .. } (.text "List.insert"), .text " ", ..].

Then, docInfoToHtml hyperlinks text tagged with constants.

However, current DocGen doesn't hyperlink field notations; .Nodup in [0, 1, 2].Nodup isn't hyperlinked to List.Nodup, this is because pp.tagAppFns doesn't tag field notations.

This problem comes from the limitation of pp.tagAppFns, so the solution is refactoring pp.tagAppFns to tag field notations, or refactoring docInfoToHtml to hyperlink terms without relying on pp.tagAppFns, so I chose the latter solution.

The new docInfoToHtml hyperlinks CodeWithInfos without constant tags, if decInfoToHtml find a constant application tag, then it checks whether all tags of explicit arguments of the tag appear in direct children. If so, it hyperlinks non-tagged text in direct children.

It hyperlinks more notations such as field notations.

As a special case, when fun x => t(x) appears as a explicit argument, docInfoToHtml allows to appear t(x) instead of fun x => t(x), to hyperlink binder notations like { s : String // 0 < s.length }.

Improvement points

Screenshot from 2025-01-10 15-06-39

Sometimes, parenthesis maybe hyperlinked. This is because docInfoToHtml hyperlinks text according to info.

resolves #91, resolves #224

@Komyyy
Copy link
Contributor Author

Komyyy commented Jan 10, 2025

My English is maybe awful, editing the PR message is very welcome!

@kmill
Copy link
Contributor

kmill commented Jan 20, 2025

I've fixed field notation linking by modifying Lean core: leanprover/lean4#6703

@Komyyy
Copy link
Contributor Author

Komyyy commented Jan 20, 2025

@kmill Thank you!
@hargoniX Which way do you want to hyperlink field notations by? Field notations will be hyperlinked by leanprover/lean4#6703, but this PR also hyperlinks many other notations like binder notations, but sometimes parenthesis maybe hyperlinked.

@hargoniX
Copy link
Collaborator

Hi, first of all thanks for putting in the effort. I had put off reviewing this PR last week because it seems like a pretty substantial change that needs careful consideration. I think I would probably prefer not touching this code if it is possible. We've tested the current version on a lot of code from the eco system and a lot of users have (rather passively by just using it) tested that this provides correct linkage.

Note that the tagAppsFn feature was introduced specifically for doc-gen so making changes around it specifically for doc-gen is probably acceptable.

@kmill
Copy link
Contributor

kmill commented Jan 20, 2025

Sometimes, parenthesis maybe hyperlinked

I had tried making a similar change a month ago, and I also ran into this parenthesis hyperlinking issue. I thought it was a big enough problem that I abandoned the changes.

@Komyyy
Copy link
Contributor Author

Komyyy commented Jan 21, 2025

@hargoniX OK, I close this PR but don't remove the branch.

@Komyyy Komyyy closed this Jan 21, 2025
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.

Declarations not hyperlinked Binder symbols in Exists is not linked
3 participants