Clean up super errors in syntax #6246

mununki · 2023-05-06T17:21:52Z

This PR removes the super_errors that were left in the syntax. The main changes are as follows:

~~Use Location.register_error_of_exn instead of Res_diagnostics.printReport to handle syntax error as a Location.error~~ Use Location.report_error instead of duplicated implementation of super_errors in res_diagnostics_printing_utils. To do so, the interface is modified to allow you to pass an src that the parser already has as an optional labeled argument.
(removed the stale codes)
Remove res_diagnostics_printing_utils.ml

Things to check

Playground
Ext_io in Location module would be fine in the playground.
~~The test in jscomp/syntax/tests/parsing/errors/* prints a fatal error, so maybe we need to move it to jscomp/build_tests/super_errors.~~

cristianoc · 2023-05-07T00:18:24Z

I think this should be a lightweight refactoring without any semantic change.

Syntax errors are different from type errors and we should not mix them lightly.
It's possible to explore but it would go onto some separate issue.

I think this one should just move some functionality around if possible.

CC @

cristianoc · 2023-05-07T00:18:45Z

CC @cknitt

mununki · 2023-05-07T01:40:22Z

I think this should be a lightweight refactoring without any semantic change.

Syntax errors are different from type errors and we should not mix them lightly. It's possible to explore but it would go onto some separate issue.

I think this one should just move some functionality around if possible.

CC @

Okay, that's good to hear.
I was just wondering what the consensus was, given the size of this refactoring and the fundamental change to the existing syntax error(Diagnostics).
So, do you think we just keep the super error for syntax errors? Actually, the error handling for syntax is a bit special, so I think it's probably better to keep it. And super errors for type checking are mixed to normal errors, we might think that #5879 is no longer an issue.

cristianoc · 2023-05-07T01:43:41Z

I think this should be a lightweight refactoring without any semantic change.
Syntax errors are different from type errors and we should not mix them lightly. It's possible to explore but it would go onto some separate issue.
I think this one should just move some functionality around if possible.
CC @

Okay, that's good to hear. I was just wondering what the consensus was, given the size of this refactoring and the fundamental change to the existing syntax error(Diagnostics). So, do you think we just keep the super error for syntax errors? Actually, the error handling for syntax is a bit special, so I think it's probably better to keep it. And super errors for type checking are mixed to normal errors, we might think that #5879 is no longer an issue.

I think if there is some real code duplication/cleanup to be done, this is the time.
But in terms of functionality, no change.
So it might be that #5879 is done, depending on whether any duplication/cleanup is required.

Let's see what @cknitt thinks.

cknitt · 2023-05-07T08:44:38Z

When I originally created this issue it was about the duplication between super_location with

(* Format.formatter -> Location.error -> unit *)
let rec super_error_reporter ppf ({loc; msg; sub} : Location.error) = ...

and res_diagnostics_printing_utils with

(* Format.formatter -> string -> Location.error -> unit *)
let super_error_reporter ppf src ({loc; msg} : Location.error) = ...

Same functionality except that the first one would read the source from the source file and the second one would get it passed as a string.

Now (after the refactorings in #6199 and before this PR) as far as I can see Location.super_errors is gone, but the function that it invokes is still duplicated (Res_diagnostics_printing_utils.print vs. Location.print). I think this duplication should be resolved in a minimally invasive manner and further refactorings done in a separate PR.

mununki · 2023-05-07T08:53:56Z

In my opinion, they are not redundant. Res_diagnostics_print* is a special implementation for printing Res_diagnostics.error, so it requires a source, unlike Location.print. As I understand it, in order to use Location.print to handle Location.error, it would have to be handled as an exn, like the implementation in this PR.

cknitt · 2023-05-07T09:12:49Z

You probably have looked into this into much more detail than me, but isn't it possible to unify this in some other way?

The source is loaded conditionally here:
https://github.com/rescript-lang/rescript-compiler/blob/c646742680031214ab41bdd0472bac52baa36a8c/jscomp/ml/location.ml#L196

Couldn't we do something like passing a callback for obtaining the source as an additional argument to the print function? So in one case it would be loaded from a file and in the other case it would just use the provided string? (Provided there are really no other differences in the functions.)

mununki · 2023-05-07T10:21:58Z

You probably have looked into this into much more detail than me, but isn't it possible to unify this in some other way?

The source is loaded conditionally here:

https://github.com/rescript-lang/rescript-compiler/blob/c646742680031214ab41bdd0472bac52baa36a8c/jscomp/ml/location.ml#L196

Couldn't we do something like passing a callback for obtaining the source as an additional argument to the print function? So in one case it would be loaded from a file and in the other case it would just use the provided string? (Provided there are really no other differences in the functions.)

Ah I missed that part, unifying Res_diagnostics_printing_utils.Super_errors.print and Location.print seems possbile using the src as optional labeled arg. Res_diagnostics_printing_utils.Super_code_frame.print and Code_frame.print looks little different to me though, which are invoked inside print functions each. It seems like we can just look at the test output and judge.
I guess Res_diagnostics_printing_utils*print can have src string without IO, becuse printing multiple errors is executed after parsing, not like type checking modules.
Let me try with less invasive way.

cknitt · 2023-05-07T10:50:08Z

Let me try with less invasive way.
👍

I mentioned a callback instead of an optional arg because I was wondering whether we'd be dragging the dependency on Ext_io.load_file into the playground otherwise. Not sure if that's an issue though.

mununki · 2023-05-07T12:50:50Z

Let me try with less invasive way.
👍

I mentioned a callback instead of an optional arg because I was wondering whether we'd be dragging the dependency on Ext_io.load_file into the playground otherwise. Not sure if that's an issue though.

Ah! Good point! Let me refactor that way.

cknitt

Nice! 👍

My only possible concern is as mentioned whether this causes the dependency on Ext_io.load_file to be dragged into the playground and whether this might cause any issues.

But I'd say let's merge and maybe @ryyppy can rebase his ongoing playground PR and check?

mununki · 2023-05-07T18:11:08Z

jscomp/ml/location.ml

@@ -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


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.

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?

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.

How about skip Ext io.

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

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

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.

Ah, I found this #6201

@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?

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

ryyppy · 2023-05-08T18:49:16Z

One thing that confuses me: Wasn't the res_diagnostics_printing_utils more up-to-date than the original super_error error reporters?

mununki · 2023-05-09T01:03:56Z

One thing that confuses me: Wasn't the res_diagnostics_printing_utils more up-to-date than the original super_error error reporters?

If by "more up-to-date" you mean different output, in fact, the output of the old super_error mixed with Location.default_error_reporter is no different from the original test-syntax output.

ryyppy · 2023-05-09T07:04:09Z

So you are saying all the tests are essentially outputting the same error messages? If so, then this looks good to me.

mununki · 2023-05-09T16:31:09Z

So you are saying all the tests are essentially outputting the same error messages? If so, then this looks good to me.

Yes.

mununki · 2023-05-09T17:39:51Z

There's nothing more to do with this PR, shall we merge?

ryyppy

LGTM. If @cknitt or @cristianoc have no objections, let's merge.

cknitt · 2023-05-10T08:43:44Z

Fine with me, I approved already. 👍 Thanks a lot for your work @mununki!

mununki · 2023-05-10T08:57:23Z

I hope @cristianoc doesn't have any objections, and if he does, I'll fix it in a subsequent PR.

clean up super errors in syntax

e5d1738

mununki force-pushed the clean-up-syntax-super-error branch from 1bb09f1 to e5d1738 Compare May 7, 2023 17:52

cknitt approved these changes May 7, 2023

View reviewed changes

mununki commented May 7, 2023

View reviewed changes

changelog

25eaf61

Merge branch 'master' into clean-up-syntax-super-error

04bfb11

ryyppy approved these changes May 10, 2023

View reviewed changes

mununki merged commit 065defe into master May 10, 2023

mununki deleted the clean-up-syntax-super-error branch May 10, 2023 08:57

fhammerschmidt mentioned this pull request Oct 2, 2023

Document v11 features. rescript-lang/rescript-lang.org#697

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up super errors in syntax #6246

Clean up super errors in syntax #6246

mununki commented May 6, 2023 •

edited

Loading

cristianoc commented May 7, 2023

cristianoc commented May 7, 2023

mununki commented May 7, 2023

cristianoc commented May 7, 2023 •

edited

Loading

cknitt commented May 7, 2023 •

edited

Loading

mununki commented May 7, 2023

cknitt commented May 7, 2023 •

edited

Loading

mununki commented May 7, 2023

cknitt commented May 7, 2023

mununki commented May 7, 2023

cknitt left a comment

mununki May 7, 2023 •

edited

Loading

ryyppy May 8, 2023

cknitt May 8, 2023

cristianoc May 8, 2023

cristianoc May 8, 2023

ryyppy May 8, 2023

mununki May 8, 2023

mununki May 8, 2023

mununki May 8, 2023 •

edited

Loading

ryyppy May 8, 2023

ryyppy commented May 8, 2023

mununki commented May 9, 2023

ryyppy commented May 9, 2023

mununki commented May 9, 2023

mununki commented May 9, 2023

ryyppy left a comment •

edited

Loading

cknitt commented May 10, 2023

mununki commented May 10, 2023

Clean up super errors in syntax #6246

Clean up super errors in syntax #6246

Conversation

mununki commented May 6, 2023 • edited Loading

cristianoc commented May 7, 2023

cristianoc commented May 7, 2023

mununki commented May 7, 2023

cristianoc commented May 7, 2023 • edited Loading

cknitt commented May 7, 2023 • edited Loading

mununki commented May 7, 2023

cknitt commented May 7, 2023 • edited Loading

mununki commented May 7, 2023

cknitt commented May 7, 2023

mununki commented May 7, 2023

cknitt left a comment

Choose a reason for hiding this comment

mununki May 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mununki May 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryyppy commented May 8, 2023

mununki commented May 9, 2023

ryyppy commented May 9, 2023

mununki commented May 9, 2023

mununki commented May 9, 2023

ryyppy left a comment • edited Loading

Choose a reason for hiding this comment

cknitt commented May 10, 2023

mununki commented May 10, 2023

mununki commented May 6, 2023 •

edited

Loading

cristianoc commented May 7, 2023 •

edited

Loading

cknitt commented May 7, 2023 •

edited

Loading

cknitt commented May 7, 2023 •

edited

Loading

mununki May 7, 2023 •

edited

Loading

mununki May 8, 2023 •

edited

Loading

ryyppy left a comment •

edited

Loading