Clean up Super_error (#6199)

* add tests

* error message with loc of repeated definition

* changelog

* clean up

* clean up super_error

* use Location.print_loc

* move some tests from ounit to super_errors

* changelog

* remove unused ounit tests

* changelog

* changelog

Author: mununki

CHANGELOG.md

@@ -24,6 +24,7 @@
 #### :boom: Breaking Change
 
 - Parse `assert` as a regular function. `assert` is no longer a unary expression. Example: before `assert 1 == 2` is parsed as `(assert 1) == 2`, now it is parsed as `assert(1 == 2)`. https://github.com/rescript-lang/rescript-compiler/pull/6180
+- `-bs-super-errors` flag has been removed along with Super_errors. https://github.com/rescript-lang/rescript-compiler/pull/6199
 
 #### :bug: Bug Fix
 
@@ -32,6 +33,11 @@
 - Add error messages for dangling doc comments/attributes and mutable in record type definition. https://github.com/rescript-lang/rescript-compiler/pull/6206
 - Fix issue with overlapping array and object in untagged variants https://github.com/rescript-lang/rescript-compiler/pull/6219
 
+#### :nail_care: Polish
+
+- Add location information to duplicate type definition error messages. https://github.com/rescript-lang/rescript-compiler/pull/6199
+- Replace normal module errors with Super_error module, and clean up Super_error. https://github.com/rescript-lang/rescript-compiler/pull/6199
+
 # 11.0.0-alpha.4
 
 #### :rocket: Main New Feature

jscomp/bsc/dune

@@ -8,5 +8,4 @@
  (public_name bsc)
  (flags
   (:standard -w +a-4-9-30-40-41-42-48-70))
- (libraries common core depends gentype js_parser syntax super_errors
-   outcome_printer))
+ (libraries common core depends gentype js_parser syntax outcome_printer))

jscomp/bsc/rescript_compiler_main.ml

