Skip to content

Commit 80f774d

Browse files
chenglouchenglou
authored
[Super errors] Custom msgs for ReasonReact pitfalls (#1928)
This fixes the 3 most common RR problems
1 parent e3458b2 commit 80f774d

File tree

8 files changed

+184
-11
lines changed

8 files changed

+184
-11
lines changed

‎jscomp/Makefile

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -347,7 +347,7 @@ BSB_SRCS= bsb_config\
347347
oCamlRes\
348348
bsb_templates\
349349
bsb_init
350-
SUPER_ERRORS_SRCS=super_misc super_warnings super_location super_typecore super_typetexp super_main
350+
SUPER_ERRORS_SRCS=super_reason_react super_misc super_warnings super_location super_typecore super_typemod super_typetexp super_main
351351
REASON_OUTCOME_PRINTER_SRCS=tweaked_reason_oprint reason_outcome_printer_main
352352

353353
BSB_CMXS=$(addprefix bsb/, $(addsuffix .cmx, $(BSB_SRCS)))

‎jscomp/super_errors/README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
Hello! This is the subdirectory for the new, newcomer-friendly OCaml/Reason warning & error report system. Most of the logic are lifted from the compiler (https://github.com/BuckleScript/ocaml/tree/master). The convention here is to have a `super_foo` for each corresponding compiler's file `foo`. So, for example, `warnings.ml` becomes `super_warnings.ml`. The exception is `super_main`, the entry point.
1+
Hello! This is the subdirectory for the new, newcomer-friendly OCaml/Reason warning & error report system. Most of the logic are lifted from the compiler (https://github.com/BuckleScript/ocaml/tree/master). The convention here is to have a `super_foo` for each corresponding compiler's file `foo`. So, for example, `warnings.ml` becomes `super_warnings.ml`. The exception is `super_main`, the entry point, and `super_reason_react`, our special handling of [ReasonReact](https://reasonml.github.io/reason-react/) errors.
22

33
Feel free to submit new ones or tweak existing messages in these files! They also have more precise comments in them that tells you how they work.
44

‎jscomp/super_errors/super_location.ml

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ let file_lines filePath =
2323
[||]
2424
with
2525
| End_of_file -> begin
26-
close_in chan;
26+
close_in chan;
2727
List.rev (!lines) |> Array.of_list
2828
end
2929

@@ -51,7 +51,7 @@ let print ~is_warning intro ppf loc =
5151
if loc.loc_start.pos_fname = "//toplevel//"
5252
&& highlight_locations ppf [loc] then ()
5353
else
54-
if is_warning then
54+
if is_warning then
5555
fprintf ppf "@[@{<info>%s@}@]@," intro
5656
else begin
5757
fprintf ppf "@[@{<error>%s@}@]@," intro
@@ -62,9 +62,9 @@ let print ~is_warning intro ppf loc =
6262
(* things to special-case: startchar & endchar2 both -1 *)
6363
if start_char == -1 || end_char == -1 then
6464
(* happens sometimes. Syntax error for example *)
65-
fprintf ppf "Is there an error before this one? If so, it's likely a syntax error. The more relevant message should be just above!@ If it's not, please file an issue here:@ github.com/facebook/reason/issues@,"
65+
fprintf ppf "Is there an error before this one? If so, it's likely a syntax error.@ The more relevant message should be just above!@ If it's not, please file an issue here:@ github.com/facebook/reason/issues@,"
6666
else begin
67-
try
67+
try
6868
let lines = file_lines file in
6969
fprintf ppf "%a"
7070
(Super_misc.print_file
@@ -109,15 +109,15 @@ let super_warning_printer loc ppf w =
109109
if Warnings.is_active w then begin
110110
setup_colors ();
111111
(* open a vertical box. Everything in our message is indented 2 spaces *)
112-
Format.fprintf ppf "@[<v 2>@,%a@,%a@,@]"
113-
(print ~is_warning:true ("Warning number " ^ (Super_warnings.number w |> string_of_int)))
114-
loc
115-
Super_warnings.print
112+
Format.fprintf ppf "@[<v 2>@,%a@,%a@,@]"
113+
(print ~is_warning:true ("Warning number " ^ (Super_warnings.number w |> string_of_int)))
114+
loc
115+
Super_warnings.print
116116
w
117117
end
118118
;;
119119

120-
(* taken from https://github.com/BuckleScript/ocaml/blob/d4144647d1bf9bc7dc3aadc24c25a7efa3a67915/parsing/location.ml#L354 *)
120+
(* taken from https://github.com/BuckleScript/ocaml/blob/d4144647d1bf9bc7dc3aadc24c25a7efa3a67915/parsing/location.ml#L354 *)
121121
let print_phanton_error_prefix ppf =
122122
(* modified from the original. We use only 2 indentations for error report
123123
(see super_error_reporter above) *)

‎jscomp/super_errors/super_main.ml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,5 @@
22
let setup () =
33
Super_location.setup ();
44
Super_typetexp.setup ();
5+
Super_typemod.setup ();
56
Super_typecore.setup ();

‎jscomp/super_errors/super_reason_react.ml

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
(* This file detects common error from
2+
[ReasonReact](https://reasonml.github.io/reason-react/) and provide
3+
situation-specific hints. See the mli file to see which heurisics we detect
4+
and related comments *)
5+
open Types
6+
7+
let deconstruct_component_type t =
8+
match t.desc with
9+
| Tconstr (p, types, _) when Path.last p = "componentSpec" -> Some types
10+
| _ -> None
11+
12+
let type_is_component_spec t =
13+
deconstruct_component_type t <> None
14+
15+
(* recursively drill down the types (first item is the type alias, if any. Second is the content of the alias) *)
16+
let rec get_to_bottom_of_aliases f = function
17+
| (_alias1, type1) :: (_alias2, type2) :: rest ->
18+
begin match get_to_bottom_of_aliases f rest with
19+
| false -> f (type1, type2)
20+
| true -> true
21+
end
22+
| _ -> false
23+
24+
let state_escape_scope = get_to_bottom_of_aliases (function
25+
(* https://github.com/BuckleScript/ocaml/blob/ddf5a739cc0978dab5e553443825791ba7b0cef9/typing/printtyp.ml?utf8=✓#L1348 *)
26+
(* so apparently that's the logic for detecting "the constructor out of scope" error *)
27+
| ({desc = Tconstr (p, _, _)}, {desc = Tvar _; level})
28+
when level < Path.binding_time p -> true
29+
| _ -> false
30+
)
31+
32+
let is_array_wanted_reactElement = get_to_bottom_of_aliases (function
33+
| ({desc = Tconstr (path1, _, _)}, {desc = Tconstr (path2, _, _)})
34+
when Path.last path1 = "array" && Path.last path2 = "reactElement" -> true
35+
| _ -> false
36+
)

‎jscomp/super_errors/super_reason_react.mli

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
val type_is_component_spec: Types.type_expr -> bool
2+
(** Used by super_typemod when we detect the message "... contains type variables that cannot be generalized" *)
3+
4+
val state_escape_scope: (Types.type_expr * Types.type_expr) list -> bool
5+
(** Used by super_typecore when we detect the message "The type constructor state would escape its scope" *)
6+
7+
val is_array_wanted_reactElement: (Types.type_expr * Types.type_expr) list -> bool
8+
(** Used by super_typecore when we detect the message "The type constructor state would escape its scope" *)

‎jscomp/super_errors/super_typecore.ml

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,18 @@ let report_error env ppf = function
4646
(Ident.name id)
4747
| Expr_type_clash trace ->
4848
(* modified *)
49+
if Super_reason_react.state_escape_scope trace then
50+
fprintf ppf "@[<v>\
51+
@[@{<info>Is this a ReasonReact component with state?@}@ If so, is the state type declared _after_ the component declaration?@ \
52+
Moving the state type before the declaration should resolved this!@]@,@,\
53+
@[@{<info>Here's the original error message@}@]@,\
54+
@]"
55+
else if Super_reason_react.is_array_wanted_reactElement trace then
56+
fprintf ppf "@[<v>\
57+
@[@{<info>Are you passing an array as a ReasonReact DOM (lower-case) component's children?@}@ If not, disregard this.@ \
58+
If so, please use `ReasonReact.createDomElement`:@ https://reasonml.github.io/reason-react/index.html#reason-react-working-with-children@]@,@,\
59+
@[@{<info>Here's the original error message@}@]@,\
60+
@]";
4961
report_unification_error ppf env trace
5062
(function ppf ->
5163
fprintf ppf "This is:")

‎jscomp/super_errors/super_typemod.ml

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
open Printtyp
2+
3+
let fprintf = Format.fprintf
4+
5+
(* taken from https://github.com/BuckleScript/ocaml/blob/d4144647d1bf9bc7dc3aadc24c25a7efa3a67915/typing/typemod.ml#L1754 *)
6+
(* modified branches are commented *)
7+
let report_error ppf = Typemod.(function
8+
Cannot_apply mty ->
9+
fprintf ppf
10+
"@[This module is not a functor; it has type@ %a@]" modtype mty
11+
| Not_included errs ->
12+
fprintf ppf
13+
"@[<v>Signature mismatch:@ %a@]" Includemod.report_error errs
14+
| Cannot_eliminate_dependency mty ->
15+
fprintf ppf
16+
"@[This functor has type@ %a@ \
17+
The parameter cannot be eliminated in the result type.@ \
18+
Please bind the argument to a module identifier.@]" modtype mty
19+
| Signature_expected -> fprintf ppf "This module type is not a signature"
20+
| Structure_expected mty ->
21+
fprintf ppf
22+
"@[This module is not a structure; it has type@ %a" modtype mty
23+
| With_no_component lid ->
24+
fprintf ppf
25+
"@[The signature constrained by `with' has no component named %a@]"
26+
longident lid
27+
| With_mismatch(lid, explanation) ->
28+
fprintf ppf
29+
"@[<v>\
30+
@[In this `with' constraint, the new definition of %a@ \
31+
does not match its original definition@ \
32+
in the constrained signature:@]@ \
33+
%a@]"
34+
longident lid Includemod.report_error explanation
35+
| Repeated_name(kind, name) ->
36+
fprintf ppf
37+
"@[Multiple definition of the %s name %s.@ \
38+
Names must be unique in a given structure or signature.@]" kind name
39+
| Non_generalizable typ ->
40+
(* modified *)
41+
fprintf ppf "@[";
42+
if Super_reason_react.type_is_component_spec typ then begin
43+
fprintf ppf "@[<v>\
44+
@[@{<info>Is this a ReasonReact component with state or retained props?@}@ If so, this error will disappear after:@]@,\
45+
@[- Defining the component's `make` function@]@,\
46+
@[- Using the state once or annotating it with a type where it's used (e.g. render)@]\
47+
@[- Do the same for retained props, if any@]@,@,\
48+
@[@{<info>Here's the original error message@}@]\
49+
@]@,"
50+
end;
51+
fprintf ppf
52+
"@[<v>@[This expression's type contains type variables that can't be generalized:@,@{<error>%a@}@]@,@,\
53+
@[This happens when the type system senses there's a mutation/side-effect, in combination with a polymorphic value.@,\
54+
Using or annotating that value usually solves it.@ \
55+
More info:@ https://realworldocaml.org/v1/en/html/imperative-programming-1.html#side-effects-and-weak-polymorphism@]@]"
56+
type_scheme
57+
typ;
58+
fprintf ppf "@]"
59+
| Non_generalizable_class (id, desc) ->
60+
fprintf ppf
61+
"@[The type of this class,@ %a,@ \
62+
contains type variables that cannot be generalized@]"
63+
(class_declaration id) desc
64+
| Non_generalizable_module mty ->
65+
fprintf ppf
66+
"@[The type of this module,@ %a,@ \
67+
contains type variables that cannot be generalized@]" modtype mty;
68+
| Implementation_is_required intf_name ->
69+
fprintf ppf
70+
"@[The interface %a@ declares values, not just types.@ \
71+
An implementation must be provided.@]"
72+
Location.print_filename intf_name
73+
| Interface_not_compiled intf_name ->
74+
fprintf ppf
75+
"@[Could not find the .cmi file for interface@ %a.@]"
76+
Location.print_filename intf_name
77+
| Not_allowed_in_functor_body ->
78+
fprintf ppf
79+
"@[This expression creates fresh types.@ %s@]"
80+
"It is not allowed inside applicative functors."
81+
| With_need_typeconstr ->
82+
fprintf ppf
83+
"Only type constructors with identical parameters can be substituted."
84+
| Not_a_packed_module ty ->
85+
fprintf ppf
86+
"This expression is not a packed module. It has type@ %a"
87+
type_expr ty
88+
| Incomplete_packed_module ty ->
89+
fprintf ppf
90+
"The type of this packed module contains variables:@ %a"
91+
type_expr ty
92+
| Scoping_pack (lid, ty) ->
93+
fprintf ppf
94+
"The type %a in this module cannot be exported.@ " longident lid;
95+
fprintf ppf
96+
"Its type contains local dependencies:@ %a" type_expr ty
97+
| Recursive_module_require_explicit_type ->
98+
fprintf ppf "Recursive modules require an explicit module type."
99+
| Apply_generative ->
100+
fprintf ppf "This is a generative functor. It can only be applied to ()"
101+
)
102+
103+
let report_error env ppf err =
104+
Printtyp.wrap_printing_env env (fun () -> report_error ppf err)
105+
106+
(* This will be called in super_main. This is how you'd override the default error printer from the compiler & register new error_of_exn handlers *)
107+
let setup () =
108+
Location.register_error_of_exn
109+
(function
110+
| Typemod.Error (loc, env, err) ->
111+
Some (Super_location.error_of_printer loc (report_error env) err)
112+
| Typemod.Error_forward err ->
113+
Some err
114+
| _ ->
115+
None
116+
)

0 commit comments

Comments
 (0)