Skip to content

Format_doc: preserve the type of Foo.report_error, add Foo.report_error_doc#13311

Merged
Octachron merged 3 commits intoocaml:trunkfrom
gasche:report-error
Jul 18, 2024
Merged

Format_doc: preserve the type of Foo.report_error, add Foo.report_error_doc#13311
Octachron merged 3 commits intoocaml:trunkfrom
gasche:report-error

Conversation

@gasche
Copy link
Member

@gasche gasche commented Jul 17, 2024

The introduction of Format_doc in #13169 changed the type of various 'report_error' functions in the compiler codebase. But this breaks user code. An alternative proposed here is to keep 'report_error' at the same type as before and introduce 'report_error_doc' in addition.

(I had a discussion with @Octachron this morning on how long it would take to write this PR. For now my current time is 40mn.)

@gasche
Copy link
Member Author

gasche commented Jul 17, 2024

One thing that makes me think is that there are other source of breakage introduced in #13169 than the change of type to those functions, see for example janestreet/base#166 : #13169 changes the type of other printer-ish types in compiler-libs, that may affect even more downstream users: github search for Printtyp.signature. I don't know if I want to apply the same approach to those changes as well, and whether it is reasonable to do this consistently. Probably?

exception Error of error

val report_error: formatter -> error -> unit

Copy link
Member

Choose a reason for hiding this comment

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

Accidental noise

type 'a format_printer = Format.formatter -> 'a -> unit
val compat: 'a printer -> 'a format_printer
val compat1: ('p1 -> 'a printer) -> ('p1 -> 'a format_printer)
val compat2: ('p1 -> 'p2 -> 'a printer) -> ('p1 -> 'p2 -> 'a format_printer)
Copy link
Member

Choose a reason for hiding this comment

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

If you want to have various compat for different arity, I have seen a case of compat0: (Format_doc.formatter -> unit) -> Format.formatter -> unit in the wild

Copy link
Member

@Octachron Octachron left a comment

Choose a reason for hiding this comment

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

The change is clearly correct, and I can grumpily agree that error reporters are part of the more public part of compiler-libs.

@gasche
Copy link
Member Author

gasche commented Jul 17, 2024

I implemented compat0, but I failed to find a single use-case in the compiler -- so I'm not sure my implementation correct. I decided to not include it in this PR, and let you push it if you like; my implementation is in gasche/ocaml@report-error...report-error-format0 . (I tried to use it in two places that were using %t before your PR, namely pp_txt in location.ml:745 and str_of_msg in ast_mapper.ml:837, but could not make it work.)

gasche added 3 commits July 17, 2024 23:04
…or_doc

The introduction of Format_doc changed the type of various
'reprot_error' functions in the compiler codebase. But this breaks
user code. An alternative proposed here is to keep 'report_error'
at the same type as before and introduce 'report_error_doc' in addition.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants