Skip to content

Commit 18ed190

Browse files
cknittclaude
andcommitted
Read @throws from library declarations in exception analysis
Why: The exception analysis knew what library functions throw only from a table kept by hand in exn_lib.ml. The table drifted: the Temporal `fromStringOrThrow` functions were never added (#9182), and neither were a few Intl and Belt ones. Third-party libraries and a project's own externals had no way to say what they throw. A call now takes the exceptions named by the `@throws` on the declaration of the value it calls, and the table is gone. The `.cmi` keeps the attributes of externals and interface declarations, so their `@throws` reaches calls in other modules and packages, through aliases, `open`, `include`, functors and first-class modules. For a `let` in a module without an interface, the type checker now copies the binding's `@throws` into the inferred signature, as it already did for `@deprecated`. The stdlib and Belt declare what their throwing functions throw, and a guard in the reanalyze tests fails when a stdlib or Belt `*OrThrow` or `*Exn` declaration has no `@throws(...)`. Analysis modified (exceptions of a call to `f`, `Exception.Event` loop): Before (rule): throws(`f`) = values(`f`) when `f` is in the values table throws(`f`) = table(`f`) otherwise After (rule): throws(`f`) = declared(`f`) ∪ values(`f`) Signature modified (inferred signature of a module without an interface, `Typemod.type_structure`): Example: /** Finds the value. */ @throws(Not_found) let find = x => x > 0 ? x : throw(Not_found) Before (rescript): the signature in the .cmi let find: int => int After (rescript): @throws(Not_found) let find: int => int Reanalyze modified (calls to annotated externals): Example: // Lib.res @throws(JsExn) external parse: string => int = "parseInt" // App.res let parse = s => Lib.parse(s) @throws(JsExn) let parseAnnotated = s => Lib.parse(s) let date = () => Temporal.PlainDate.fromStringOrThrow("x") Before (reanalyze): Exception Analysis App.res:4:5-18 parseAnnotated throws nothing and is annotated with redundant @throws(JsExn) Analysis reported 1 issues (Exception Analysis:1) After (reanalyze): Exception Analysis App.res:1:5-9 parse might throw JsExn (App.res:1:17) and is not annotated with @throws(JsExn) Exception Analysis App.res:6:5-8 date might throw JsExn (App.res:6:17) and is not annotated with @throws(JsExn) Analysis reported 2 issues (Exception Analysis:2) Review: Why the union of the declaration and the values table, rather than one winning? The values table holds the `@throws` on the implementation's binding, the declaration the one in its interface. Either can be the only one written, as with an annotated `.resi` `let` whose `.res` isn't annotated, so the call can throw what either names. Review: Why does the inferred signature take only the `@throws` family and `@deprecated`, and not every attribute? A doc comment is an attribute too: in the `.cmi` it would make editing a comment recompile the module's dependents. A module with a `.resi` is unchanged, since its `.cmi` holds the interface's declarations. Review: What does a `@throws` without a payload mean? Nothing, as on a binding. The guard requires a payload, and the stdlib's two payload-less `@throws` in Stdlib_JSON.res now name `JsExn`. Fixes #9182 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kbf3gGhHjzBTXqSo8Dckpb Signed-off-by: Christoph Knittel <ck@cca.io>
1 parent 32116b5 commit 18ed190

68 files changed

Lines changed: 484 additions & 308 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎analysis/reanalyze/src/exception.ml‎

Lines changed: 39 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -127,17 +127,18 @@ module Event = struct
127127
if config.Dce_config.cli.debug then Log_.item "%a@." print ev;
128128
exceptions |> Exceptions.iter (fun exn -> extend_exn_table exn loc);
129129
loop (Exceptions.union exn_set exceptions) rest
130-
| ({kind = Call {callee; module_path}; loc} as ev) :: rest ->
130+
| ({kind = Call {callee; module_path}; exceptions = declared; loc} as ev)
131+
:: rest ->
131132
if config.Dce_config.cli.debug then Log_.item "%a@." print ev;
133+
(* [declared] comes from the declaration's [@throws], the values table
134+
from the [@throws] of the implementation's binding. Both describe
135+
what the call can throw, so take both. *)
132136
let exceptions =
133137
match
134138
callee |> Values.find_path values_table ~module_name ~module_path
135139
with
136-
| Some exceptions -> exceptions
137-
| _ -> (
138-
match Exn_lib.find callee with
139-
| Some exceptions -> exceptions
140-
| None -> Exceptions.empty)
140+
| Some exceptions -> Exceptions.union declared exceptions
141+
| None -> declared
141142
in
142143
exceptions |> Exceptions.iter (fun exn -> extend_exn_table exn loc);
143144
loop (Exceptions.union exn_set exceptions) rest
@@ -280,7 +281,7 @@ let traverse_ast ~file ~values_builder ~checks_builder () =
280281
case.c_rhs |> iter_expr self)
281282
in
282283
let is_throw s = s = "Pervasives.raise" || s = "Pervasives.throw" in
283-
(* [/] and [mod] are listed in [Exn_lib] as throwing [Division_by_zero].
284+
(* [/] and [mod] are declared with [@throws(Division_by_zero)] in Pervasives.
284285
They cannot throw when the divisor is a non-zero literal, as in [x / 10]. *)
285286
let is_division s =
286287
s = "Pervasives./" || s = "Pervasives.mod" || s = "Stdlib./"
@@ -322,14 +323,40 @@ let traverse_ast ~file ~values_builder ~checks_builder () =
322323
| _ -> false)
323324
<> None
324325
in
326+
let get_exceptions_from_annotations attributes =
327+
let throws_annotation_payload =
328+
attributes
329+
|> Annotation.get_attribute_payload (fun s ->
330+
s = "throws" || s = "throw" || s = "raises" || s = "raise")
331+
in
332+
let rec get_exceptions payload =
333+
match payload with
334+
| Annotation.StringPayload s ->
335+
[Exn.from_string s] |> Exceptions.from_list
336+
| Annotation.ConstructPayload s when s <> "::" ->
337+
[Exn.from_string s] |> Exceptions.from_list
338+
| Annotation.IdentPayload s ->
339+
[Exn.from_string (s |> Longident.flatten |> String.concat ".")]
340+
|> Exceptions.from_list
341+
| Annotation.TuplePayload tuple ->
342+
tuple
343+
|> List.map (fun payload ->
344+
payload |> get_exceptions |> Exceptions.to_list)
345+
|> List.concat |> Exceptions.from_list
346+
| _ -> Exceptions.empty
347+
in
348+
match throws_annotation_payload with
349+
| None -> Exceptions.empty
350+
| Some payload -> payload |> get_exceptions
351+
in
325352
let expr ~(module_path : Module_path.t) (self : Tast_mapper.mapper)
326353
(expr : Typedtree.expression) =
327354
let loc = expr.exp_loc in
328355
let is_does_no_throw = expr.exp_attributes |> does_not_throw in
329356
let old_events = !current_events in
330357
if is_does_no_throw then current_events := [];
331358
(match expr.exp_desc with
332-
| Texp_ident (callee_, _, _) ->
359+
| Texp_ident (callee_, _, vd) ->
333360
let callee =
334361
callee_ |> Dce_path.from_path_t |> Module_path.resolve_alias module_path
335362
in
@@ -343,9 +370,12 @@ let traverse_ast ~file ~values_builder ~checks_builder () =
343370
"@{<info>%s@} can be analyzed only if called directly"
344371
(callee_name |> Name.to_string);
345372
});
373+
(* A [@throws] on the declaration (an external, a [let] in an
374+
interface, or a library's) is kept in the .cmi, so it reaches calls
375+
from other modules and packages. *)
346376
current_events :=
347377
{
348-
Event.exceptions = Exceptions.empty;
378+
Event.exceptions = get_exceptions_from_annotations vd.val_attributes;
349379
loc;
350380
kind = Call {callee; module_path = module_path.path};
351381
}
@@ -423,32 +453,6 @@ let traverse_ast ~file ~values_builder ~checks_builder () =
423453
:: old_events);
424454
expr
425455
in
426-
let get_exceptions_from_annotations attributes =
427-
let throws_annotation_payload =
428-
attributes
429-
|> Annotation.get_attribute_payload (fun s ->
430-
s = "throws" || s = "throw" || s = "raises" || s = "raise")
431-
in
432-
let rec get_exceptions payload =
433-
match payload with
434-
| Annotation.StringPayload s ->
435-
[Exn.from_string s] |> Exceptions.from_list
436-
| Annotation.ConstructPayload s when s <> "::" ->
437-
[Exn.from_string s] |> Exceptions.from_list
438-
| Annotation.IdentPayload s ->
439-
[Exn.from_string (s |> Longident.flatten |> String.concat ".")]
440-
|> Exceptions.from_list
441-
| Annotation.TuplePayload tuple ->
442-
tuple
443-
|> List.map (fun payload ->
444-
payload |> get_exceptions |> Exceptions.to_list)
445-
|> List.concat |> Exceptions.from_list
446-
| _ -> Exceptions.empty
447-
in
448-
match throws_annotation_payload with
449-
| None -> Exceptions.empty
450-
| Some payload -> payload |> get_exceptions
451-
in
452456
let toplevel_eval (self : Tast_mapper.mapper) (expr : Typedtree.expression)
453457
attributes =
454458
let old_id = !current_id in

