Repository navigation
Conversation
Semantic Diff1 commit · Full page Check
|
|
@codex review |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## instanceof-typeof-annotations #8966 +/- ##
==============================================================
Coverage 81.27% 81.27%
==============================================================
Files 456 456
Lines 63982 64001 +19
==============================================================
+ Hits 52000 52016 +16
- Misses 11982 11985 +3
🚀 New features to boost your workflow:
|
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. |
5f2e4de to
df024f4
Compare
Semantic Diff1 commit · Full page Check
|
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
Why:
Interface inclusion ignored `@notUndefined`. An interface could declare
`@notUndefined type t` over `type t = option<int>`, and clients then
unboxed `option<t>`, so `Some(M.none)` was `None` (#8964).
When the second declaration of an inclusion is `@notUndefined`, the first
must be a type that cannot hold `undefined`, by the rule `option` unboxing
uses, or an abstract type, which has no representation to contradict it.
This covers `.resi` files, functor arguments and `with type` constraints.
The check runs in `Includemod`, since `Typeopt` depends on `Includecore`
through `Typedecl`.
Diagnostic added:
Example:
// Hidden.resi
@notundefined type t
// Hidden.res
type t = option<int>
After (diagnostic):
Type declarations do not match:
type t = option<int>
is not included in
type t
Their runtime representations differ:
the second declaration is @notundefined,
but the first declaration can hold undefined.
Review: Why accept an abstract implementation without the attribute? The
stdlib's `Lazy.res` declares `type t<+'a>` under `@notUndefined` in its
interface, and so may bindings; such a type has no representation that could
hold `undefined`, so requiring the attribute twice would only break them.
Fixes: #8964
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fc7LEuyEfynjaKCVEBPmTF
Signed-off-by: Christoph Knittel <ck@cca.io>
Why:
The check rejected bindings that are fine: an interface's
`@notUndefined type t` over `type t = Dom.element`, an abbreviation of an
abstract type, or over a JavaScript object type, which is never
undefined.
An implementation whose representation the compiler does not see has none
that contradicts the interface: an abstract type, also reached through an
abbreviation, or a JavaScript object type. A functor parameter's or a locally
abstract type stands for any type and does not count as abstract.
Diagnostic removed (an abbreviation of an abstract type):
Example:
// A.resi
@notundefined type t
// A.res
type t = Dom.element
Before (diagnostic):
Their runtime representations differ:
the second declaration is @notundefined,
but the first declaration can hold undefined.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CMkrYaktctdKc4M41WF36t
Signed-off-by: Christoph Knittel <ck@cca.io>
df024f4 to
b19d240
Compare
|
@codex review |
Semantic Diff2 commits · Full page Check
|
Fixes #8964. Stacked on #8950, whose stored type shapes it reads.
Interface inclusion ignored
@notUndefined. An interface could declare@notUndefined type tovertype t = option<int>, and clients then unboxedoption<t>, soSome(M.none)wasNone.Now, when an interface (or a functor parameter, or the original of a
with typeconstraint) declares a type@notUndefined, its implementation must be a type that cannot holdundefined. The rule is the oneoptionunboxing uses: a record, a builtin type other thanoption/unit, an annotated or@notUndefinedtype, and so on. An implementation whose representation the compiler does not see is accepted too, because it has none to contradict the claim: an abstract type, also through an abbreviation such astype t = Dom.element, or a JavaScript object type. The stdlib'sLazy.res(type t<+'a>under@notUndefinedinLazy.resi) relies on that, and so do bindings. The type of a functor parameter or a locally abstract type stands for any type and does not count as abstract.The check sits in
Includemod.type_declarations, which every inclusion goes through, becauseTypeoptdepends onIncludecorethroughTypedecl.UntaggedVariants.mdrule 13 now states it.New tests:
super_errors_multi/Cross_not_undefined_option_implementation(the issue's repro),super_errors/not_undefined_functor_argumentandnot_undefined_functor_parameter, andtests/tests/src/not_undefined_interface_test.res(an abbreviation of an abstract type and an object type, accepted).🤖 Generated with Claude Code
https://claude.ai/code/session_01Fc7LEuyEfynjaKCVEBPmTF