From 86453bbaba1fb2b8beba4cb904e8fc001f33cdfa Mon Sep 17 00:00:00 2001 From: Christoph Knittel Date: Sun, 1 Nov 2026 16:59:37 +0000 Subject: [PATCH] Stop warning 20 on calls of open JS object methods Why: Calling a method read from an open JS object type, such as `obj["getData"]("text/plain")` with `obj: {..}`, reported warning 20 "this argument will not be used by the function". The read adds the method to the object's open row with a fresh type variable as its type, and type_application takes a callee whose type is a fresh variable as a function that never returns. The JS method does receive its arguments. Fixes #8159. The read of a field that the open row gains now records that field's type, and the application skips the warning only when its callee is that read. Callees that cannot return still warn, including field reads on them: `throw(e)(1)`, `throw(e)["m"](x)`, `{"m": throw(e)}["m"](x)`. Diagnostic removed (warning 20 on open object methods): Example: @get external clipboardData: string => {..} = "clipboardData" let text = clipboardData("event")["getData"]("text/plain") let fromThrow = () => throw(Failure("boom"))["m"](Console.log("b")) Before (diagnostic): Warning number 20 Ex.res:2:46-57 this argument will not be used by the function. Warning number 20 Ex.res:3:51-66 this argument will not be used by the function. After (diagnostic): Warning number 20 Ex.res:3:51-66 this argument will not be used by the function. Review: Why does the read record the field type instead of the typed tree marking the callee? The Pexp_apply case clears the record, types the callee and reads the record right away, comparing it with the callee's type by physical equality, so only the callee's own read can match. It passes the result to type_application as ~open_row_method. A typed-tree flag would change Texp_object_get and the .cmt format for a warning. Review: Can a read inside an argument replace the callee's record? No: the check runs before any argument is typed. The fixture covers arguments that read closed and open object fields, first or last, and nested method calls. Review: Why a Request_local key rather than a plain ref? Requests type-checked concurrently on separate domains would share a ref, and one could overwrite another's record between typing a callee and checking it. An ounit test compiles open-object method calls in two concurrent requests with warning 20 on and expects none; it failed on every run with a shared ref. Review: Does a method read before the call, `let o = f("e")` then `o["m"]` and `o["m"]("used")`, need a record of every field? No: the binding keeps the object type at the binding's level, below the level of the later call, so the fresh-variable check does not apply. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Kbf3gJavp7rWUAfzV9rjFj Signed-off-by: Christoph Knittel --- changes/object-method-unused-argument.fix.md | 1 + compiler/ml/typecore.ml | 47 +++++++++++++++++-- ...unused_argument_object_method.res.expected | 36 ++++++++++++++ .../unused_argument_object_method.res | 33 +++++++++++++ .../ounit_compiler_request_tests.ml | 33 +++++++++++++ 5 files changed, 146 insertions(+), 4 deletions(-) create mode 100644 changes/object-method-unused-argument.fix.md create mode 100644 tests/build_tests/super_errors/expected/unused_argument_object_method.res.expected create mode 100644 tests/build_tests/super_errors/fixtures/unused_argument_object_method.res diff --git a/changes/object-method-unused-argument.fix.md b/changes/object-method-unused-argument.fix.md new file mode 100644 index 0000000000..8ae5956216 --- /dev/null +++ b/changes/object-method-unused-argument.fix.md @@ -0,0 +1 @@ +- Fix warning 20 ("this argument will not be used by the function") firing on calls of methods of open JS object types, such as `obj["getData"]("text/plain")`. diff --git a/compiler/ml/typecore.ml b/compiler/ml/typecore.ml index 6a11c596f2..4329b46e3b 100644 --- a/compiler/ml/typecore.ml +++ b/compiler/ml/typecore.ml @@ -2542,6 +2542,15 @@ let should_unify_expected_result_before_typing_lowered_apply funct sargs = type targs = (Asttypes.arg_label * Typedtree.expression option) list +(* The type of the most recent object field read if that read added the field + to an open object row ([obj["m"]] with [obj: {..}]), [None] after any other + read. Only the [Pexp_apply] case reads it: it resets it before typing the + callee and checks it right after, where it is only meaningful when the + callee itself is such a read. Request-local, so requests type-checked + concurrently on other domains cannot overwrite it in between. *) +let open_row_field_read : type_expr option ref Request_local.key = + Request_local.new_key (fun () -> ref None) + (* Eliminate an object-field scheme for a read. A declared field payload is wrapped in [Tpoly]: an empty binder list needs ordinary instantiation, while a nonempty list needs polymorphic instantiation. An inferred field whose @@ -2810,9 +2819,19 @@ and type_expect_ ?deprecated_context ~context ?(recarg = Rejected) assert (sargs <> []); begin_def (); (* one more level for non-returning functions *) + Request_local.get open_row_field_read := None; let funct = type_exp ~deprecated_context:FunctionCall ~context:None env sfunct in + (* Whether the callee is a JS object method that its read added to an open + object row ([obj["m"]] with [obj: {..}]). Its type is a fresh variable + like that of a callee that never returns, but the method receives its + arguments, so [type_application] must not report them as unused. *) + let open_row_method = + match (funct.exp_desc, !(Request_local.get open_row_field_read)) with + | Texp_object_get _, Some ty -> repr ty == repr funct.exp_type + | _ -> false + in (if should_unify_expected_result_before_typing_lowered_apply funct sargs then (* The async wrapper is lowered to a regular application, so thread the @@ -2835,7 +2854,8 @@ and type_expect_ ?deprecated_context ~context ?(recarg = Rejected) let args, ty_res, fully_applied = match translate_unified_ops env funct sargs with | Some (targs, result_type) -> (targs, result_type, true) - | None -> type_application ~context total_app env funct sargs + | None -> + type_application ~context ~open_row_method total_app env funct sargs in end_def (); if transformed_jsx then check_jsx_props ~loc env args; @@ -3666,11 +3686,23 @@ and type_expect_ ?deprecated_context ~context ?(recarg = Rejected) } | Pexp_object_get (e, ({txt = met} as met_loc)) -> ( let obj = type_exp ~context:None env e in + (* An object type without the field gets it from its open row (or the + lookup fails). An object whose type is still a variable, such as + [failwith("x")], does not count. *) + let adds_field_to_row = + match (expand_head env obj.exp_type).desc with + | Tobject fields -> + let fields, _ = Ctype.flatten_fields fields in + not (List.exists (fun (f : Ctype.field_info) -> f.f_name = met) fields) + | _ -> false + in try (* Lookup constrains an open object row to contain the field, without requiring or adding write capability. *) let typ = filter_method env met obj.exp_type in let typ = object_field_use_type env typ in + Request_local.get open_row_field_read + := if adds_field_to_row then Some typ else None; rue { exp_desc = Texp_object_get (obj, met_loc); @@ -4475,8 +4507,8 @@ and translate_unified_ops (env : Env.t) (funct : Typedtree.expression) | _ -> None) | _ -> None -and type_application ~context total_app env funct (sargs : sargs) : - targs * Types.type_expr * bool = +and type_application ~context ~open_row_method total_app env funct + (sargs : sargs) : targs * Types.type_expr * bool = match sargs with (* Special case for ignore: avoid discarding warning *) | [(Nolabel, sarg)] when is_ignore ~env funct -> @@ -4505,9 +4537,16 @@ and type_application ~context total_app env funct (sargs : sargs) : List.map (fun (l1, _) -> {Types.lbl = l1; typ = newvar ()}) sargs in let ty_ret = newvar () in + (* A fresh type variable as function type usually means the function + never returns (e.g. [failwith("x")(1)]), so the arguments are + unused. A JS object method that the read added to an open object + row ([obj["m"]] with [obj: {..}]) also gets a fresh variable, but it + is an ordinary JS method that receives its arguments + ([open_row_method]). *) if ty_fun.level >= ty_ret.level - && not (is_identity_coercion funct.exp_desc) + && (not (is_identity_coercion funct.exp_desc)) + && not open_row_method then List.iter (fun (_, sarg1) -> diff --git a/tests/build_tests/super_errors/expected/unused_argument_object_method.res.expected b/tests/build_tests/super_errors/expected/unused_argument_object_method.res.expected new file mode 100644 index 0000000000..cbdbee71ad --- /dev/null +++ b/tests/build_tests/super_errors/expected/unused_argument_object_method.res.expected @@ -0,0 +1,36 @@ + + Warning number 20 + /.../fixtures/unused_argument_object_method.res:29:51 + + 27 │ + 28 │ // A function that never returns ignores its arguments: warning 20. + 29 │ let ignored = () => throw(Failure("unreachable"))(1) + 30 │ + 31 │ // A field read that cannot return ignores its arguments: warning 20. + + this argument will not be used by the function. + + + Warning number 20 + /.../fixtures/unused_argument_object_method.res:32:62-77 + + 30 │ + 31 │ // A field read that cannot return ignores its arguments: warning 20. + 32 │ let fromLiteral = () => ({"m": throw(Failure("boom"))})["m"](Console.lo + │ g("a")) + 33 │ let fromThrow = () => throw(Failure("boom"))["m"](Console.log("b")) + 34 │ + + this argument will not be used by the function. + + + Warning number 20 + /.../fixtures/unused_argument_object_method.res:33:51-66 + + 31 │ // A field read that cannot return ignores its arguments: warning 20. + 32 │ let fromLiteral = () => ({"m": throw(Failure("boom"))})["m"](Console.lo + │ g("a")) + 33 │ let fromThrow = () => throw(Failure("boom"))["m"](Console.log("b")) + 34 │ + + this argument will not be used by the function. \ No newline at end of file diff --git a/tests/build_tests/super_errors/fixtures/unused_argument_object_method.res b/tests/build_tests/super_errors/fixtures/unused_argument_object_method.res new file mode 100644 index 0000000000..eae8b6174f --- /dev/null +++ b/tests/build_tests/super_errors/fixtures/unused_argument_object_method.res @@ -0,0 +1,33 @@ +@get external clipboardData: string => {..} = "clipboardData" + +// A method of an open JS object type receives its arguments: no warning 20. +let text = clipboardData("event")["getData"]("text/plain") + +// Arguments that read object fields do not change that. +let closed: {"x": string} = {"x": "x"} +let closedArg = clipboardData("e")["getData"](closed["x"]) +let openArg = clipboardData("e")["getData"](clipboardData("o")["x"]) +let closedLast = clipboardData("e")["m"]("a", closed["x"]) +let closedFirst = clipboardData("e")["m"](closed["x"], "a") +let openLast = clipboardData("e")["m"]("a", clipboardData("o")["x"]) +let openFirst = clipboardData("e")["m"](clipboardData("o")["x"], "a") +let nested = clipboardData("e")["m"](clipboardData("f")["n"]("y")) + +// Reading the method before calling it does not change that either. +let retained = clipboardData("e") +let first = retained["m"] +let second = retained["m"]("used") + +let readElsewhere = () => { + let o = clipboardData("e") + let get = () => o["m"] + o["m"]("used")->ignore + get +} + +// A function that never returns ignores its arguments: warning 20. +let ignored = () => throw(Failure("unreachable"))(1) + +// A field read that cannot return ignores its arguments: warning 20. +let fromLiteral = () => ({"m": throw(Failure("boom"))})["m"](Console.log("a")) +let fromThrow = () => throw(Failure("boom"))["m"](Console.log("b")) diff --git a/tests/ounit_tests/ounit_compiler_request_tests.ml b/tests/ounit_tests/ounit_compiler_request_tests.ml index cac6ab4ff7..dd5b5eeabd 100644 --- a/tests/ounit_tests/ounit_compiler_request_tests.ml +++ b/tests/ounit_tests/ounit_compiler_request_tests.ml @@ -1505,6 +1505,38 @@ let concurrent_jsx_diagnostic_tests _context = assert_equal right_expected.stderr (Domain.join right).stderr done) +(* Calls of open JS object methods mark their callee in request-local typing + state. Another request typing applications at the same time must not + overwrite that mark, which would report the method's arguments as unused + (warning 20, off by default and enabled here). *) +let concurrent_open_object_method_tests _context = + with_temp_dir "rewatch-driver-open-method-" (fun root -> + let source = + "@get external obj: string => {..} = \"obj\"\n" + ^ String.concat "" + (List.init 200 (fun i -> + Printf.sprintf "let v%d = obj(\"e\")[\"m\"](\"a\", \"b\")\n" i)) + in + let setup name = + let directory = Filename.concat root name in + ensure_dir directory; + write directory "Calls.res" source; + fun () -> snd (run directory ~extra:["-w"; "+20"] "Calls.res") + in + let left_request = setup "left" in + let right_request = setup "right" in + let expect_clean result = + expect_code 0 result; + assert_equal ~printer:Fun.id "" result.Rescript_compiler_driver.stderr + in + expect_clean (left_request ()); + for _ = 1 to 5 do + let left = Domain.spawn left_request in + let right = Domain.spawn right_request in + expect_clean (Domain.join left); + expect_clean (Domain.join right) + done) + (* Exercise concurrent compiler requests without the process-worker boundary. *) let concurrent_request_isolation_tests _context = with_temp_dir "rewatch-driver-domains-" (fun root -> @@ -1710,6 +1742,7 @@ let suites = "concurrent_diagnostic_recovery" >:: concurrent_diagnostic_recovery_tests; "concurrent_jsx_diagnostic" >:: concurrent_jsx_diagnostic_tests; + "concurrent_open_object_method" >:: concurrent_open_object_method_tests; "concurrent_request_isolation" >:: concurrent_request_isolation_tests; "concurrent_async_context" >:: concurrent_async_context_tests; ]