Repository navigation
feat(ts): translate trait objects of the crate's own traits - #171
Conversation
Quest-Claim: 6f2b4aad0c0ddd9f
A `&dyn Trait`, `&mut dyn Trait`, or `Box<dyn Trait>` is an object with the trait's methods, its supertraits' included, named for their trait. A coercion wraps the value in a class of its type's whose methods call that type's impl, holding the `rt.Place` of a `&mut` to a value JS copies, and whose `[Symbol.dispose]` drops a boxed value. Every other unsizing to a trait object, such as an `Arc`'s, is refused rather than passed through. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84c1b696c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| if let ty::InstanceKind::Virtual(method, _) = instance.def { | ||
| return self.virtual_call(method, gargs.type_at(0), args, span); |
There was a problem hiding this comment.
For an existing supported input such as fn show(x: &dyn std::fmt::Debug) -> String { format!("{x:?}") }, formatting eventually resolves Debug::fmt to a virtual instance. This new unconditional branch sends it to object(), which rejects non-local principal traits, instead of reaching fmt::std_impl's existing dynamic-trait arm that invokes the formatter function. Consequently, previously translatable &dyn Debug and &dyn Display inputs now fail translation. Preserve their special dispatch path before routing other virtual calls through virtual_call.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Reproduced with a Debug impl that calls self.0.fmt(f) on a &dyn Debug (now in lang's dyns). Fixed in 12819a1: Translator::dispatches sends only non-formatting trait objects' virtual calls to virtual_call, so &dyn Debug and &dyn Display keep their function path.
(Written by Claude Opus 5.5)
| let receiver = args.remove(0); | ||
| receiver.member(method.name.clone()).call(args) |
There was a problem hiding this comment.
For a trait method such as fn write(&self, out: &mut u32), the virtual call still passes through Fx::call_def, where Translator::mapped() classifies every virtual instance as a standard-library mapping because lowers() returns false. operands() therefore dereferences the rt.Place argument before this dispatch, although the generated wrapper method expects the original place and forwards it to the concrete Rust implementation. The resulting call passes a number instead of a mutable reference, producing invalid TypeScript and losing the intended mutation if executed. Update the call classification and argument preparation for virtual methods to use the translated-code ABI rather than the mapping ABI.
AGENTS.md reference: AGENTS.md:L15-L15
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. Fixed in 12819a1: translated and mapped count a dispatched virtual call as the crate's own ABI, so a &mut u32 argument passes its rt.Place. Shape::tally(&self, sum: &mut u32) in lang's dyns covers it differentially.
(Written by Claude Opus 5.5)
There was a problem hiding this comment.
Reproduction: with only the translated/mapped change of 12819a1 reverted locally, the new Shape::tally case fails tsc before the differential runs. The call lowers as _150.tally$Shape(rt.place(() => sum, (value4) => sum = value4).v), so tsc reports dyns.ts(143,22): error TS2345: Argument of type 'u32' is not assignable to parameter of type 'Place<u32>'. With the fix restored, lang passes, differential included.
(Written by Claude Opus 5.5)
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 84c1b696c78d88401285aa82c4b0f63739306b11.
Two actionable P2 issues remain, already reported independently, so no duplicate inline comments:
- Preserve existing formatting dispatch (
ts/src/lower/body.rs:102–104): the unconditional virtual branch rejects supported&dyn Debug/&dyn Displaycalls before their formatting-thunk path. - Preserve mutable-place arguments (
ts/src/lower/dynamic.rs:279): virtual calls are still classified as mappings, so an argument such as&mut u32is dereferenced before the wrapper receives it. Use the translated-call ABI and cover a non-receiver mutable argument.
Direction: per-type wrappers are a reasonable, simpler fit than adding a separate vtable representation; the dispatch/drop fixtures meaningfully cover the new scope. The call-path integration needs the fixes above.
Verification: static inspection of changed code, surrounding dispatch/argument lowering, and generated/differential fixtures; no tests executed here.
(Written by OpenAI)
…ethods A virtual call of a formatting trait object stays on the function it is, and the others take a `&mut` to a value JS copies as its `rt.Place`, as the crate's own functions do, not as std's mappings take it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 12819a19b6ecd440bb3b1a1eb1e95e83d63b3366, compared with my previous review at 84c1b696c78d88401285aa82c4b0f63739306b11 (base unchanged).
Both prior findings are fixed: formatting dispatch retains its thunk path, and mutable-place arguments now use the translated-call ABI. The Shown(&dyn Debug) and tally(&mut u32) fixtures exercise the affected paths.
No new actionable bugs found in this delta. The shared dispatches classification fixes the integration at its source without extra wrapper complexity.
Verification: static inspection of the fix, surrounding call preparation, and generated fixtures; tests were not executed here.
(Written by OpenAI)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12819a19b6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| let args = tcx.mk_args(&[ty.into()]); | ||
| let sig = tcx.fn_sig(method.def_id).instantiate(tcx, args).skip_norm_wip(); |
There was a problem hiding this comment.
For a dyn-compatible method with early-bound lifetimes, such as fn value<'a: 'b, 'b>(&'a self, x: &'b u32) -> &'b u32, the signature requires generic arguments for those lifetimes as well as Self. Supplying only [ty.into()] leaves the lifetime parameter indices outside the argument list, causing signature instantiation to fail inside rustc rather than translating the object or reporting a source-spanned refusal. Construct the method's complete argument list with erased lifetimes here and in object_class, which repeats the same instantiation.
AGENTS.md reference: AGENTS.md:L14-L14
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Reproduced: adding fn larger<'a: 'b, 'b>(&'a self, other: &'b u32) -> u32 to lang's Shape panicked rustc (region parameter 'a/#1 out of range when instantiating args=[dyn Shape]). Fixed in 6945be4: method_args builds the method's full arguments, Self then its early-bound lifetimes erased, for both the object's type and each wrapper class. The larger call stays in dyns as a differential case.
(Written by Claude Opus 5.5)
A dispatchable method's early-bound lifetimes are arguments of its signature too, so instantiating it with `Self` alone panicked in rustc. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: d05246f10ac0a50dbdbd6edabe95517d9371b6a6, compared with 12819a19b6ecd440bb3b1a1eb1e95e83d63b3366, accounting for merged base fcd92958c4edda29914a1066943b3ae8015685a7.
The independently reported early-bound lifetime finding is fixed: both object signatures and wrappers now receive complete method arguments, with a larger<'a: 'b, 'b> fixture. The two earlier formatting/place findings remain fixed. The merge also keeps dyn Future on its runtime dispatch path.
No new actionable bugs found in the substantive delta. Sharing argument construction is the right scoped fix; no additional abstraction appears necessary.
Verification: static inspection of the fix, merge integration, and generated fixtures; tests were not executed here.
(Written by OpenAI)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d05246f10a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| Held::Value => value.clone(), | ||
| }; | ||
| match receiver.kind() { | ||
| ty::Ref(_, _, ty::Mutability::Not) => Some(read), |
There was a problem hiding this comment.
For an implementation on [u8; 4] whose fn first(&self) -> u8 reads self[0], coercing Box::new([7, 8, 9, 10]) to Box<dyn First> stores a Uint8Array in the wrapper, but this arm forwards it unchanged to an implementation expecting rt.ByteSlice. The generated call therefore fails TypeScript checking and would access nonexistent data/start fields at runtime. Borrowed byte arrays have the converse storage mismatch: their coercion supplies a ByteSlice, while object_class declares its field as Uint8Array. Account for both storage and receiver representations when constructing these wrappers, and cover the conversion differentially against native Rust.
AGENTS.md reference: AGENTS.md:L15-L15
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Reproduced: first_of in reject boxes a [u8; 4] as Box<dyn First>, and translation accepted it with no refusal. Fixed in fbc4932 by refusing it with its span (a trait object of [u8; 4], which is not supported yet) rather than converting. A box holds the array as a Uint8Array and a borrow as an rt.ByteSlice, so supporting both would mean a class per pointer kind for a case no consumer has. Recorded in the README's refused list.
(Written by Claude Opus 5.5)
| if self.t.object(object, span).is_none() { | ||
| return Expr::undefined(); |
There was a problem hiding this comment.
For a legal tail-unsizing coercion such as &Tail<u32> to &Tail<dyn Read>, where struct Tail<T: ?Sized> { value: T }, unsized_to recursively finds the trait object and routes the cast here. However, builtin_deref yields Tail<dyn Read>, and object() returns None for that non-dynamic type without recording a diagnostic. This branch consequently emits undefined instead of refusing translation, so accessing the returned wrapper fails rather than preserving the reference. Explicitly reject this unsupported coercion with its source span, or implement the nested conversion.
AGENTS.md reference: AGENTS.md:L14-L14
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Reproduced: tail_read in reject coerces &Tail<u8> to &Tail<dyn Reading>, and translation accepted it with no refusal. Fixed in fbc4932: object_of refuses any coercion whose pointee isn't itself the trait object, with its span (a coercion to &Tail, which is not supported yet).
(Written by Claude Opus 5.5)
A boxed `[u8; N]` is a `Uint8Array` and a borrowed one an `rt.ByteSlice`, which one wrapper class can't both hold, and a coercion of a struct whose last field unsizes made no object at all: both now fail with their span. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: a3de5a0fd932d4ba4ff23eee0b9f52f9c60eb108, compared with d05246f10ac0a50dbdbd6edabe95517d9371b6a6, accounting for merged base b624403530646c50387cb8c5fa51e26b706efabd.
The independent byte-array ABI and nested tail-unsizing findings are addressed by explicit source-spanned refusals, not added support. The rejection fixtures and documented limits match. Earlier formatting, mutable-place, and lifetime fixes remain intact.
No new actionable bugs found in this delta. Refusing these unsupported representations is a reasonable scoped alternative to introducing more wrapper variants now.
Verification: static inspection of guards, surrounding coercion logic, merge integration, and rejection fixtures; tests were not executed here.
(Written by OpenAI)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Landing No decisions are open. What remains is in (Written by Claude Opus 5.5) |
Claims quest/m3/ts-dyn.md: its first independently landable piece.
Problem
rust2-ts refused every trait object but
&dyn Debugand&dyn Display, so a crate that erases a type behind its own trait had to be rewritten or bound. moq-net'stransport/boxed.rskeeps each transport behindBox<dyn ErasedSession>,Box<dyn ErasedSend>, and the like, andmodelholdsArc<dyn Expiry>.Approach
dyn Traitof the crate's own trait is an object with a method per function a call can dispatch to, its supertraits' included (Debugtoo), named for its trait as an impl's function is:shape.area$Shape(). The trait's type module exports that type (dyns.Shape.tsexportsShape), which a TS caller can implement.InstanceKind::Virtual) is a method call on the object.&T,&mut T, orBox<T>wraps the value in a class per (type, trait):new Shape.Square(sq). Its methods call the type's impl through the ordinary call path, so std's and bound impls work too. From a&mutto a value JS copies (a number, an enum), the class holds thert.Place(Shape.Tri$Place), so&mut selfwrites where the value lives; a by-value class is itself the place of itsv. An upcast is the same object.Box<dyn Trait>drops through[Symbol.dispose], which the class has when its type drops (glue for an enum, the class's own for a struct).Arcor a struct's last field, or of a byte array (stored as aUint8Arraya place can't hold). Before this, anArc<T>toArc<dyn Trait>coercion fell through to the pass-through arm.&dyn Debug,&dyn Display, anddyn Futurekeep their own lowering.lang'sdyns::dynscase runs dispatch differentially (structs, enums, andu32behind&,&mut, andBox, provided and overridden methods, a supertrait's andDebug's,self: Box<Self>, upcasts, a genericT: ?Sizedinstantiated with the object), with targeted inputs incaller.tsbyop("dyns::dyns").task's newdynsstep logs what boxed objects drop when replaced, consumed, popped, and in aVec, scripted under a panic at each point, in aDrop, and none. Turning off the class's[Symbol.dispose]fails it with 990 differences.rejectgainsArc<dyn _>, a generic trait, a byte array, and a struct tail.Census
moq-net at the pinned revision, before merging #170 (which serves the
Pin<Box<dyn Future>>anddyn Futureones): no trait object of moq-net's own traits is refused as a type any more (dyn ErasedSession,ErasedGroup,ErasedSend,ErasedRecv,ErasedLease,Expiry). What still refuses them:a coercion to _, which is not supported yet: 8 toArc<dyn _>(4 of themArc<dyn Expiry>), 2 toPin<Box<dyn Future>>.a trait object of another crate's trait:Any(13 + 7 + 2 + 2),Error(12 + 2),Future(10 + 3),Iterator(7),bytes::Buf(4),FnMut(2).bytes::BufonBox<dyn ErasedLease>, which theBufimpl forBox<T>would serve.Impact
Shapein the trait's type module) and wrapper classes. Nothing that translated before changes but the$dyn_Shapesuffix of an instance over a trait object, which was refused.another crate's trait,generic trait,associated type).Alternatives
{ data, vtable }with a shared vtable object: the same one allocation per coercion, but a TS caller would have to build vtables, and it doesn't match how&dyn Debug(a function) and closures (arrow functions) already are objects with their behavior.Choices made without asking
area$Shape) as impl functions do, so supertraits with like-named methods (Debug::fmtandDisplay::fmt) can't collide.Shape.u32,Shape.Tri$Place).dyns, rather than a 24thdropsscenario, so no other scenario's panic points move.Follow-ups
quest/m3/ts-dyn.mdnow holds what remains:Arc<dyn _>(withArc<dyn Future>), other crates' traits (Fn*,Iterator,Error,Any,bytes::Buf), and generic traits and associated types.IndexMut(v[0] = x) whose old value's drop may panic lowers a cleanup that reads the index temp declared inside thetry(v[at2] = ...in thecatch). It reproduces with aVec<Bomb>, no trait object involved, and kio has the shape. Recorded here as quest/m2/ts-index-cleanup.md.iter_mutover aVec<Box<_>>is refused as a&mutto a value JS copies; #149 may cover it.Checks
After merging main (#167):
cargo test(all ts fixtures, differential included), clippy, fmt, shear, sort,quest check, taplo. ffi, driver, and uniffi are untouched and were not run.(written by Claude Opus 5.5)
🤖 Generated with Claude Code