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

Add support for latest trunk (5.03) #487

Merged
merged 10 commits into from
Jul 5, 2024

Conversation

NathanReb
Copy link
Collaborator

Most of the work comes from @hhugo 's trunk-support-53 branch.

I rebased it all on top of main and reverted some of the formatting changes to instead use the original fromatting from the compiler modules to avoid large diffs if when we update the Ast_503 module in the future.

I also fixed part of the migrations that had the same Pmod_apply_unit migration bug we had on the 5.2 support branch so I suspect the migrations were copied from the 5.02 <-> 5.01 ones.
I think this makes for another argument in favor in importing the omp tools to generate deep copy functions to use as a base for the migration modules. That would prevent this kind of bug.

I haven't reviewed the whole migrations extensively so I'm leaving this as draft for now. The branch can still be useful to people that wish to test trunk.

@NathanReb NathanReb added no changelog Use this label to disable the changelog check github action and removed no changelog Use this label to disable the changelog check github action labels Apr 23, 2024
@NathanReb
Copy link
Collaborator Author

NathanReb commented Apr 24, 2024

I tried this with ocaml-variants.5.3.0+trunk and it builds, most of the tests pass except for some slight changes in some expect tests output but nothing meaningful there. E.g. in error messages Ppxlib.Ast.expression is now reported as Ppxlib.expression.

I need to look into those more to better understand them but it just looks like the error reporting got better on the compiler side and it picks up the global include Ast in ppxlib.ml.

There is also a change in the quoter text which I'm not sure how to interpret yet so I'll look into it.

@kit-ty-kate
Copy link
Contributor

shouldn't this be going into the trunk-support branch instead of main ?

@NathanReb
Copy link
Collaborator Author

shouldn't this be going into the trunk-support branch instead of main ?

No, we're trying to add support gradually on the main branch instead. We'll exclude 5.03 when releasing but main will stay compatible with trunk as much as possible.

@NathanReb NathanReb force-pushed the trunk-support-503 branch from 7e812b0 to 3c51e0c Compare May 21, 2024 07:56
@kit-ty-kate
Copy link
Contributor

ah cool!

@NathanReb NathanReb force-pushed the trunk-support-503 branch from 3c51e0c to dbe3b70 Compare May 22, 2024 12:02
@NathanReb NathanReb marked this pull request as ready for review May 23, 2024 10:04
@NathanReb
Copy link
Collaborator Author

