Refactor Printtyp: split it in three modules#13336
Conversation
gasche
left a comment
gasche
left a comment
There was a problem hiding this comment.
I agree that the result is better than with my approach (which I suppose inspired commit three, "expose compatibility printers first"), thanks! I left a reasonable amount of review comments, especially on the documentation (moved or new).
.depend
Outdated
| utils/format_doc.cmi | ||
| typing/includeclass.cmo : \ | ||
| typing/types.cmi \ | ||
| typing/typerror_report.cmi \ |
There was a problem hiding this comment.
Not fond of the name Typerror_report -- it's three words with two different word-break conventions, and unidiomatic punning of "type error" into "typerror". Maybe Type_error_report or even Report_type_error? I would also find Errortrace_report somewhat more natural (despite also being three words juxtaposed in two different ways), as an extension of the pre-existing module Errortrace. (the disambiguation error does not come from Errortrace, but oh well.) Or just Report_error?
There was a problem hiding this comment.
I like Errortrace_report since the functions are mostly consumers of error traces, which suggests to move amniguous_type to Typecore.
typing/out_type.mli
Outdated
| have been passed to {!prepare_for_printing}. Unlike {!Printtyp.type_expr}, | ||
| this function does no extra work before printing a type; in particular, this | ||
| means that any loops in the type expression may cause a stack overflow (see | ||
| #8860) since this function does not mark any loops. The benefit of this is |
There was a problem hiding this comment.
I now that this comment existed before, but on re-reading here in the context of the global Out_type documentation this is a bit confusing.
Out_type above explains that it tracks context for handling cycles/loops (note: maybe it would be nice to use 'loops' consistently, or 'cycles' consistently), and now we explain that loops are not handled by this function. A reader may naively assume that there is no way to call prepared_type_expr on a type expression that may contain loops, and that another printing function should be used instead. What I understand is that we don´t mean that, we want to point out that the expression should be prepared first.
I think that it should be possible to rephrase the current documentation commend to explain, more directly, that this expects a function that has already been prepared by using prepare_for_printing or add_type_to_preparation.
There was a problem hiding this comment.
Indeed, I agree that the current formulation can be simplified now that prepared_type_expression is not in competition for reader attention in the same module as type_expression.
typing/out_type.mli
Outdated
| (** Add external type equalities*) | ||
| val add_subst: (type_expr * type_expr) list -> unit | ||
|
|
||
| (** [reserve ty] register the variable names appearing in [ty] *) |
typing/typerror_report.ml
Outdated
| mark_loops x; | ||
| mark_loops y; | ||
| Aliases.mark_loops x; | ||
| Aliases.mark_loops y; |
There was a problem hiding this comment.
Naive question: why don't we just call List.iter add_type_to_preparation [x; y] here?
There was a problem hiding this comment.
This should work, and should be enough to remove the Aliases module from the Out_types module.
typing/out_type.mli
Outdated
| (** {1 Printing type expressions} *) | ||
|
|
||
| (** [prepare_for_printing] resets the global naming environment, a la {!reset}, | ||
| and prepares the types for printing by reserving names and marking loops. |
typing/out_type.mli
Outdated
| type variables. *) | ||
|
|
||
| (** [type_expr_with_reserved_names ty] marks loop in [ty] but share names and | ||
| ident conflicts with the current variable names context. *) |
There was a problem hiding this comment.
I found the comment on half-preparation somewhere else useful, maybe it could be partially included here. (Before calling this function, you should reserve variable names and not mark loops; this can be deduced from the current documentation but it is not completely obvious and could be pointed out).
There was a problem hiding this comment.
I added the comments, but I am starting to wonder if the function is still needed.
Octachron
force-pushed
the
split_printtyp
branch
from
0352be2 to
992bdea
Compare
typing/out_type.mli
Outdated
| Calling this function on non-prepared types may cause a stack overflow (see | ||
| #8860) due to cycles in the printed types. | ||
|
|
||
| See {!Printtyp.type_expr} for a safer printer. *) |
|
I'm happy with the new batch of commits. I wonder if you have plans to cleanup the history. Except for the history, this is now good to merge. |
Octachron
force-pushed
the
split_printtyp
branch
from
992bdea to
4f48bdf
Compare
|
I have cleanup the history, I propose to merge by the end of the week to give time for people to raise objections to the refactoring. |
|
(My gut feeling is that the proposed interface change is a notable improvement, and that the compatibility-restoration part will help people building their code on top of |
This reverts commit e2fa976. The ocaml/ocaml#13336 PR restored backward compatibility for the `Printtyp` module and the patch to `base` is no longer required.
This reverts commit e2fa976. The ocaml/ocaml#13336 PR restored backward compatibility for the `Printtyp` module and the patch to `base` is no longer required.
2 participants
This PR proposes to split the
Printtypmodule in three modules:Out_typemodule focused on generating outcome trees (aka close to source tree type expressions) from type expression digraphs. In particular, this is the module that contains the various naming and loop marking contextPrinttypmodule which only retains printing functions that are safe to use in all circumstances (with restored backward compatibility)Typerror_reportmodule for generating reports for core-level type errors.The first two commits in this PR splits the
Printtypmodule in two (with minimal code changes).The third one restores backward compatibility with
Formatprinters for thePrinttypprinters. This my alternative proposition to #13319.The fourth commit move a specialized function to its only user in
Includemod_errorprinterThe fifth and last commit reorganizes the
Out_typeandPrinttypmodules to provide better documentation and moreself-documenting names. In particular, the four naming contexts (for idents, type variables, internal names and aliases) are named more explicitly.