@@ -25,8 +25,7 @@ let setup_compiler_printer (syntax_kind : [ syntax_kind | `default])=
    | `default -> ()  
    | #syntax_kind as k -> Config.syntax_kind := k);   
   let syntax_kind = !Config.syntax_kind in
-  if syntax_kind = `rescript then begin 
-    Lazy.force Super_main.setup;  
+  if syntax_kind = `rescript then begin
     Lazy.force Res_outcome_printer.setup  
   end  
 
@@ -206,7 +205,6 @@ let print_version_string () =
 
 let [@inline] set s : Bsc_args.spec = Unit (Unit_set s)
 let [@inline] clear s : Bsc_args.spec = Unit (Unit_clear s)
-let [@inline] unit_lazy s : Bsc_args.spec = Unit(Unit_lazy s)
 let [@inline] string_call s : Bsc_args.spec = 
   String (String_call s)
 let [@inline] string_optional_set s : Bsc_args.spec = 
@@ -294,10 +292,6 @@ let buckle_script_flags : (string * Bsc_args.spec * string) array =
 
     (******************************************************************************)
 
-
-    "-bs-super-errors", unit_lazy Super_main.setup,
-    "*internal* Better error message combined with other tools ";
-
     "-unboxed-types", set Clflags.unboxed_types,
     "*internal* Unannotated unboxable types will be unboxed";
 

jscomp/build_tests/super_errors/expected/repeated_def_extension_constr.res.expected

@@ -0,0 +1,12 @@
+
+  We've found a bug for you!
+  /.../fixtures/repeated_def_extension_constr.res:3:6
+
+  1 │ type a = ..
+  2 │ 
+  3 │ type a
+  4 │ 
+
+  Multiple definition of the type name a 
+  at /.../fixtures/repeated_def_extension_constr.res:1:6
+   Names must be unique in a given structure or signature.

jscomp/build_tests/super_errors/expected/repeated_def_module_types.res.expected

@@ -0,0 +1,12 @@
+
+  We've found a bug for you!
+  /.../fixtures/repeated_def_module_types.res:3:13
+
+  1 │ module type M = {}
+  2 │ 
+  3 │ module type M = {}
+  4 │ 
+
+  Multiple definition of the module type name M 
+  at /.../fixtures/repeated_def_module_types.res:1:13
+   Names must be unique in a given structure or signature.

jscomp/build_tests/super_errors/expected/repeated_def_modules.res.expected

@@ -0,0 +1,12 @@
+
+  We've found a bug for you!
+  /.../fixtures/repeated_def_modules.res:3:8
+
+  1 │ module M = {}
+  2 │ 
+  3 │ module M = {}
+  4 │ 
+
+  Multiple definition of the module name M 
+  at /.../fixtures/repeated_def_modules.res:1:8
+   Names must be unique in a given structure or signature.

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

@@ -0,0 +1,12 @@
+
+  We've found a bug for you!
+  /.../fixtures/repeated_def_types.res:3:6
+
+  1 │ type a
+  2 │ 
+  3 │ type a
+  4 │ 
+
+  Multiple definition of the type name a 
+  at /.../fixtures/repeated_def_types.res:1:6
+   Names must be unique in a given structure or signature.

jscomp/build_tests/super_errors/expected/type2.res.expected

@@ -0,0 +1,14 @@
+
+  We've found a bug for you!
+  /.../fixtures/type2.res:6:11-13
+
+  4 │ let () = {
+  5 │   push(a, 3)->ignore
+  6 │   push(a, "3")->ignore
+  7 │ }
+  8 │ 
+
+  This has type: string
+  Somewhere wanted: int
+  
+  You can convert string to int with Belt.Int.fromString.

jscomp/build_tests/super_errors/expected/type3.res.expected

@@ -0,0 +1,13 @@
+
+  We've found a bug for you!
+  /.../fixtures/type3.res:1:5
+
+  1 │ let u = []
+  2 │ 
+
+  This expression's type contains type variables that cannot be generalized:
+  array<'_weak1>
+  
+  This happens when the type system senses there's a mutation/side-effect,
+  in combination with a polymorphic value.
+  Using or annotating that value usually solves it.

jscomp/build_tests/super_errors/fixtures/repeated_def_extension_constr.res

@@ -0,0 +1,3 @@
+type a = ..
+
+type a

jscomp/build_tests/super_errors/fixtures/repeated_def_module_types.res

@@ -0,0 +1,3 @@
+module type M = {}
+
+module type M = {}

jscomp/build_tests/super_errors/fixtures/repeated_def_modules.res

@@ -0,0 +1,3 @@
+module M = {}
+
+module M = {}

jscomp/build_tests/super_errors/fixtures/repeated_def_types.res

@@ -0,0 +1,3 @@
+type a
+
+type a

jscomp/build_tests/super_errors/fixtures/type2.res

@@ -0,0 +1,7 @@
+@send external push: (array<'a>, 'a) => unit = "push"
+
+let a = []
+let () = {
+  push(a, 3)->ignore
+  push(a, "3")->ignore
+}

jscomp/build_tests/super_errors/fixtures/type3.res

@@ -0,0 +1 @@
+let u = []

jscomp/build_tests/super_errors/input.js

@@ -27,7 +27,7 @@ let atLeastOneTaskFailed = false
 
 fixtures.forEach(fileName => {
   const fullFilePath = path.join(__dirname, 'fixtures', fileName)
-  const command = `${prefix} -color always -bs-super-errors ${fullFilePath}`
+  const command = `${prefix} -color always ${fullFilePath}`
   console.log(`running ${command}`)
   child_process.exec(command, (err, stdout, stderr) => {
     doneTasksCount++

jscomp/gentype_tests/typescript-react-example/bsconfig.json

@@ -15,7 +15,7 @@
     "exportInterfaces": false
   },
   "name": "sample-typescript-app",
-  "bsc-flags": ["-bs-super-errors"],
+  "bsc-flags": [],
   "jsx": { "version": 3 },
   "bs-dependencies": ["@rescript/react"],
   "sources": [

jscomp/super_errors/super_code_frame.ml renamed to jscomp/ml/code_frame.ml

@@ -2276,32 +2276,42 @@ let env_of_only_summary env_from_summary env =
 
 open Format
 
+(* taken from https://github.com/rescript-lang/ocaml/blob/d4144647d1bf9bc7dc3aadc24c25a7efa3a67915/typing/env.ml#L1842 *)
+(* modified branches are commented *)
 let report_error ppf = function
-  | Illegal_renaming(modname, ps_name, filename) -> fprintf ppf
-      "Wrong file naming: %a@ contains the compiled interface for @ \
-       %s when %s was expected"
-      Location.print_filename filename ps_name modname
-  | Inconsistent_import(name, source1, source2) -> fprintf ppf
+  | Illegal_renaming(name, modname, _filename) ->
+    (* modified *)
+    fprintf ppf
+      "@[You referred to the module %s, but we've found one called %s instead.@ \
+       Is the name's casing right?@]"
+      name modname
+  | Inconsistent_import(name, source1, source2) ->
+    (* modified *)
+    fprintf ppf "@[<v>\
+                 @[@{<info>It's possible that your build is stale.@}@ Try to clean the artifacts and build again?@]@,@,\
+                 @[@{<info>Here's the original error message@}@]@,\
+                 @]";
+    fprintf ppf
       "@[<hov>The files %a@ and %a@ \
-              make inconsistent assumptions@ over interface %s@]"
+       make inconsistent assumptions@ over interface %s@]"
       Location.print_filename source1 Location.print_filename source2 name
   | Need_recursive_types(import, export) ->
-      fprintf ppf
-        "@[<hov>Unit %s imports from %s, which uses recursive types.@ %s@]"
-        export import "The compilation flag -rectypes is required"
+    fprintf ppf
+      "@[<hov>Unit %s imports from %s, which uses recursive types.@ %s@]"
+      export import "The compilation flag -rectypes is required"
   | Missing_module(_, path1, path2) ->
-      fprintf ppf "@[@[<hov>";
-      if Path.same path1 path2 then
-        fprintf ppf "Internal path@ %s@ is dangling." (Path.name path1)
-      else
-        fprintf ppf "Internal path@ %s@ expands to@ %s@ which is dangling."
-          (Path.name path1) (Path.name path2);
-      fprintf ppf "@]@ @[%s@ %s@ %s.@]@]"
-        "The compiled interface for module" (Ident.name (Path.head path2))
-        "was not found"
+    fprintf ppf "@[@[<hov>";
+    if Path.same path1 path2 then
+      fprintf ppf "Internal path@ %s@ is dangling." (Path.name path1)
+    else
+      fprintf ppf "Internal path@ %s@ expands to@ %s@ which is dangling."
+        (Path.name path1) (Path.name path2);
+    fprintf ppf "@]@ @[%s@ %s@ %s.@]@]"
+      "The compiled interface for module" (Ident.name (Path.head path2))
+      "was not found"
   | Illegal_value_name(_loc, name) ->
-      fprintf ppf "'%s' is not a valid value identifier."
-        name
+    fprintf ppf "'%s' is not a valid value identifier."
+      name
 
 let () =
   Location.register_error_of_exn

jscomp/ml/env.ml

@@ -254,7 +254,7 @@ let report_error ppf = function
   | Unterminated_string_in_comment (_, loc) ->
       fprintf ppf "This comment contains an unterminated string literal@.\
                    %aString literal begins here"
-              Location.print_error loc
+              (Location.print_error "") loc
   | Keyword_as_label kwd ->
       fprintf ppf "`%s' is a keyword, it cannot be used as label name" kwd
   | Invalid_literal s ->