It would be nice to add trunk builds in the CI from there onward. If they could be flagged as optional (as in don't have to be green to merge) that would be even better but I'll take anything.

What's the best way to add that?

@NathanReb
Copy link
Collaborator Author

I guess we could reuse #410

@NathanReb
Copy link
Collaborator Author

Just rebased on top of the new trunk CI build addition, let's see how it goes!

@NathanReb
Copy link
Collaborator Author

The trunk build seems to work as expected and even picked up an error, I'll look into it!

@NathanReb
Copy link
Collaborator Author

I pushed support for the latest changes to location.

Our limited preprocessor to use different code branches based on compiler versions is reaching its limits I think but is still holding up.

I think we can go ahead and review this. @Octachron if you want to look at the latest commit that adds compat for your latest changes to the compiler, there might be room for improvement there!

@@ -112,4 +124,6 @@ module Error = struct
(*IF_AT_LEAST 408 _set_main_loc_new error loc*)
end

let raise_errorf ?loc msg = raise_errorf ?loc msg
let raise_errorf ?loc msg =
(*IF_AT_LEAST 503 Format.kasprintf (fun s -> raise_errorf ?loc "%s" s) msg *)
Copy link
Contributor

Choose a reason for hiding this comment

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

This part looks problematic: you are rendering to a string too early.
I can see two options:

  • either update the type of raise_errorf to the new printers from Format_doc
  • or use the deprecated escape-hatch from Format_doc:
Format.kdprintf (fun pr -> raise_errorf ?loc "%t" (Format_doc.deprecated_printer pr)) msg

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that the escape hatch is here to stay, but it will impede serialization of error reports in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah thanks! I'll update to you suggestion. We unfortunately cannot update the type of raise_errorf since we both need to stay compatible with a range of OCaml versions in which Format_doc does not exist and the type change would be a breaking change (at least for some users, depending on how raise_errorf is used I expect in some cases it would be transparent).

We'll have to support it's current form for a little while.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It also seems that we'll need to bump our lower bound to 4.08 to use Format.kdprintf. This is something we could consider doing though.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is a bit cumbersome but ppxlib could also vendor the definition of kdprintf:

open CamlinternalFormat
open Format
open CamlinternalFormatBasics

let compute_tag output tag_acc =
  let buf = Buffer.create 16 in
  let ppf = formatter_of_buffer buf in
  output ppf tag_acc;
  pp_print_flush ppf ();
  let len = Buffer.length buf in
  if len < 2 then Buffer.contents buf
  else Buffer.sub buf 1 (len - 2)
let pp_open_box_gen ppf indent box_type =
  match box_type with
  | Pp_vbox -> pp_open_vbox ppf indent
  | Pp_hbox | Pp_fits -> pp_open_hbox ppf ()
  | Pp_hovbox -> pp_open_hovbox ppf indent
  | Pp_hvbox -> pp_open_hvbox ppf indent
  | Pp_box -> pp_open_box ppf indent
 (* Interpret a formatting entity on a formatter. *)
let output_formatting_lit ppf fmting_lit = match fmting_lit with
  | Close_box                 -> pp_close_box ppf ()
  | Close_tag                 -> pp_close_stag ppf ()
  | Break (_, width, offset)  -> pp_print_break ppf width offset
  | FFlush                    -> pp_print_flush ppf ()
  | Force_newline             -> pp_force_newline ppf ()
  | Flush_newline             -> pp_print_newline ppf ()
  | Magic_size (_, _)         -> ()
  | Escaped_at                -> pp_print_char ppf '@'
  | Escaped_percent           -> pp_print_char ppf '%'
  | Scan_indic c              -> pp_print_char ppf '@'; pp_print_char ppf c
let rec output_acc ppf acc = match acc with
  | Acc_string_literal (Acc_formatting_lit (p, Magic_size (_, size)), s)
  | Acc_data_string (Acc_formatting_lit (p, Magic_size (_, size)), s) ->
    output_acc ppf p;
    pp_print_as ppf size s;
  | Acc_char_literal (Acc_formatting_lit (p, Magic_size (_, size)), c)
  | Acc_data_char (Acc_formatting_lit (p, Magic_size (_, size)), c) ->
    output_acc ppf p;
    pp_print_as ppf size (String.make 1 c);
  | Acc_formatting_lit (p, f) ->
    output_acc ppf p;
    output_formatting_lit ppf f;
  | Acc_formatting_gen (p, Acc_open_tag acc') ->
    output_acc ppf p;
    pp_open_stag ppf (String_tag (compute_tag output_acc acc'))
  | Acc_formatting_gen (p, Acc_open_box acc') ->
    output_acc ppf p;
    let (indent, bty) = open_box_of_string (compute_tag output_acc acc') in
    pp_open_box_gen ppf indent bty
  | Acc_string_literal (p, s)
  | Acc_data_string (p, s)   -> output_acc ppf p; pp_print_string ppf s;
  | Acc_char_literal (p, c)
  | Acc_data_char (p, c)     -> output_acc ppf p; pp_print_char ppf c;
  | Acc_delay (p, f)         -> output_acc ppf p; f ppf;
  | Acc_flush p              -> output_acc ppf p; pp_print_flush ppf ();
  | Acc_invalid_arg (p, msg) -> output_acc ppf p; invalid_arg msg;
  | End_of_acc               -> ()

let kdprintf k (Format (fmt, _)) =
  make_printf
    (fun acc -> k (fun ppf -> output_acc ppf acc))
    End_of_acc fmt

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we can actually consider requiring 4.08+. In the meantime I suggest we merge the 5.3 support as it is and we have time to improve this later on!

If we do end up choosing to require 4.08+ we can then simply use your suggested implementation, using kdprintf!

Hugo Heuzard and others added 6 commits July 5, 2024 08:36
Signed-off-by: Nathan Rebours <[email protected]>
This must have been copied from the 5.02 <-> 5.01 migrations
which were themselves faulty.

That's one more good reason to import the deep copy generation tool
from omp to generate the base for the migrations.

Signed-off-by: Nathan Rebours <[email protected]>
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