‎analysis/reanalyze/src/exn.ml‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,7 @@
11
type t = string
22

33
let compare = String.compare
4-
let decode_error = "DecodeError"
5-
let assert_failure = "Assert_failure"
6-
let division_by_zero = "Division_by_zero"
7-
let invalid_argument = "Invalid_argument"
8-
let js_exn = "JsExn"
94
let match_failure = "Match_failure"
10-
let not_found = "Not_found"
115
let from_lid lid = lid |> Longident.flatten |> String.concat "."
126
let from_string s = s
137
let to_string s = s
14-
let yojson_json_error = "Yojson.Json_error"
15-
let yojson_type_error = "Yojson.Basic.Util.Type_error"

‎analysis/reanalyze/src/exn.mli‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,7 @@
11
type t
22

33
val compare : t -> t -> int
4-
val assert_failure : t
5-
val decode_error : t
6-
val division_by_zero : t
74
val from_lid : Longident.t -> t
85
val from_string : string -> t
9-
val invalid_argument : t
10-
val js_exn : t
116
val match_failure : t
12-
val not_found : t
137
val to_string : t -> string
14-
val yojson_json_error : t
15-
val yojson_type_error : t

‎analysis/reanalyze/src/exn_lib.ml‎

Lines changed: 0 additions & 200 deletions
This file was deleted.
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
- Exception analysis reads `@throws` from library and external declarations, including third-party ones, instead of a built-in list, and a `@throws` on a `let` now reaches other modules even without an interface.

