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.

Sign up for GitHub

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

Clean up super errors in syntax #6246

Merged
merged 3 commits into from
May 10, 2023
+26 −403
Merged

Clean up super errors in syntax #6246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e5d1738
clean up super errors in syntax
mununki May 7, 2023
25eaf61
changelog
mununki May 8, 2023
04bfb11
Merge branch 'master' into clean-up-syntax-super-error
mununki May 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@
#### :rocket: Main New Feature
- Make uncurried mode opt-out: by default, every project is now in uncurried mode, unless `"uncurried": false` is specified in the project config. https://github.com/rescript-lang/rescript-compiler/pull/6249

#### :nail_care: Polish
- Removed duplicate Super_error implementation in syntax https://github.com/rescript-lang/rescript-compiler/pull/6246

# 11.0.0-alpha.6

#### :boom: Breaking Change
Expand Down
26 changes: 18 additions & 8 deletions jscomp/ml/location.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ let print_loc ppf (loc : t) =
fprintf ppf "@{<filename>%a@}%a" print_filename loc.loc_start.pos_fname dim_loc normalized_range
;;

let print ~message_kind intro ppf (loc : t) =
let print ?(src = None) ~message_kind intro ppf (loc : t) =
begin match message_kind with
| `warning -> fprintf ppf "@[@{<info>%s@}@]@," intro
| `warning_as_error -> fprintf ppf "@[@{<error>%s@} (configured as error) @]@," intro
Expand Down Expand Up @@ -193,7 +193,12 @@ let print ~message_kind intro ppf (loc : t) =
| None -> ()
| Some _ -> begin
try
let src = Ext_io.load_file file in
(* Print a syntax error that is a list of Res_diagnostics.t.
Instead of reading file for every error, it uses the source that the parser already has. *)
let src = match src with
| Some src -> src
| None -> Ext_io.load_file file
in
(* we're putting the line break `@,` here rather than above, because this
branch might not be reached (aka no inline file content display) so
we don't wanna end up with two line breaks in the the consequent *)
Expand Down Expand Up @@ -329,17 +334,22 @@ let error_of_exn exn =

(* taken from https://github.com/rescript-lang/ocaml/blob/d4144647d1bf9bc7dc3aadc24c25a7efa3a67915/parsing/location.ml#L380 *)
(* This is the error report entry point. We'll replace the default reporter with this one. *)
let rec default_error_reporter ppf ({loc; msg; sub}) =
let rec default_error_reporter ?(src = None) ppf ({loc; msg; sub}) =
setup_colors ();
(* open a vertical box. Everything in our message is indented 2 spaces *)
Format.fprintf ppf "@[<v>@, %a@, %s@,@]" (print ~message_kind:`error "We've found a bug for you!") loc msg;
List.iter (Format.fprintf ppf "@,@[%a@]" default_error_reporter) sub
(* If src is given, it will display a syntax error after parsing. *)
let intro = match src with
| Some _ -> "Syntax error!"
| None -> "We've found a bug for you!"
in
Format.fprintf ppf "@[<v>@, %a@, %s@,@]" (print ~src ~message_kind:`error intro) loc msg;
List.iter (Format.fprintf ppf "@,@[%a@]" (default_error_reporter ~src)) sub
(* no need to flush here; location's report_exception (which uses this ultimately) flushes *)

let error_reporter = ref default_error_reporter

let report_error ppf err =
!error_reporter ppf err
let report_error ?(src = None) ppf err =
!error_reporter ~src ppf err
;;

let error_of_printer loc print x =
Expand Down Expand Up @@ -375,7 +385,7 @@ let rec report_exception_rec n ppf exn =
match error_of_exn exn with
| None -> reraise exn
| Some `Already_displayed -> ()
| Some (`Ok err) -> fprintf ppf "@[%a@]@." report_error err
| Some (`Ok err) -> fprintf ppf "@[%a@]@." (report_error ~src:None) err
Copy link
Member Author

@mununki mununki May 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I've tried a callback to remove the dependency on the Ext_io module, but it's hard to implement. Here's the problem. Since it uses %a, I can't omit it even if I use an optional arg with a defalt value, and I think I'll have to use something like ~src:Ext_io.load_file if we use a callback. That means we cann't remove the Ext_io dependency anyway.

And since Jsoo_main and Jsoo_playground are already using Location.report_error function, I'd like to check with @ryyppy about the dependency on Ext_io.

Copy link
Member

Choose a reason for hiding this comment

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

I recalled that I had to copy / paste some syntax diagnostics code to the playground so I can circumvent the IO issue. I am sure JSOO has a way to replace the Ext_io module with a different impl somehow?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, so referencing Ext_io is indeed an issue. Good to know, I wasn't sure.

One could also do something like

Location.load_source_file := Ext_io.load_file

on (non-JSOO) compiler startup.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How about skip Ext io.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or make it platform independent. (After checking if there's some actually observable perf difference).

Copy link
Member

Choose a reason for hiding this comment

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

This is the code I essentially copied and adapted to get rid of the IO call: https://github.com/rescript-lang/rescript-compiler/blob/5fef8d33fd29bf6ddb3905a687a13c0c9ea147bd/jscomp/jsoo/jsoo_playground_main.ml#L222-L243

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks.
I'm looking into the Jsoo_playground_main. It is already dependent to the previous Super_errors. I'm still not sure if copy/pasting that part would have avoided the IO call.
I think the playground stuff in CONTRIBUTING.md is a bit out of date, but I'll follow it and give the post-build js a spin to check.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I found this #6201

Copy link
Member Author

@mununki mununki May 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@ryyppy I applied the changes from PR #6201 to this PR and replaced Res_diagnostics_printing_utils.Super_location.super_error_reporter with Location.report_error. Then I built the playground and ran the tests, which resulted in a stack overflow first. When I commented out the part using React.useState, the tests passed.
Does this mean we can consider there are no issues related to the Ext_io dependency?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, if you get to this point, it means the playground compiled successfully. The stack overflow issue is unrelated.

with exn when n > 0 -> report_exception_rec (n-1) ppf exn

let report_exception ppf exn = report_exception_rec 5 ppf exn
Expand Down
8 changes: 4 additions & 4 deletions jscomp/ml/location.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ type 'a loc = {
val mknoloc : 'a -> 'a loc
val mkloc : 'a -> t -> 'a loc

val print: message_kind:[< `error | `warning | `warning_as_error > `warning] -> string -> formatter -> t -> unit
val print: ?src:string option -> message_kind:[< `error | `warning | `warning_as_error > `warning] -> string -> formatter -> t -> unit
val print_compact: formatter -> t -> unit
val print_filename: formatter -> string -> unit

Expand Down Expand Up @@ -131,12 +131,12 @@ val register_error_of_exn: (exn -> error option) -> unit
a location, a message, and optionally sub-messages (each of them
being located as well). *)

val report_error: formatter -> error -> unit
val report_error: ?src:string option -> formatter -> error -> unit

val error_reporter : (formatter -> error -> unit) ref
val error_reporter : (?src:string option -> formatter -> error -> unit) ref
(** Hook for intercepting error reports. *)

val default_error_reporter : formatter -> error -> unit
val default_error_reporter : ?src:string option -> formatter -> error -> unit
(** Original error reporter for use in hooks. *)

val report_exception: formatter -> exn -> unit
Expand Down
3 changes: 1 addition & 2 deletions jscomp/syntax/src/res_diagnostics.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,7 @@ let printReport diagnostics src =
match diagnostics with
| [] -> ()
| d :: rest ->
Res_diagnostics_printing_utils.Super_location.super_error_reporter
Format.err_formatter src
Location.report_error ~src:(Some src) Format.err_formatter
Location.
{
loc = {loc_start = d.startPos; loc_end = d.endPos; loc_ghost = false};
Expand Down
Loading