Repository navigation
Conversation
Semantic Diff1 commit · Full page Check
|
|
@codex review |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## untagged-no-hardcoded-classes #8966 +/- ##
==============================================================
Coverage 81.22% 81.22%
==============================================================
Files 457 457
Lines 63987 63997 +10
==============================================================
+ Hits 51973 51982 +9
- Misses 12014 12015 +1
🚀 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. |
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>
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: |
Fixes #8964. Stacked on #8949.
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 abstract implementation is accepted too, because it has no representation to contradict the claim. The stdlib'sLazy.res(type t<+'a>under@notUndefinedinLazy.resi) relies on that, and so may bindings.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) andsuper_errors/not_undefined_functor_argument.🤖 Generated with Claude Code
https://claude.ai/code/session_01Fc7LEuyEfynjaKCVEBPmTF