‎compiler/ml/typemod.ml‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1442,11 +1442,18 @@ and type_structure funct_body anchor env sstr =
14421442
in
14431443
(* The environment keeps let-bound values without attributes, so uses
14441444
in this module do not warn. The signature carries their [deprecated]
1445-
attributes, so uses from other modules do. *)
1446-
let deprecated =
1445+
attributes, so uses from other modules do, and their [throws]
1446+
attributes, which reanalyze's exception analysis reads at calls from
1447+
other modules. Other attributes stay out: a doc comment in the .cmi
1448+
would make editing it recompile the module's dependents. *)
1449+
let signature_attributes (attrs : Parsetree.attributes) =
1450+
Builtin_attributes.deprecated_attributes attrs
1451+
@ List.filter (fun ({txt}, _) -> txt = "throws") attrs
1452+
in
1453+
let kept_attributes =
14471454
List.concat_map
14481455
(fun (vb : value_binding) ->
1449-
match Builtin_attributes.deprecated_attributes vb.vb_attributes with
1456+
match signature_attributes vb.vb_attributes with
14501457
| [] -> []
14511458
| attrs ->
14521459
List.map (fun id -> (id, attrs)) (pat_bound_idents vb.vb_pat))
@@ -1460,7 +1467,9 @@ and type_structure funct_body anchor env sstr =
14601467
let vd = Env.find_value (Pident id) newenv in
14611468
let vd =
14621469
match
1463-
List.find_opt (fun (id', _) -> Ident.same id id') deprecated
1470+
List.find_opt
1471+
(fun (id', _) -> Ident.same id id')
1472+
kept_attributes
14641473
with
14651474
| None -> vd
14661475
| Some (_, attrs) ->

0 commit comments

Comments
 (0)