Clean up Super_error #6199

mununki · 2023-04-25T17:37:11Z

This PR enhances the error message for duplicate type definitions in ReScript by including the location of the previously declared definition. This improvement aims to provide better context for developers, making it easier to identify and resolve type conflicts in their code.

This PR incorporates the Super_error module into the regular modules and cleans up the Super_error module. It also includes adding location information of the already defined declaration in the error message for duplicated type definitions.

In this change, replace the SetString ref data structure with (string, Warnings.loc) Hashtbl.t for better handling of location information.

cristianoc

Just one final thing to check, then ready to go.

cristianoc · 2023-04-25T23:10:44Z

jscomp/ml/typemod.ml

+  | Repeated_name(kind, name, repeated_loc) ->
+      let start_line = repeated_loc.loc_start.pos_lnum in
+      let start_col = repeated_loc.loc_start.pos_cnum - repeated_loc.loc_start.pos_bol in
+      let end_col = repeated_loc.loc_end.pos_cnum - repeated_loc.loc_end.pos_bol in


Is there some logic already to do this? The ocaml and vscode loc info's are different (zero-based vs one-based) so it's a bit tricky and best to keep all in the same place.

Look for the part that says: "we've found a bug for you".

I have added wider task: #6202
This PR fits well in that.

cristianoc · 2023-04-26T19:13:23Z

jscomp/ml/typemod.ml

-      let start_line = repeated_loc.loc_start.pos_lnum in
-      let start_col = repeated_loc.loc_start.pos_cnum - repeated_loc.loc_start.pos_bol in
-      let end_col = repeated_loc.loc_end.pos_cnum - repeated_loc.loc_end.pos_bol in
+      let (_, start_line, start_char) = Location.get_pos_info repeated_loc.loc_start in


Would you check that these correspond to the locations printed normally (whether starting from 0 or 1) for other error messages?

cristianoc

The locations are not quite right I think.

jscomp/build_tests/super_errors/expected/repeated_def_types.res.expected

cristianoc · 2023-04-27T00:20:06Z

jscomp/build_tests/super_errors/expected/repeated_def_types.res.expected

