Repository navigation
Conversation
Changelog🚀 New Feature
Semantic Diff1 commit · Full page Read
|
| Before | After |
|---|---|
|
|
|
Signature modified (inferred signature of a module without an interface, Typemod.type_structure)
/** Finds the value. */
@throws(Not_found)
let find = x => x > 0 ? x : throw(Not_found)rescript · Before: the signature in the .cmi
+@throws(Not_found)
let find: int => intReanalyze modified (calls to annotated externals)
// 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")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)
+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)Fixes #9182
Review
- Why the union of the declaration and the values table, rather than one winning? The values table holds the
@throwson the implementation's binding, the declaration the one in its interface. Either can be the only one written, as with an annotated.resiletwhose.resisn't annotated, so the call can throw what either names. - Why does the inferred signature take only the
@throwsfamily and@deprecated, and not every attribute? A doc comment is an attribute too: in the.cmiit would make editing a comment recompile the module's dependents. A module with a.resiis unchanged, since its.cmiholds the interface's declarations. - What does a
@throwswithout a payload mean? Nothing, as on a binding. The guard requires a payload, and the stdlib's two payload-less@throwsin Stdlib_JSON.res now nameJsExn.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9198 +/- ##
=======================================
Coverage 81.73% 81.73%
=======================================
Files 458 457 -1
Lines 65292 65286 -6
=======================================
- Hits 53364 53361 -3
+ Misses 11928 11925 -3
🚀 New features to boost your workflow:
|
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
|
Developer playground preview: https://rescript-lang.github.io/rescript/dev-playground/?version=pr-9198 |
0a37396 to
9b3d665
Compare
cknitt
changed the title
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
9b3d665 to
76f8a78
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
76f8a78 to
68c774c
Compare
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>
68c774c to
1d38461
Compare
The exception analysis knew what library functions throw only from a hand-kept table in
analysis/reanalyze/src/exn_lib.ml, and that table drifted. #9182 is the example: the eight TemporalfromStringOrThrowfunctions were never added, sorescript-tools reanalyze -exceptiontreatedTemporal.PlainDate.fromStringOrThrow("x")as throwing nothing. A few Intl and Belt functions were missing as well. Third-party libraries, and a project's own externals, had no way to declare their exceptions at all.Fixes #9182
The new rule
At each call, the analysis reads
@throws(...)from the declaration of the value being called. The.cmikeeps the attributes of externals and interface declarations, so their@throwsreaches calls from other modules and packages. It also survives module aliases,open,include, functor results and first-class modules.A call's exceptions are
declared(f) ∪ values(f): the declaration's@throws, together with the values-table entry (the@throwson the implementation's own binding, as before). There is no table any more. I chose the union over letting one source win because either source can be the only one written down. An annotated.resiletwhose.resbinding isn't annotated is a common case, and a call can throw whatever either source names.A
@throwswithout a payload still means nothing, as it always has on bindings.Compiler:
@throwsin inferred signaturesA
letin a module without an interface used to lose its attributes in the inferred signature, so its@throwscouldn't reach other modules. That is whyStdlib.panicneeded the table.Typemod.type_structurealready copied a binding's@deprecatedinto the signature item (#8924). It now copies the@throwsattributes the same way, into the signature only. The other spellings (@throwand the deprecated@raises/@raise) are not copied; a separate PR removes them from reanalyze altogether.Other attributes stay out on purpose. A doc comment is an attribute, and having it in the
.cmiwould make editing a comment recompile the module's dependents. A module with a.resiis unaffected, because its.cmiholds the interface's declarations. The test below checks that an annotated binding behind an unannotated.resiis still not reported through the signature. The.cmiformat and magic numbers are unchanged.What moved into the sources
Every stdlib and Belt declaration the table listed now carries its own annotation, in its
.resior, where a module has none, in the.res:getExn/getOrThrowfamily across Belt's collections (including the.res-only internal ones),OptionandResultBelt.Array.getExn/setExn(Assert_failure)List,Null,Nullable,Option,Result,BoolandBigInt*OrThrowJSON.parseOrThrowandstringifyAnyJsErrorthrowers/andmod(Division_by_zero)Stdlib.panicandStdlib.assertEqualThe functions #9182 found missing are annotated too: the Temporal
fromStringOrThrow, the Intl*Exnexternals,Belt.MutableStack.popOrThrow, andBelt.Map.Dict/Belt.Set.DictgetOrThrow.One correction: the old table said
assertEqualthrowsJsExn, but it is implemented withassert(false), which throwsAssert_failure. The annotation now saysAssert_failure. The two payload-less@throwsinStdlib_JSON.resnow nameJsExn.The only change to generated JavaScript is the line number inside
assertEqual'sAssert_failureinStdlib.mjs/Stdlib.cjs, which moved because a line was added above it.I deliberately left unannotated the functions that throw only for some arguments and whose names don't say so: Temporal constructors and arithmetic,
Int.rangewith step 0, andDate.toTemporalInstanton an invalid date. Annotating them would flag every call to them. The%operator, the same primitive asmod, was never in the table, and I've left it as it was.exn_lib.mlis deleted, along with the Yojson stub, its test and the emptyBsJson.res. Calls to bs-json or Yojson functions are reported only if those libraries annotate their declarations.Guard
tests/analysis_tests/tests-reanalyze/checkThrowsAnnotations.mjsruns at the start of the reanalyzetest.sh. It fails when a stdlib or Belt value named*OrThrowor*Exnhas no@throws(...)with a payload. It checks the module's.resi, or everyletand external of a.resthat has no interface.JsError.toJsExnis allowlisted because it converts a value and throws nothing. Removing a single annotation makes the guard fail.Tests
src/exception/ThrowsDecl.res,ThrowsIface.res/.resiandThrowsDeclCalls.resare new. Calls from another module to an annotated external, a nested external, an annotated.resilet(whose.resbinding is unannotated), a.resiexternal, and a module type'sletreached through a functor and a first-class module are reported. So are calls made through a pipe, an alias,openandinclude.callViaLetIncludedcalls an annotatedletin a.reswithout an interface, through aninclude, so the values table can't see it. It is reported only because of the typemod change.callImplOnlyIncludedshows that a.resiwithout@throwsstill wins over an annotated.resbinding.StdlibTest.resandBeltTest.reskeep the cases from the first version of this PR. They also coverpanicandassertEqual, which reach calls through the new signature attributes, plusJsError.RangeError.throwWithMessage,List.headOrThrow,Bool.fromStringOrThrow,Nullable.getOrThrow,JSON.stringifyAny,Belt.Array.getExnandBelt.MutableQueue.popOrThrow.ExnPayloadGuard.restakes overTestYojson.res's case: catching an exception with a payload pattern and a guard, using its own exception.🤖 Generated with Claude Code
https://claude.ai/code/session_01Kbf3gGhHjzBTXqSo8Dckpb