Repository navigation
Follow up on the reflection stack reviews - #5297
Merged
Merged
Conversation
A class of the caller's own that implements reflect.TypeView reached the natives unchanged, and they only unwrap the nine built-in kind views: reflect.Class.new threw "class fields must be type values or reflect.WithMeta<reflect.Type> rows", Union.new and Map.new hit an unreachable!, and implements() answered false. Every public entry point that takes a TypeView is now a BAML wrapper that calls as_type() on it, so interface dispatch runs the user's own implementation, and hands plain type values to a private native: Class.new, Class.extends, Union.new, Map.new, class.Builder.field, class.PendingType.union, Package.with_types, and Type.implements / implemented_by (TypeView's defaults already delegate to these). Public signatures and throws clauses are unchanged. The natives no longer unwrap views themselves. The field-row wrappers (Class.new, Class.extends, Builder.field) test a row against TypeView, which reads a WithMeta row's class type arguments. Rows allocated natively (Type.meta, PendingType.meta, builder rows) had none, which trips the VM's instance invariant; they now carry reflect.Type or reflect.class.PendingType like any generic instance. The stdlib guard test runs every new wrapper, since they loop over lists and map keys inside reflect.
let type T = e accepts any reflect.TypeView, but the VM's type-operand boundary only unwraps reflect.Type, the nine kind views and a builder's pending type. A class of the user's own that implements TypeView type-checked and then failed with "VM internal error: type error: expected type, got instance". Lowering now dispatches TypeView.as_type() on the operand when its static type holds anything else: a user class, the TypeView interface, a type parameter bounded by it, or a union with one of those. reflect.Type, the kind views and a pending type keep the existing path. The checker admits a pending type only on its own, so an operand that gets the call always holds a TypeView.
The docs on reflect.Type.implements said an interface operand always pins its associated types and that there is no way to ask "is this any iterator". That holds only for an interface built with reflect.Type.of<I<...>>(), which names every associated type (defaults filled in). One looked up by name (Package.get_interface, find_interface, interfaces) names none, and the runtime treats an empty request as open, so it matches any implementation, at any type arguments. Standard-library interfaces cannot be looked up by name, so for those the associated types must be named. The docs on Type.implements, the TypeView defaults and get_interface now say so, and tests pin both forms in both directions, including a defaulted associated type and a generic interface.
"Scoped runtime type bindings" still documented the removed `type T = unreflect(e)` statement, E0168 for unreflect elsewhere, and the bounded form as future work. It now documents `let type T = e`, the bounded `let type T extends I = e` with its optional diverging `else`, the runtime bound check (a bound, not an interface type), and E0098 for the removed spellings, as proposed on #5278 with its correction. A TypeView operand binds the type its as_type() returns. A new subsection documents the static type() that baml.Concrete gives every concrete type, with its two lookup rules, rewritten from the #5272 proposal for the final design: type() lives on baml.Concrete and returns reflect.Type.of<Self>(); there is no reflect.Reflectable.
reflect does not depend on baml, but several of its natives still threw baml.errors.InvalidArgument: an uninitialized reflect.Package receiver (get_class and the other lookups, with_types, tests, Session.new's packages), a bad or consumed compile artifact and a dependency mismatch in Package._finish and Session._finish, a failed runtime link, and an uninitialized reflect.Session in Session.eval's compile step. They all throw reflect.errors.InvalidArgument now, with the same messages; no throws clause changes. The heap-only helpers return a message and the native allocates the error. graft.rs gets its own LinkError, which the loading native throws.
antoniosarosi
requested review from
2kai2kai2,
aaronvg and
hellovai
as code owners
October 16, 2026 22:05
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 54.13% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 109 functions across 22 files. (12 skippe… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title accurately relates to the pull request, which addresses review items from the reflection stack work. It is broad and does not identify the main changes, but it remains clear and relevant.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 54.13% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 109 functions across 22 files. (12 skipped: 9 unsupported, 3 too large.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
A rabbit checks a type by moonlit fern,
Then lets each bound its turn to learn.
If one says no, the else-path springs,
While views become the types they bring.
The rabbit hops past mismatch things.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Binary size checks failed❌ 2 violations · ✅ 1 passed
Details & how to fixViolations:
Add/update baselines:
[artifacts.bridge_wasm]
file_bytes = 26133693
gzip_bytes = 7664554
[artifacts.packed-program]
file_bytes = 39920296
gzip_bytes = 16258025Generated by |
2kai2kai2
approved these changes
Oct 16, 2026
| type S = Wrapper<int>; // a static right-hand side fills the slot with its realized template | ||
| let type T = e; // `e: reflect.Type | reflect.TypeView`, evaluated once when the statement runs | ||
| // (a builder's pending type is accepted too, once the builder is built) | ||
| let type U extends I & J = e; // bounded: each bound is checked when the statement runs |
Contributor
There was a problem hiding this comment.
we shouldn't allow fallible checks without an else
Contributor
|
The |
Contributor
|
I'm curious if this bug also applies with generics? |
This reverts commit 7f6dee7. An interface from Package.get_interface leaves its associated types blank, which no interface type written in code can do: the compiler rejects `Source` without `Item`. The runtime accepts it only because bounds and interface types share one representation, a flaw to be fixed separately, so the docs should not promise it and tests should not pin it. Type.implements, implemented_by and the TypeView defaults again say that an interface operand pins its associated types and only an exact implementation matches; the get_interface note and the new tests are gone. The describe snapshot is regenerated.
`let type U extends I & J = e;` checked its bounds when it ran and threw
reflect.errors.TypeMismatch on a miss, a runtime failure the statement did
not say how to handle. A bounded let type must now have an else block, and
a type that misses a bound runs it:
let type U extends I & J = e; // error E0111
let type V extends I = e else { return x }; // unchanged
The checker reports E0111, the code of a refutable `let` pattern, on the
statement: "`let type U extends …` checks its bounds when it runs, so it
needs an `else` block for a type that misses them". The statement no longer
adds reflect.errors.TypeMismatch to an inferred throws set, and lowering
sends every bound miss to the else block.
Nothing else used the BOUND_MISMATCH instruction or the VM's TypeMismatch
error builder, so both are gone. The instruction was the last one, so no
other discriminant moves; FORMAT_VERSION goes to 20. A session re-binds an
earlier bounded binding with an else block that panics, since that step
already checked the bounds.
Tests that relied on the throw now check the else path; the diagnostics
project covers the missing else, and a session test covers it in eval.
TYPE_SYSTEM.md and the `type` keyword docs say the else block is required.
`let type T = builder.type();` compiled and, if the builder was not built yet, threw CompilationError when the statement ran. A plain let type has no else block, so it should never fail at run time. The operand must now be a reflect.Type or a reflect.TypeView; a reflect.class.PendingType is an E0001 compile error: "`let type` cannot bind a class builder's pending type; call `build()` and bind the class it returns". PendingType does not implement reflect.TypeView, so the user-view as_type() path cannot reach it either. With the operand checked statically, nothing hands the VM's type-operand boundary a pending type: call type arguments come from LoadType and bound checks read the bound slot. The pending branch in type_operand_value and its helpers in runtime_class_builder are gone, and MIR no longer counts a pending type among the operands the VM unwraps. The runtime builder test that bound an unbuilt pending type now checks the compile error through a session; the diagnostics project covers it too. TYPE_SYSTEM.md and the `type` keyword docs say what the operand may be.
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the open review items from the reflection stack (#5283), one commit per item.
A user's own
reflect.TypeViewworks everywhere aTypeViewis accepted (Accept reflect kind views anywhere a type is #5277). The natives only unwrapped the nine built-in kind views. Every entry point that takes aTypeViewnow callsas_type()in BAML before it reaches the native, and lowering does the same for alet typeoperand.TYPE_SYSTEM.md (Replace type T = unreflect(e) with let type T = e, with a bounded form #5278, Add a static type() method to every type #5272). "Scoped runtime type bindings" documents
let type, including the bounded form, as proposed on Replace type T = unreflect(e) with let type T = e, with a bounded form #5278 with its correction. A new subsection documentstype()onbaml.Concrete.reflect natives throw
reflect.errors.InvalidArgument, neverbaml.errors.InvalidArgument(Add a static type() method to every type #5272). This covers an uninitializedreflect.Packageorreflect.Session, a bad compile artifact and a failed runtime link. Nothrowsclause changes.reflect.Package { _inner: 1 }.get_class("Foo") // before: throws baml.errors.InvalidArgument // after: throws reflect.errors.InvalidArgumentA bounded
let typeneeds anelseblock (Replace type T = unreflect(e) with let type T = e, with a bounded form #5278). Its bounds are checked when it runs, so it has to say what a miss does. A miss always runs theelseblock now; it never throws. TheBOUND_MISMATCHinstruction is gone, soFORMAT_VERSIONgoes to 20.let typeno longer takes a class builder's pending type (Replace type T = unreflect(e) with let type T = e, with a bounded form #5278). It threwCompilationErrorat run time when the builder was not built yet. Callbuild()and bind the class.Summary by CodeRabbit
baml.Concreteinterface also provides a statictype()member for eligible receivers.let typebindings now require a divergingelsebranch, which runs when a type fails its bounds.build()first. The previous static alias form andunreflectsyntax now produce an error.reflect.errors.InvalidArgumentconsistently.