+  [1;31m3[0m [2m│[0m type [1;31ma[0m
+  4 [2m│[0m 
+
+  Multiple definition of the type name a at line 1, characters 5-6.


I think this should say 1:6 for consistency.

I think this is using ocaml locations, from Location..
Instead it should be using super-errors locations. The super-error location treatment is use to output 3:6 above.
I think the printing here should use that location logic.

Now I get it about error handling including super_error.
I'm going to fix this with super_error then will take care of #6202

Looks like this test output has non changed yet?

jscomp/ounit_tests/ounit_cmd_tests.ml

mununki · 2023-04-27T17:00:33Z

jscomp/super_errors/super_typemod.ml

-      ppf
-      (fun ppf () ->
-         fprintf ppf
-           "@[This expression's type contains type variables that can't be generalized:@,@{<error>%a@}@]"


The ounit test seemed to need to pass "can't be generalized," but the test case was written as "cannot be generalized." So, when it was moved to typemod.ml as is, it failed the test, and that's why it was moved to typemod.ml as "cannot be generalized."

When testing "This expression's type contains," it passes, so I think there is no problem.

mununki · 2023-04-27T17:03:24Z

Two of the ounit tests did not pass, and there seems to be a problem with the existing tests or I did not understand them well. So, I rewrote the tests, and I would like to request a review. @cristianoc

cristianoc · 2023-04-27T17:16:56Z

If the output of the tests changed slightly, you can update the expected result of tests. See jscomp/build_tests/super_errors/README.md.

Is this correct, that there was just a small difference in what has been printed? If so, it seems better to just update the expected result instead of changing the tests.

If I misunderstood: what tests were not working and why?

mununki · 2023-04-27T17:39:58Z

If the output of the tests changed slightly, you can update the expected result of tests. See jscomp/build_tests/super_errors/README.md.

Is this correct, that there was just a small difference in what has been printed? If so, it seems better to just update the expected result instead of changing the tests.

If I misunderstood: what tests were not working and why?

Two Ounit_cmd_tests tests cannot pass:

"has type string" -> typecore.ml
I couldn't figure out why "has type string" test was okay so far, the error message should be "has type: string". Another thing I couldn't figure out is that "has type: string" also fails to pass now.
"cannot be generalized" -> typemod.ml
Super_typemod would give us "can't be generalized" for Typemod.Non_generalizable error. But the test was written by "cannot be generalized". No clue why test result was okay so far.

cristianoc · 2023-04-27T17:44:49Z

If the output of the tests changed slightly, you can update the expected result of tests. See jscomp/build_tests/super_errors/README.md.
Is this correct, that there was just a small difference in what has been printed? If so, it seems better to just update the expected result instead of changing the tests.
If I misunderstood: what tests were not working and why?

Two Ounit_cmd_tests tests cannot pass:

"has type string" -> typecore.ml
I couldn't figure out why "has type string" test was okay so far, the error message should be "has type: string". Another thing I couldn't figure out is that "has type: string" also fails to pass now.

"cannot be generalized" -> typemod.ml
Super_typemod would give us "can't be generalized" for Typemod.Non_generalizable error. But the test was written by "cannot be generalized". No clue why test result was okay so far.

OK to move these to super-error tests? So we can check that they're working.

mununki · 2023-04-27T17:45:33Z

Cleaning up the Super_error doesn't make changes of test result in fixtures /jscomp/build_tests/super_errors by node ./jscomp/build_tests/super_errors/input.js update.
Ounit_cmd_tests are the problem we have now.

cristianoc · 2023-04-27T17:47:00Z

Cleaning up the Super_error doesn't make changes of test result in fixtures /jscomp/build_tests/super_errors by node ./jscomp/build_tests/super_errors/input.js update.

Ounit_cmd_tests are the problem we have now.

Yes thanks I have understood now. That's why I'm asking to move those tests out of ounit.

mununki · 2023-04-27T17:55:36Z

Yes thanks I have understood now. That's why I'm asking to move those tests out of ounit.

Got it, done 10ac8c0

cristianoc

This looks great.
Left a tiny comment.
Anything else to do? Looks like this can be merged.

jscomp/ml/lexer.mll

jscomp/ounit_tests/ounit_cmd_tests.ml

cristianoc · 2023-04-27T17:57:08Z

jscomp/ounit_tests/ounit_cmd_tests.ml

@@ -276,14 +276,14 @@ let rec y = A y;;
      OUnit.assert_bool __LOC__
        (Ext_string.contain_substring should_err.stderr "contravariant")
    end;
-    __LOC__ >:: begin fun _ ->


Would you delete instead of commenting out?

cristianoc · 2023-04-27T17:59:47Z

jscomp/bsc/rescript_compiler_main.ml

@@ -294,10 +292,6 @@ let buckle_script_flags : (string * Bsc_args.spec * string) array =

    (******************************************************************************)

-
-    "-bs-super-errors", unit_lazy Super_main.setup,


Perhaps mention this as breaking change in the changelog.
I don't think there's any use for calling bsc directly, but just to be on the safe side.

Isn't there also something in bsconfig? Perhaps one could deprecate or remove that too.

I can add it to the changelog as a breaking change, but how do I deprecate it when I've already removed the super_error? There is a way to make it deprecated?

This is low risk. Remove is OK. No need to deprecate.

jscomp/ml/location.ml

mununki · 2023-04-27T18:16:29Z

I have nothing left for this PR now.

mununki added 4 commits April 26, 2023 02:32

add tests

8db4e2f

error message with loc of repeated definition

4b8c3ee

changelog

4ec6771

clean up

2bd9366

cristianoc reviewed Apr 26, 2023

View reviewed changes

cristianoc reviewed Apr 27, 2023

View reviewed changes

clean up super_error

6a76c51

mununki changed the title ~~Include Location Information for Duplicate Type Definition Errors~~ Clean up Super_error Apr 27, 2023

mununki commented Apr 27, 2023

View reviewed changes

use Location.print_loc

9706da8

move some tests from ounit to super_errors

10ac8c0

cristianoc approved these changes Apr 27, 2023

View reviewed changes

changelog

9d4e302

cristianoc reviewed Apr 27, 2023

View reviewed changes

mununki commented Apr 27, 2023

View reviewed changes

jscomp/ml/location.ml Show resolved Hide resolved

mununki added 3 commits April 28, 2023 03:05

remove unused ounit tests

8d8bf81

changelog

4ecaeb3

changelog

2c24e15

cristianoc approved these changes Apr 27, 2023

View reviewed changes

cristianoc merged commit 63fd342 into master Apr 27, 2023

cristianoc deleted the error-msg-multiple-def branch April 27, 2023 18:17

mununki mentioned this pull request May 4, 2023

Deprecate -bs-super-errors flag for backward compat #6243

Merged

cknitt mentioned this pull request May 7, 2023

Clean up super errors in syntax #6246

Merged

fhammerschmidt mentioned this pull request Oct 2, 2023

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

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up Super_error #6199

Clean up Super_error #6199

mununki commented Apr 25, 2023 •

edited

Loading

cristianoc left a comment

cristianoc Apr 25, 2023

cristianoc Apr 25, 2023

cristianoc Apr 26, 2023

cristianoc Apr 26, 2023

cristianoc left a comment

cristianoc Apr 27, 2023 •

edited

Loading

mununki Apr 27, 2023

cristianoc Apr 27, 2023

mununki Apr 27, 2023

mununki commented Apr 27, 2023

cristianoc commented Apr 27, 2023

mununki commented Apr 27, 2023

cristianoc commented Apr 27, 2023

mununki commented Apr 27, 2023

cristianoc commented Apr 27, 2023

mununki commented Apr 27, 2023

cristianoc left a comment

cristianoc Apr 27, 2023

mununki Apr 27, 2023

cristianoc Apr 27, 2023

mununki Apr 27, 2023

cristianoc Apr 27, 2023

mununki Apr 27, 2023

mununki Apr 27, 2023

mununki commented Apr 27, 2023

		@@ -294,10 +292,6 @@ let buckle_script_flags : (string * Bsc_args.spec * string) array =

		(******************************************************************************)


		"-bs-super-errors", unit_lazy Super_main.setup,

Clean up Super_error #6199

Clean up Super_error #6199

Conversation

mununki commented Apr 25, 2023 • edited Loading

cristianoc left a 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

cristianoc left a comment

Choose a reason for hiding this comment

cristianoc Apr 27, 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

mununki commented Apr 27, 2023

cristianoc commented Apr 27, 2023

mununki commented Apr 27, 2023

cristianoc commented Apr 27, 2023

mununki commented Apr 27, 2023

cristianoc commented Apr 27, 2023

mununki commented Apr 27, 2023

cristianoc left a 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

Choose a reason for hiding this comment

mununki commented Apr 27, 2023

mununki commented Apr 25, 2023 •

edited

Loading

cristianoc Apr 27, 2023 •

edited

Loading