From 1d4c0281421f24d88162da2d38a2b08424c3fa38 Mon Sep 17 00:00:00 2001 From: Christoph Knittel Date: Sun, 1 Nov 2026 08:12:24 +0000 Subject: [PATCH 1/2] Expand %todo to the standard library's JsError, located like __LOC__ Why: %todo called `JsError.throwWithMessage` by name, so a user module named JsError replaced it. Its message also embedded the absolute path of the source file, and character offsets from the start of the file as columns. JS modified (%todo with a local JsError module): Example: module JsError = { let throwWithMessage = (s: string) => s } let f = (_: int): int => %todo("later") Before (diagnostic): This has type: string But it's expected to have type: int After (js): function f(param) { return Stdlib_JsError.throwWithMessage("A.res:4:25-30 - Todo: later"); } JS modified (%todo message): Example: let a = 1 let f = () => %todo Before (js): return Stdlib_JsError.throwWithMessage("/home/me/app/src/D.res:2:24-29 - Todo"); After (js): return Stdlib_JsError.throwWithMessage("D.res:2:14-19 - Todo"); Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Kbf3gJFAva9685fjvkajub Signed-off-by: Christoph Knittel --- changes/todo-jserror-shadowing.fix.md | 2 ++ compiler/frontend/ast_exp_extension.ml | 17 +++++++++------ tests/tests/src/todo_shadowing_test.mjs | 29 +++++++++++++++++++++++++ tests/tests/src/todo_shadowing_test.res | 18 +++++++++++++++ 4 files changed, 59 insertions(+), 7 deletions(-) create mode 100644 changes/todo-jserror-shadowing.fix.md create mode 100644 tests/tests/src/todo_shadowing_test.mjs create mode 100644 tests/tests/src/todo_shadowing_test.res diff --git a/changes/todo-jserror-shadowing.fix.md b/changes/todo-jserror-shadowing.fix.md new file mode 100644 index 0000000000..d3d7fcb997 --- /dev/null +++ b/changes/todo-jserror-shadowing.fix.md @@ -0,0 +1,2 @@ +- Fix `%todo` calling a user-defined `JsError` module instead of the standard library one. +- Fix the location in the `%todo` error message: the file's base name instead of its absolute path, and columns instead of character offsets. diff --git a/compiler/frontend/ast_exp_extension.ml b/compiler/frontend/ast_exp_extension.ml index a2076e7963..745c7f758b 100644 --- a/compiler/frontend/ast_exp_extension.ml +++ b/compiler/frontend/ast_exp_extension.ml @@ -33,17 +33,20 @@ let handle_extension e (_self : Ast_mapper.mapper) | None -> None in Location.prerr_warning e.Parsetree.pexp_loc (Bs_todo todo_message); + (* Like [__LOC__]: the file's base name, so the generated code does not + depend on where it was built, and columns rather than offsets. *) + let file, line, start_col = Location.get_pos_info loc.loc_start in + let end_col = loc.loc_end.pos_cnum - loc.loc_start.pos_cnum + start_col in 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 + Printf.sprintf "%s:%d:%d-%d" (Filename.basename file) line start_col + end_col in Exp.apply ~loc - (Exp.ident ~loc {txt = Longident.parse "JsError.throwWithMessage"; loc}) + (* The compilation unit, not [JsError]: a user module of that name + would shadow the [Stdlib] alias. *) + (Exp.ident ~loc + {txt = Ldot (Lident "Stdlib_JsError", "throwWithMessage"); loc}) [ ( Nolabel, Exp.constant ~loc diff --git a/tests/tests/src/todo_shadowing_test.mjs b/tests/tests/src/todo_shadowing_test.mjs new file mode 100644 index 0000000000..305f45a6be --- /dev/null +++ b/tests/tests/src/todo_shadowing_test.mjs @@ -0,0 +1,29 @@ +// Generated by ReScript, PLEASE EDIT WITH CARE + +import * as Mocha from "mocha"; +import * as Test_utils from "./test_utils.mjs"; +import * as Stdlib_JsError from "@rescript/runtime/lib/es6/Stdlib_JsError.mjs"; + +function throwWithMessage(s) { + return s; +} + +let JsError = { + throwWithMessage: throwWithMessage +}; + +function notImplemented(param) { + return Stdlib_JsError.throwWithMessage("todo_shadowing_test.res:12:38-43 - Todo: later"); +} + +Mocha.describe("Todo_shadowing_test", () => { + Mocha.test("%todo throws even with a local JsError module", () => Test_utils.throws("File \"todo_shadowing_test.res\", line 16, characters 11-18", () => { + Stdlib_JsError.throwWithMessage("todo_shadowing_test.res:12:38-43 - Todo: later"); + })); +}); + +export { + JsError, + notImplemented, +} +/* Not a pure module */ diff --git a/tests/tests/src/todo_shadowing_test.res b/tests/tests/src/todo_shadowing_test.res new file mode 100644 index 0000000000..1803d27e08 --- /dev/null +++ b/tests/tests/src/todo_shadowing_test.res @@ -0,0 +1,18 @@ +// Every %todo reports warning 110, which this test exercises on purpose. +@@warning("-110") + +open Mocha +open Test_utils + +// A local module named JsError must not change what %todo expands to. +module JsError = { + let throwWithMessage = (s: string) => s +} + +let notImplemented = (_: int): int => %todo("later") + +describe(__MODULE__, () => { + test("%todo throws even with a local JsError module", () => { + throws(__LOC__, () => ignore(notImplemented(1))) + }) +}) From 0c2890fb3ef0de4c2eee76026e5f4188487e28b7 Mon Sep 17 00:00:00 2001 From: Christoph Knittel Date: Sun, 1 Nov 2026 16:10:19 +0000 Subject: [PATCH 2/2] Allow the %todo warning in the tests project instead of in the test Every %todo reports warning 110, which the tests project turned into an error. Answers the Codex review on #9160: rather than disabling the warning in todo_shadowing_test.res, the project keeps 110 a warning (-warn-error A-110), and every other warning stays an error. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Kbf3gJFAva9685fjvkajub Signed-off-by: Christoph Knittel --- tests/tests/rescript.json | 2 +- tests/tests/src/todo_shadowing_test.mjs | 6 +++--- tests/tests/src/todo_shadowing_test.res | 3 --- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/tests/tests/rescript.json b/tests/tests/rescript.json index b79f9f59b2..dc53e8cbd9 100644 --- a/tests/tests/rescript.json +++ b/tests/tests/rescript.json @@ -15,6 +15,6 @@ "compiler-flags": [ "-check-lam", "-w -3-6-26-27-29-30-32..40-44-45-52-60-9-106+104", - "-warn-error A" + "-warn-error A-110" ] } diff --git a/tests/tests/src/todo_shadowing_test.mjs b/tests/tests/src/todo_shadowing_test.mjs index 305f45a6be..f9ecfc0cb3 100644 --- a/tests/tests/src/todo_shadowing_test.mjs +++ b/tests/tests/src/todo_shadowing_test.mjs @@ -13,12 +13,12 @@ let JsError = { }; function notImplemented(param) { - return Stdlib_JsError.throwWithMessage("todo_shadowing_test.res:12:38-43 - Todo: later"); + return Stdlib_JsError.throwWithMessage("todo_shadowing_test.res:9:38-43 - Todo: later"); } Mocha.describe("Todo_shadowing_test", () => { - Mocha.test("%todo throws even with a local JsError module", () => Test_utils.throws("File \"todo_shadowing_test.res\", line 16, characters 11-18", () => { - Stdlib_JsError.throwWithMessage("todo_shadowing_test.res:12:38-43 - Todo: later"); + Mocha.test("%todo throws even with a local JsError module", () => Test_utils.throws("File \"todo_shadowing_test.res\", line 13, characters 11-18", () => { + Stdlib_JsError.throwWithMessage("todo_shadowing_test.res:9:38-43 - Todo: later"); })); }); diff --git a/tests/tests/src/todo_shadowing_test.res b/tests/tests/src/todo_shadowing_test.res index 1803d27e08..297207f883 100644 --- a/tests/tests/src/todo_shadowing_test.res +++ b/tests/tests/src/todo_shadowing_test.res @@ -1,6 +1,3 @@ -// Every %todo reports warning 110, which this test exercises on purpose. -@@warning("-110") - open Mocha open Test_utils