Skip to content

gprinttyp: a graphical debugging printer for types#13049

Merged
Octachron merged 3 commits intoocaml:trunkfrom
Octachron:gprinttyp_core
Jul 29, 2024
Merged

gprinttyp: a graphical debugging printer for types#13049
Octachron merged 3 commits intoocaml:trunkfrom
Octachron:gprinttyp_core

Conversation

@Octachron
Copy link
Copy Markdown
Member

This purely internal PR proposes to add a Gprinttyp module which can print faithfully type expressions as digraphs using the graphviz format.

This module is intended as a helper for typechecker debugging session (see 71dd0d0 for an example of use case), since graph representation scales better than purely textual representation when there are more than a dozen of nodes involved.

As an illustration, the digraph representation for

val f : (< m : 'a; w : < n : 'a; z : 'b; .. > as 'b; .. > as 'a) -> 'b -> 'a list * 'b list

is
0182-unify-f-l1 4-5

which is still readable even with the 16 nodes of the type expressions, and the few empty universal type variable binders.

Since printing digraphs on stdout does not work that well when printing hundreds of graphs (my debugging session in #13004 typically produced between 100 and 600 graphs by execution trace), the interface of Gprinttyp is oriented toward dumping files in a directory specified by dump-dirs, with few helpers functions that helps to provide context.

All feature of the type expressions are covered (type expansions, object fields and polymorphic variants included) and there are few knobs exposed for tuning the level of detail of the graph, and it should be straightforward to add more, in particular for printing levels and scopes.

I imagine that I should mention that the unicode characters in the printers can be removed if required.

@gasche
Copy link
Copy Markdown
Member

gasche commented Mar 25, 2024

This is a nice idea and I agree in principle. I would expect a review for readability (not "correctness", this is debug-only and we don't care if there is a slight issue here or there) before we can merge.

I don't quite follow the actual example (as always): f has a function type with two arguments (... -> ... -> ...), but I see only one arrow node in your graph. I am also not sure what , 7 represents, is it the product type? (Then maybe * would be more clear.)

@Octachron
Copy link
Copy Markdown
Member Author

Octachron commented Mar 25, 2024

Oops, I picked a non-representative subgraph, here is a graph of an unification step with the two arrows present.
0169-unify-f-l1 4-5

The , nodes are indeed tuples. I agree that a x or * would be more readable.

Copy link
Copy Markdown
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

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

I did a quick review (I did not read the dot-producing code in full details). This is the usual nice, slightly over-engineered Florian style and I trust it to perform above expectations at printing graphs from types. Thank you!

I think that you should split the change to match_row_field and the row_field_cell type to a separate preparatory commit.

I will be happy to approve once you have done a pass taking the review comments into account. (The most invasive one is to mass-rewrite ellide into elide.)

Comment thread typing/gprinttyp.ml Outdated
Comment thread typing/gprinttyp.ml Outdated
Comment thread typing/gprinttyp.ml Outdated
Comment thread typing/gprinttyp.ml Outdated
let _index params ty = pretty_id params (repr params ty).id
let field ~name x = match x with
| Main id -> Field {id;name;synth=false}
| Field r -> Field {r with name}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't understand the logic here -- I'm not sure what field is and from a distance I would have expected that the field baz of foo.bar is foo.bar.baz, not foo.baz. No big deal, but maybe a comment. Also when reading that I wondered if we should have Field of t * string instead of your weird definition, but I'm fine with the weird definition as well.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I have renamed the constructor to Named_subnode to make it clearer that those index are associated to a main index and that the subnode of a subnode is not really supported at this point.

Comment thread typing/gprinttyp.ml
Comment thread typing/gprinttyp.ml Outdated
Comment thread typing/gprinttyp.ml Outdated
Comment thread typing/gprinttyp.ml Outdated
@gasche gasche self-assigned this Apr 3, 2024
Copy link
Copy Markdown
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

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

This looks good to me now. I think you could cleanup the history my merging the N-2 middle commits in one.

Comment thread typing/gprinttyp.ml Outdated
| "p" -> "𝜋"
| "i" -> "𝜄"
| "h" -> "𝜂"
| "k" -> "k"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't that be a kappa (κ or one of the other versions) ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Indeed (even if κ and k are quite confusable depending on the font).

Comment thread typing/gprinttyp.ml Outdated
Comment on lines +415 to +422
| "r" -> "𝜌"
| "s" -> "𝜎"
| "p" -> "𝜋"
| "i" -> "𝜄"
| "h" -> "𝜂"
| "k" -> "κ"
| "l" -> "𝜆"
| "m" -> "μ"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
| "r" -> "𝜌"
| "s" -> "𝜎"
| "p" -> "𝜋"
| "i" -> "𝜄"
| "h" -> "𝜂"
| "k" -> "κ"
| "l" -> "𝜆"
| "m" -> "μ"
| "r" -> "𝜌"
| "s" -> "𝜎"
| "p" -> "𝜋"
| "i" -> "𝜄"
| "h" -> "𝜂"
| "k" -> "𝜅"
| "l" -> "𝜆"
| "m" -> "𝜇"

If I'm not mistaken, you're using Unicode's Mathematical Bold Italic Small symbols, from U+1D736 to U+1D714. The kappa and mu were not exactly consistent…

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Indeed, this is better, thanks!

@Octachron
Copy link
Copy Markdown
Member Author

While squashing the history, I have added a basic description of the graph format; since I had been told that the format might be a bit too arcane to decipher without any explanation.

@gasche
Copy link
Copy Markdown
Member

gasche commented Jul 20, 2024

@Octachron I suggest that we merge after you fix the conflicts.

@gasche
Copy link
Copy Markdown
Member

gasche commented Jul 26, 2024

Thanks for the rebase. CI report:

File "/home/runner/work/ocaml/ocaml/typing/rawprinttyp.ml", line 140, characters 23-27:
140 |           match e with None -> fprintf ppf " RFnone"
                             ^^^^
Error: This pattern should not be a constructor, the expected type is
       Types.row_field_cell * Types.row_field option

@Octachron
Copy link
Copy Markdown
Member Author

Yep, I fell to the hubris of not checking the rebasing, this should be fixed now.

@Octachron Octachron merged commit 48d3c5a into ocaml:trunk Jul 29, 2024
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.

4 participants