Repository navigation
Conversation
Quest-Claim: a55e344250325ad4
…m its lib name Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…at stay apart 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>
…tself Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 22 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head: 45cd701
One finding: the ABI/accessor renames leave the existing moq benchmarks using names that are no longer generated. Please migrate those consumers alongside the tests.
Verification: GitHub-only static review of the changed generators, facade collection, fixtures, and benchmark callers. I did not run builds or tests. The check runs inspected for this head were skipped or neutral, so they do not verify just ffi bench.
(Written by review (OpenAI))
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…lashes with A package function named as a type of its module is New and that name when it returns that type, or an Option of it, and takes a trailing _ otherwise. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… area function Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… refuse a facade named as a foreign type's crate A facade whose lib is named core, alloc, or std exported nothing of its own, since the walk took any crate of those names for the standard library. One named url or serde_json that maps that crate's type wrote glue whose two dependencies shared a name; it now fails at collection. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…crate keeps its own dependency A facade whose lib is named url or serde_json and maps that crate's type wrote glue whose dependencies on the facade and on the crate shared one name. The IR now marks a foreign object, the glue depends on its crate as <crate>_foreign, and the census counts objects by the mark instead of by their root. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ver by a spelled path A foreign type shows at the facade's root under its role name (`Url`, `Json`), so the ABI names it as any other item and a facade's own `serde_json::Value` keeps its name. core and alloc are known by the lang items they define, so a facade named `core` keeps its own `SocketAddrV4` and iterators. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Design change: after the round-3 checkpoint, the maintainer chose to continue and to give Go a package per Rust module. They also ruled that the exporter identifies what a type is by its defining crate or module, never by a path the facade could spell.
The next local review starts with (Written by Claude Opus 5.5 with high effort) |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…o package 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>
|
Design change: the maintainer settled the Go question on 2026-11-07: one Go package per Rust crate. Crates can't form import cycles, and modules can (moq-net's, hang's and mux's, and tokio's do), so callers write
The next local review starts with (Written by Claude Opus 5.5 with high effort) |
Finishes quest/m1/ffi-export/names.md: rust2-ffi names every exported item by a collision-free rule and takes the crate set and prefix from Cargo, so
ffi.tomlkeeps onlyexclude.Summary
moq_session_Statsis the type,moq_Session_statsthe method. A field's getter and setter add_getand_set(SWIG's convention), so moq-tokio'sBackoff::initialfield ismoq_tokio_Backoff_initial_get/_setbeside the methodmoq_tokio_Backoff_initial, and the reject fixture'sLimit::maxsetter stays apart fromset_max. A tag or constant is the name in upper case (MOQ_HANG_CATALOG_AUDIOCODEC_OPUS). A clash, which only unconventional names can cause, fails the export at both spans. Every generator derives its names from the C ones, so the field accessors arex_get()/x_set(v)in C++, ax_getproperty andx_set(v)in Python,xGet/xSet(v)in Kotlin, Swift, and Dart, andXGet()/XSet(v)in Go.pubAPI, following itspub usere-exports. Items from other crates are named by their path in the facade (moq'stokio::Clientismoq_tokio_Client).[crates.*]andpackageare gone, and the CLI takes the facade's--manifest-path.[lib] name = "moq"givesmoq_). The fixtures setapi,reject, anddep, so diagnostics andexcludekeys spell paths by the lib crate's name (api::Size, notapi_fixture::Size).ffi.tomlreads onlyexclude, andprefix,package,[crates.*], andrenameare removed (ffi/src/config.rs).ffi/moq/ffi.tomlis rewritten as facade paths.just ffi standinnow writes a facade ofpub uses: moq-net and moq-pattern at the root, kio's waiter and clock items beside them, andhang,mux, andtokiomodules.Newand that name when it returns that type (or aResultorOptionof it, asNew{T}forT::newalready ignores fallibility), and takes a trailing_otherwise. moq'skio::clock::interval(period)iskio.NewClockInterval,kio::clock::wall()(which returnsWall) iskio.NewClockWall, andmux::codec::{aac,mp3,opus}::config(init), which returnhang::catalog::AudioConfig, aremux.Codec{Aac,Mp3,Opus}Config_. The api fixture covers both branches (polar::pointreturnsOption<polar::Point>, soNewPolarPoint; the newspan(start, end)returnsu32, soSpan_).LIMIT).## Decisions).go/holdsgo/<package>/for each crate whose items the facade exports, andgo/rt/, the runtime they share. A crate's package is its name without the prefix and its_(moq_muxismux,serde_jsonisserdejson), so moq's export isnet,pattern,kio,hang,mux,tokio,url,serdejson, andrt, and callers writemux.CodecOpusConfig_andhang.CatalogAudioConfig. Names inside a package drop the facade segments outside the crate (moq::mux::codec::opus::Configismux.CodecOpusConfig).ffi/src/collect.rs,rehome): each item's IR now carries aHome, its crate and how many leading path segments lie outside it. An instance of a generic type, a function naming one, and a method whose instance names a later crate's type belong to the last of the crates involved in rustc'spostorder_cnumsorder, which puts each crate after its dependencies, so every Go import points earlier in that order and the packages cannot form a cycle. moq'smux::catalog::Producer<mux::catalog::hang::Extra>stays inmux. moq-net'sError::from_transport::<E>at moq-tokio's QUIC errors moves totokio(tokio.ErrorFromTransportSessionError), as a package function taking the receiver first, since Go keeps a type's methods in its own package.go.modabove--out, plus the path togo/. With none, the export writesgo/go.moddeclaring module<prefix>, which a caller requires and replaces with that directory, asjust ffi moqandapi_gonow do. No config key: the derivation always has an answer.runtime/rt.gois the sharedrtpackage:Bytesand its lending,ErrClosed, the handle typeHandle_, the//exported call and release, and the contract check, plus generatedBound, ranges,TupleN, and error structs of no crate's type (rt.StringError).runtime/cgo.gois each crate package's copy of what converts its own C types (the arena, buffers, durations, addresses,await_). A record's, enum's, object's, or callback's conversions are generated in whichever package lifts or lowers it.Handle_ *rt.Handle_, so another package can lift one, and a trait exports<Trait>Vtable_(), so another package can lower an implementation without naming another package's C symbols. Callers use neither.url::Urlandserde_json::Valuekeep their parse-from-text mapping unless the facade re-exports that exact type. The old check was crate-wide, so re-exporting any other item of the crate dropped the mapping.ir::Object::foreign), and the glue depends on its crate as<crate>_foreign, apart from the facade's<root>_export. A facade whose lib is namedurlorserde_jsonand maps that crate's type now builds; before, its two glue dependencies shared one key. Census counts objects by the mark rather than by path root. No check identifies a type by a path the facade could spell: a foreign object shows at the facade's root under its role name (Url,Json), soabinames it as any other item and a facade's ownserde_json::Valueandserde_json::Jsonkeep their names; the standard library's crates are known by what they define (Sized,Box,HashMap), and core's socket addresses and core's and alloc's drained iterators by those lang items too, so a facade whose lib is namedcorekeeps its ownSocketAddrV4. Tests:foreign_lib_name,foreign_role_names, andstandard_names. serde's traits are still found by crate name, since that only adds a candidate trait solving then checks.excludemay name what acfgleaves out of this build, such as moq'stokio::unix::Erroron Windows. With facade paths, the check walks the path through the facade's modules, and a missing segment counts as left out when acfgstripped it from that module or from one the module glob-re-exports (ffi/src/driver.rs,configured_out). Its renames side is gone withrename. Test:configured_out.ffi/moq/bench/export.*in all seven languages use the new names (moq_broadcast_Info,moq::Timescale::MICRO(), Go'sPayloadGet(), and thepayload_get/payloadGetproperty).ffi/README.md's naming and config sections are updated to the new names.names.mdand its links. It adds the follow-up quest/m1/ffi-export/modules.md to the epic'sRequired(see below).Choices made without asking
ConnectTargetErroralias: the stand-in re-exports moq-tokio'sconnect::Errorastokio::ConnectTargetError. Kotlin, Swift, Python, Go, and Dart join a type's path segments in Pascal case, soConnectErrorandconnect::Errorwould otherwise both beTokioConnectError. C is unaffected. Removing the alias is the newmodules.mdquest, which this PR adds directly instead of through/quest-plan._from the crate's name, matching the decision'smux.CodecOpusConfig_example; the export refuses two crates with one package name, or one named asrt,C,main, a Go keyword, or a standard package the generated items use.go.modabove--outmakes the export writego/go.mod(module<prefix>), rather than refuse as quest/m4/go/runtime-names.md recommends for rust2-go: rust2-ffi exports every language at once, so a refusal would fail a C-only export.spanin the api fixture:area(size)no longer clashes in Go, sinceAreais nowdep.Area, so the newspan(start, end) -> u32, beside theSpanrecord, keeps the trailing-_branch covered (api.Span_).api.LIMIT,moq::Timescale::MILLI()), so a constant stays apart from a method of the same name.Public API
Breaking for Go callers: the generated Go is now
go/<package>/per crate plusgo/rt/, instead ofgo/<prefix>.go, so imports and qualifiers change (moq.MuxCodecOpusConfig_ismux.CodecOpusConfig_,moq.Bytesisrt.Bytes,moq.ErrClosedisrt.ErrClosed). To migrate, require the export's module (itsgo.mod, or the one above--out) and import each crate's package. An object gains theHandle_field and a trait the<Trait>Vtable_function, both for the generated packages. The IR gainsHomeon every item andApi::crates. Other languages and the C ABI are unchanged, but for the api fixture's newspanfunction.Decisions
Decided 2026-11-06
modules.mdin this PR rather than planning it separately.api.LIMIT,moq::Timescale::MILLI()).Newwhere the function returns the type it clashes with, a trailing_otherwise (rejected: aPath_namesplit such asMuxCodecOpus_config, which needs a second rule once Go gets a package per module; anFn/Callprefix; a trailing_everywhere). Each clash is within one Rust module, so the rule still holds undermodules.md's Go package per module (clock.NewInterval(p),opus.Config_(init)), which its Plan now notes. The decision's exampleClockWall_()forclock::wall()becameNewClockWall(), sincekio::clock::wallreturnsclock::Wall.Decided 2026-11-07
mux.CodecOpusConfig_, as uniffi-bindgen-go lays out crates; within a crate, names keep the settled flat form with theNewand trailing_rule (rejected: one Go package, the earlier recommendation; alias packages per module; one package per import cycle; one package per module, which Go's cycle ban rules out). The packages' import path is derived from thego.modabove--out, with no config key.Jsonname applied only whereforeignis set). Done for the foreign role names and core's and alloc's types.Models
Checks
nix develop --command just ci(lint, quest, ffi check and test) passes on 3889c07, after merging origin/main at efa6300.api_gobuilds the api fixture's five packages (api,dep,url,serdejson,rt) through cgo, vets them, and runs the transcript, futures, closing, bytes, and contract cases.just ffi moqpasses on a715023: moq's export vets as nine packages, andmoq/moq.goimportsnet,pattern,kio,hang,mux,tokio,url,serdejson, andrt, and runs.moq/bench/export.govets against them; the bench itself did not run.foreign_role_names,standard_names,foreign_lib_name, andlookalikepass with crates of one name in separate Go packages.Review
Rewritten by
quest reviewafter each round.01a1b3a8-d966-79c2-98f0-448cc99bf967, round 3The dependency collision is fixed, but ABI naming still misclassifies a facade-owned Value type when its library is named serde_json.ffi/src/abi.rs:142:Apply the Json role name only to foreign objectsffi/src/collect.rs:276:Separate dependency identities instead of requiring a facade rename;ffi/src/glue.rs:105 and ffi/src/glue.rs:266 now distinguish foreign dependencies and type paths from the facade, and ffi/tests/fixtures.rs:1185 covers successful export and compilation of the original url collision.Round 3's finding is fixed in 5dd3a41 (the role name moved into the IR's
shownpath). The next local review starts withquest review --newafter the Go design change.Follow-ups
excludeby making each refused item export or fail.urlitem from the api fixture and keepurl::Url's mapping. It's not added here because it changes the api fixture's generated surface in every language.Local review restarted with
quest review --newat round 1 on 3420e2d. The stored session from rounds 1 to 3 (through f8835e3, clean) recorded each round's findings as a count, which quest 0.1.0 reads as a list, so resuming it failed withthe reviewer's reply does not settle each of the last round's undefined finding(s) once.(Written by Claude Opus 5.5 with high effort)
🤖 Generated with Claude Code