From 5c867a3ab0e2348e76389860ed10ad92887633cb Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Wed, 3 Apr 2024 21:23:17 +0200 Subject: [PATCH 1/8] PoC for adding a %todo extension that warns with proper locations --- .../expected/todo_with_no_payload.res.expected | 11 +++++++++++ .../expected/todo_with_payload.res.expected | 12 ++++++++++++ .../super_errors/fixtures/todo_with_no_payload.res | 5 +++++ .../super_errors/fixtures/todo_with_payload.res | 5 +++++ jscomp/ext/warnings.ml | 7 +++++++ jscomp/ext/warnings.mli | 1 + jscomp/frontend/ast_exp_extension.ml | 9 +++++++++ 7 files changed, 50 insertions(+) create mode 100644 jscomp/build_tests/super_errors/expected/todo_with_no_payload.res.expected create mode 100644 jscomp/build_tests/super_errors/expected/todo_with_payload.res.expected create mode 100644 jscomp/build_tests/super_errors/fixtures/todo_with_no_payload.res create mode 100644 jscomp/build_tests/super_errors/fixtures/todo_with_payload.res diff --git a/jscomp/build_tests/super_errors/expected/todo_with_no_payload.res.expected b/jscomp/build_tests/super_errors/expected/todo_with_no_payload.res.expected new file mode 100644 index 0000000000..bd440792d8 --- /dev/null +++ b/jscomp/build_tests/super_errors/expected/todo_with_no_payload.res.expected @@ -0,0 +1,11 @@ + + Warning number 110 + /.../fixtures/todo_with_no_payload.res:1:38-42 + + 1 │ let implementMeLater = (): string => %todo + 2 │ + 3 │ let x = implementMeLater() + + Todo found. + +This code will crash if it's run. Be sure to finish it before running your program. \ No newline at end of file diff --git a/jscomp/build_tests/super_errors/expected/todo_with_payload.res.expected b/jscomp/build_tests/super_errors/expected/todo_with_payload.res.expected new file mode 100644 index 0000000000..428a6afb59 --- /dev/null +++ b/jscomp/build_tests/super_errors/expected/todo_with_payload.res.expected @@ -0,0 +1,12 @@ + + Warning number 110 + /.../fixtures/todo_with_payload.res:1:38-85 + + 1 │ let implementMeLater = (): string => %todo("This should return a string  + │ eventually.") + 2 │ + 3 │ let x = implementMeLater() + + Todo found: This should return a string eventually. + +This code will crash if it's run. Be sure to finish it before running your program. \ No newline at end of file diff --git a/jscomp/build_tests/super_errors/fixtures/todo_with_no_payload.res b/jscomp/build_tests/super_errors/fixtures/todo_with_no_payload.res new file mode 100644 index 0000000000..d7aa60688c --- /dev/null +++ b/jscomp/build_tests/super_errors/fixtures/todo_with_no_payload.res @@ -0,0 +1,5 @@ +let implementMeLater = (): string => %todo + +let x = implementMeLater() + +Js.log(x->Js.String2.includes("x")) diff --git a/jscomp/build_tests/super_errors/fixtures/todo_with_payload.res b/jscomp/build_tests/super_errors/fixtures/todo_with_payload.res new file mode 100644 index 0000000000..a52d80d5c1 --- /dev/null +++ b/jscomp/build_tests/super_errors/fixtures/todo_with_payload.res @@ -0,0 +1,5 @@ +let implementMeLater = (): string => %todo("This should return a string eventually.") + +let x = implementMeLater() + +Js.log(x->Js.String2.includes("x")) diff --git a/jscomp/ext/warnings.ml b/jscomp/ext/warnings.ml index a839f711f5..15170e34a8 100644 --- a/jscomp/ext/warnings.ml +++ b/jscomp/ext/warnings.ml @@ -86,6 +86,7 @@ type t = | Bs_integer_literal_overflow (* 107 *) | Bs_uninterpreted_delimiters of string (* 108 *) | Bs_toplevel_expression_unit of (string * topLevelUnitHelp) option (* 109 *) + | Bs_todo of string option (* 110 *) (* If you remove a warning, leave a hole in the numbering. NEVER change the numbers of existing warnings. @@ -151,6 +152,7 @@ let number = function | Bs_integer_literal_overflow -> 107 | Bs_uninterpreted_delimiters _ -> 108 | Bs_toplevel_expression_unit _ -> 109 + | Bs_todo _ -> 110 let last_warning_number = 110 @@ -509,6 +511,11 @@ let message = function | Other -> "yourExpression") in Printf.sprintf "\n\n Possible solutions:\n - Assigning to a value that is then ignored: `let _ = %s`\n - Piping into the built-in ignore function to ignore the result: `%s->ignore`" helpText helpText | _ -> "") + | Bs_todo maybe_text -> ( + match maybe_text with + | None -> "Todo found." + | Some todo -> "Todo found: " ^ todo + ) ^ "\n\nThis code will crash if it's run. Be sure to finish it before running your program." let sub_locs = function | Deprecated (_, def, use) -> diff --git a/jscomp/ext/warnings.mli b/jscomp/ext/warnings.mli index d5d8445c20..b5dab78bf0 100644 --- a/jscomp/ext/warnings.mli +++ b/jscomp/ext/warnings.mli @@ -79,6 +79,7 @@ type t = | Bs_integer_literal_overflow (* 107 *) | Bs_uninterpreted_delimiters of string (* 108 *) | Bs_toplevel_expression_unit of (string * topLevelUnitHelp) option (* 109 *) + | Bs_todo of string option (* 110 *) val parse_options : bool -> string -> unit diff --git a/jscomp/frontend/ast_exp_extension.ml b/jscomp/frontend/ast_exp_extension.ml index 0b1e032c6e..542e50dadb 100644 --- a/jscomp/frontend/ast_exp_extension.ml +++ b/jscomp/frontend/ast_exp_extension.ml @@ -26,6 +26,15 @@ open Ast_helper let handle_extension e (self : Bs_ast_mapper.mapper) (({txt; loc}, payload) : Parsetree.extension) = match txt with + | "todo" -> + Location.prerr_warning e.Parsetree.pexp_loc + (Bs_todo + (match Ast_payload.is_single_string payload with + | Some (s, _) -> Some s + | None -> None)); + Exp.apply ~loc + (Exp.ident ~loc {txt = Longident.parse "Obj.magic"; loc}) + [(Nolabel, Exp.construct ~loc {txt = Longident.Lident "()"; loc} None)] | "ffi" -> Ast_exp_handle_external.handle_ffi ~loc ~payload | "bs.raw" | "raw" -> Ast_exp_handle_external.handle_raw ~kind:Raw_exp loc payload From 47e59ac799291e0b4d68a24ac39b28251895c574 Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Wed, 3 Apr 2024 21:52:39 +0200 Subject: [PATCH 2/8] raise instead of emulating Obj.magic --- jscomp/frontend/ast_exp_extension.ml | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/jscomp/frontend/ast_exp_extension.ml b/jscomp/frontend/ast_exp_extension.ml index 542e50dadb..d9e2dd9042 100644 --- a/jscomp/frontend/ast_exp_extension.ml +++ b/jscomp/frontend/ast_exp_extension.ml @@ -27,14 +27,23 @@ let handle_extension e (self : Bs_ast_mapper.mapper) (({txt; loc}, payload) : Parsetree.extension) = match txt with | "todo" -> - Location.prerr_warning e.Parsetree.pexp_loc - (Bs_todo - (match Ast_payload.is_single_string payload with - | Some (s, _) -> Some s - | None -> None)); + let todo_message = + match Ast_payload.is_single_string payload with + | Some (s, _) -> Some s + | None -> None + in + Location.prerr_warning e.Parsetree.pexp_loc (Bs_todo todo_message); Exp.apply ~loc - (Exp.ident ~loc {txt = Longident.parse "Obj.magic"; loc}) - [(Nolabel, Exp.construct ~loc {txt = Longident.Lident "()"; loc} None)] + (Exp.ident ~loc {txt = Longident.parse "failwith"; loc}) + [ + ( Nolabel, + Exp.constant ~loc + (Pconst_string + ( (match todo_message with + | None -> "todo" + | Some msg -> msg), + None )) ); + ] | "ffi" -> Ast_exp_handle_external.handle_ffi ~loc ~payload | "bs.raw" | "raw" -> Ast_exp_handle_external.handle_raw ~kind:Raw_exp loc payload From 34bee17d6fb25d99f97dadcde6e4a2da68781b0d Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Thu, 4 Apr 2024 09:04:32 +0200 Subject: [PATCH 3/8] align --- jscomp/ext/warnings.ml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jscomp/ext/warnings.ml b/jscomp/ext/warnings.ml index 15170e34a8..85b43c19af 100644 --- a/jscomp/ext/warnings.ml +++ b/jscomp/ext/warnings.ml @@ -515,7 +515,7 @@ let message = function match maybe_text with | None -> "Todo found." | Some todo -> "Todo found: " ^ todo - ) ^ "\n\nThis code will crash if it's run. Be sure to finish it before running your program." + ) ^ "\n\n This code will crash if it's run. Be sure to finish it before running your program." let sub_locs = function | Deprecated (_, def, use) -> From 08fd41dcf726be33fa5879d172077d3f96a2279b Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Thu, 4 Apr 2024 09:05:20 +0200 Subject: [PATCH 4/8] use Js.Exn.raiseError and include ReScript file name + location of todo code in error thrown at runtime --- jscomp/frontend/ast_exp_extension.ml | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/jscomp/frontend/ast_exp_extension.ml b/jscomp/frontend/ast_exp_extension.ml index d9e2dd9042..2d9054a0c4 100644 --- a/jscomp/frontend/ast_exp_extension.ml +++ b/jscomp/frontend/ast_exp_extension.ml @@ -33,15 +33,26 @@ let handle_extension e (self : Bs_ast_mapper.mapper) | None -> None in Location.prerr_warning e.Parsetree.pexp_loc (Bs_todo todo_message); + let pretext = + loc.loc_start.pos_fname ^ ":" + ^ string_of_int loc.loc_start.pos_lnum + ^ ":" + ^ string_of_int loc.loc_start.pos_cnum + ^ "-" + ^ string_of_int loc.loc_end.pos_cnum + in + Exp.apply ~loc - (Exp.ident ~loc {txt = Longident.parse "failwith"; loc}) + (Exp.ident ~loc {txt = Longident.parse "Js.Exn.raiseError"; loc}) [ ( Nolabel, Exp.constant ~loc (Pconst_string - ( (match todo_message with - | None -> "todo" - | Some msg -> msg), + ( (pretext + ^ + match todo_message with + | None -> " - Todo" + | Some msg -> " - Todo: " ^ msg), None )) ); ] | "ffi" -> Ast_exp_handle_external.handle_ffi ~loc ~payload From 47b761c5edf24a85ac864d0346b3159e218de56b Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Thu, 4 Apr 2024 09:13:17 +0200 Subject: [PATCH 5/8] update fixtures --- .../super_errors/expected/todo_with_no_payload.res.expected | 2 +- .../super_errors/expected/todo_with_payload.res.expected | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/jscomp/build_tests/super_errors/expected/todo_with_no_payload.res.expected b/jscomp/build_tests/super_errors/expected/todo_with_no_payload.res.expected index bd440792d8..86d75bc0ed 100644 --- a/jscomp/build_tests/super_errors/expected/todo_with_no_payload.res.expected +++ b/jscomp/build_tests/super_errors/expected/todo_with_no_payload.res.expected @@ -8,4 +8,4 @@ Todo found. -This code will crash if it's run. Be sure to finish it before running your program. \ No newline at end of file + This code will crash if it's run. Be sure to finish it before running your program. \ No newline at end of file diff --git a/jscomp/build_tests/super_errors/expected/todo_with_payload.res.expected b/jscomp/build_tests/super_errors/expected/todo_with_payload.res.expected index 428a6afb59..2b2ff0e9f0 100644 --- a/jscomp/build_tests/super_errors/expected/todo_with_payload.res.expected +++ b/jscomp/build_tests/super_errors/expected/todo_with_payload.res.expected @@ -9,4 +9,4 @@ Todo found: This should return a string eventually. -This code will crash if it's run. Be sure to finish it before running your program. \ No newline at end of file + This code will crash if it's run. Be sure to finish it before running your program. \ No newline at end of file From feca554bc8b2d1facb415416b26efabd3383794b Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Thu, 4 Apr 2024 11:26:06 +0200 Subject: [PATCH 6/8] change text --- jscomp/ext/warnings.ml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jscomp/ext/warnings.ml b/jscomp/ext/warnings.ml index 85b43c19af..9229eb5db1 100644 --- a/jscomp/ext/warnings.ml +++ b/jscomp/ext/warnings.ml @@ -515,7 +515,7 @@ let message = function match maybe_text with | None -> "Todo found." | Some todo -> "Todo found: " ^ todo - ) ^ "\n\n This code will crash if it's run. Be sure to finish it before running your program." + ) ^ "\n\n This code is not implemented yet and will crash at runtime. Make sure you implement this before running the code." let sub_locs = function | Deprecated (_, def, use) -> From 290fb9faa935f462f739adb5698cf7ae1962f0d7 Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Thu, 4 Apr 2024 11:32:55 +0200 Subject: [PATCH 7/8] fixtures --- .../super_errors/expected/todo_with_no_payload.res.expected | 2 +- .../super_errors/expected/todo_with_payload.res.expected | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/jscomp/build_tests/super_errors/expected/todo_with_no_payload.res.expected b/jscomp/build_tests/super_errors/expected/todo_with_no_payload.res.expected index 86d75bc0ed..3037b8845c 100644 --- a/jscomp/build_tests/super_errors/expected/todo_with_no_payload.res.expected +++ b/jscomp/build_tests/super_errors/expected/todo_with_no_payload.res.expected @@ -8,4 +8,4 @@ Todo found. - This code will crash if it's run. Be sure to finish it before running your program. \ No newline at end of file + This code is not implemented yet and will crash at runtime. Make sure you implement this before running the code. \ No newline at end of file diff --git a/jscomp/build_tests/super_errors/expected/todo_with_payload.res.expected b/jscomp/build_tests/super_errors/expected/todo_with_payload.res.expected index 2b2ff0e9f0..37335aee1a 100644 --- a/jscomp/build_tests/super_errors/expected/todo_with_payload.res.expected +++ b/jscomp/build_tests/super_errors/expected/todo_with_payload.res.expected @@ -9,4 +9,4 @@ Todo found: This should return a string eventually. - This code will crash if it's run. Be sure to finish it before running your program. \ No newline at end of file + This code is not implemented yet and will crash at runtime. Make sure you implement this before running the code. \ No newline at end of file From c4a1d77ac45709d6a3604a059599877ad0f59c05 Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Thu, 4 Apr 2024 11:34:12 +0200 Subject: [PATCH 8/8] changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index aaf913f35a..b0aef3bd19 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,10 @@ # 11.1.0-rc.8 (Unreleased) +#### :rocket: New Feature + +- Add `%todo` extension for leaving implementation for later. https://github.com/rescript-lang/rescript-compiler/pull/6713 + #### :bug: Bug Fix - Improve error when using '@deriving(accessors)' on a variant with record arguments. https://github.com/rescript-lang/rescript-compiler/pull/6712