Repository navigation
feat(ffi): export generic types at the instances the config names - #103
Conversation
Quest-Claim: fcd028eb3bc03965
A config's `instances` names each instance a generic type exports at, with its C name. A signature that reaches an instance the config does not name is refused at its span. moq's config names the catalog producer, consumers, and import container at moq-mux's untyped extension, and drops the excludes they left stale. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0115AT2ue6x9cT5e5J9SZtCn
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: d0fa0b6dab
ℹ️ 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".
| .map(|ty| match ty.kind() { | ||
| ty::Tuple(list) if list.is_empty() => Ok(Type::Unit), | ||
| _ => self.ty(ty, Pos::Behind), |
There was a problem hiding this comment.
When a generic argument is defaulted, instance_ty bypasses the explicit argument restrictions, and this fallback accepts types the generators cannot spell. For example, pub struct Wrapper<T = Option<u32>> { pub value: T } with instance "Wrapper" = "wrapper" reaches the IR with Type::Option as its argument, then panics in Api::arg during ABI generation instead of producing a source-spanned refusal. Reject unsupported defaulted arguments here, or support them consistently in both Rust spelling functions. This violates the root AGENTS.md requirement to report unsupported constructs as source-spanned errors.
AGENTS.md reference: AGENTS.md:L14-L14
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d687109, reproduced first: Holder<T = Option<u32>> named as "Holder" ICEd with "no type argument is Option". Every instance argument, a default included, must now export as one a key can spell ((), a primitive, String, a record, enum, or object). Otherwise the instance is refused at its span with "type argument the config cannot spell". The reject fixture covers it.
(written by 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: d0fa0b6
One actionable issue, already independently reported in this finding: defaulted arguments bypass the supported instance-type restrictions (ffi/src/collect.rs:440-442, 705-712). A public-field Holder<T = Option<u32>> configured as "Holder" = "holder" reaches Api::arg with Type::Option and panics during ABI naming. Validate resolved defaults before IR construction, or support their spelling consistently; cover this with a regression test. Referencing the existing thread rather than duplicating it inline.
Direction: explicit instances and C names keep the export surface predictable; reusing normal collection and rejecting ambiguous inference is preferable to automatic instancing. No additional actionable bugs found in the changed collector, ABI/glue, configuration, and fixtures.
Verification: static GitHub inspection, including an independent collector review. I did not run the compiler, fixtures, or benchmarks.
(Written by OpenAI)
A defaulted argument skipped the key's restrictions, so `Holder<T = Option<u32>>` reached the IR with an `Option` argument and panicked while naming it. Each instance argument must now export as one a key can spell, or the instance is refused at its span. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0115AT2ue6x9cT5e5J9SZtCn
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d68710966a
ℹ️ 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".
| match self.function(item.def_id, method, Some(self_ty), Some(parent)) { | ||
| Ok(function) => methods.push(function), | ||
| Err(why) => refused.push((method, span, why)), | ||
| Err(why) => refused.push((shown, span, why)), |
There was a problem hiding this comment.
When a method constrains an impl parameter in its own where clause, its bounds are never checked if it has no type parameters of its own: impl_args checks only the impl's clauses, and instantiate skips the inherited parameters without calling choose. For example, exporting Wrapper<String> for pub struct Wrapper<T> { pub value: T } with pub fn copied(&self) -> T where T: Copy incorrectly exports copied, even though String is not Copy. The exporter succeeds but generates a call that fails when the generated crate is built. Check the method's instantiated predicates before accepting it, and report an unavailable method at its source span rather than emitting unusable glue.
AGENTS.md reference: AGENTS.md:L14-L14
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7dbf18f, reproduced first: Pair<L, R>::even (where L: PartialEq<R>) exported at Pair<Size, u8>, and the glue failed with E0277. I did not refuse the method. It is now left out of an instance whose bounds it fails, the same way an impl whose where-clauses fail is left out, since Rust has no such method at that instance. Bounds on the method's own type parameters still go to the type-parameter rules. The api fixture calls even from C at Pair<u32>, where it holds.
(written by 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: d687109
Reviewed the one-commit delta from d0fa0b6. The previous default-argument panic finding is fixed: Collector::args now rejects IR argument kinds the formatters cannot spell, including defaulted Option<u32>, before ABI generation. The reject fixture checks the source-spanned refusal.
No new actionable bugs found. Central validation is the simpler, appropriate fix and keeps the explicit instance grammar unchanged.
Verification: static GitHub comparison of the collector and three fixture changes; I did not run tests or reproduce the compiler execution locally.
(Written by OpenAI)
A method's `where` clause on its impl's parameters went unchecked, so `Pair<Size, u8>` exported `even` (`where L: PartialEq<R>`) and the glue failed to build. Such a method is not one of that instance's, as an impl whose bounds fail is not. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0115AT2ue6x9cT5e5J9SZtCn
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 7dbf18f
Reviewed the one-commit delta from d687109. The independent method-bound finding is fixed: applies() substitutes impl arguments and filters methods with failed concrete predicates before emitting glue, while leaving method-generic predicates to type selection. The Pair::even fixture covers an available instance and makes invalid emission for the other configured instances fail the generated-crate build. The earlier default-argument validation remains intact.
No new actionable bugs found. Filtering methods that cannot be called at an instance is consistent with impl filtering and avoids adding a separate configuration workaround.
Verification: static GitHub inspection of the collector, docs and differential fixture changes, with an independent second pass. Tests and generated glue were not executed by this review.
(Written by OpenAI)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0115AT2ue6x9cT5e5J9SZtCn
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Finishes quest/m1/ffi-generic.md, and deletes it.
Problem
The exporter refused every generic type. moq-ffi's media API uses several of them, each at one type, for example
catalog::Producer<hang::Extra>. #94 found that refused generic types blocked about 67 of moq-ffi's partly covered items.Approach
Config
A new
instancestable, besideexcludeandrename, at the top level or under[crates.<pkg>]. Each key is an instance and each value is its C name without the prefix:(), a primitive, orString;Pair<u32>isPair<u32, u32>. A default must also be a type a key could spell, or the instance is refused at its span.Collector
DefIdplus its erased arguments when it is generic. A generic type exports only at the named instances.X<..>" at its span. A note at the type's definition says the type exports only at the instances the config names.impl Pair<u32>reaches onlyPair<u32>. A method whose own where-clause on the impl's parameters fails at the instance is likewise left out (Pair::even,where L: PartialEq<R>).excludecan leave a method out of every instance, by the generic path (catalog::Producer::stats), or out of one instance, by its spelling (Pair<u8, dep_fixture::Size>::swap). The census lists a refused member by its instance spelling.Config<()>::with_catalog<E>(self, Catalog<E>) -> Config<E>.C
instancesvalue, and its methods through that name. For example,moq_catalog_producer_new(moq_broadcast_producer *, moq_catalog_config_extra *, ...).::dep::Pair::<u32, u32>). Docs and messages spell it as Rust does.moq
ffi/moq/ffi.toml:Catalog<Extra>ashang_catalog.Extraor atcatalog::hang::Container:catalog::{Config<()>, Config<Extra>, Producer, Reserved, Timebase}catalog::hang::Consumercontainer::{Consumer, GroupConsumer}import::ContainerProducer/Reservedreturn the types, so the census no longer refuses them:import::{Track, TrackStream},codec::*::Import,container::ts::Programs, andselect::Broadcast::retain.Producer::json_snapshotandProducer::modify;decodeandfinishof the codec imports andPrograms.Census
Each export at the pin. A cell reads before → after.
Producer::{json_*, binary_*}take a type parameter no rule picks.Producer::modifyreturns a borrowingGuard.Catalog<Extra>::mediareturnsCatalog<()>.Coverage of moq-ffi
356 items: moq-ffi's UniFFI functions, methods, and types, without tests. I re-scored the before header by the same method. My count differs from #94's 146 / 114 / 96 by a few judgment calls.
41 items moved from partial to covered:
MoqBroadcastProducer(new,close) andMoqOriginProducer::create_broadcastMoqMediaTimebase,MoqContainerInit, andMoqMediaCatalogProducerMoqMediaContainerProducer:new,write,cut,seek, andfinishMoqMediaTrackProducerandMoqMediaTrackStreamProducer.What still blocks the 72 partial items, ranked:
Producer::modify's borrowingGuard, 3.hang::catalog::Containerexports as an opaque object, sinceUnknownholds aserde_json::Map, 7.FromStrforEpochorPattern, 4.Option<&T>parameters ofSource::resolve, 3.DEFAULT_NAMEonhang::Catalog<()>, which is not named (see Decisions).Tests
apifixture. C compiled against the generated header runs the same script as the Rust transcript, and the two transcripts must match line for line. New in the fixture:Pair<L, R = L>, a record, at three instances:Pair<u32>(a defaulted argument),Pair<Size, u8>, andPair<u8, Size>.swapreturns one instance as the other.impl Pair<u32>addssum.Pair<u32>::beside<T: Copy>resolvesTtoSizeby the new rule. It checksu32 as u8truncation from C.swapby its spelling.Stack<Counter>, an object whose handles move in and out.rejectfixture, each refusal at its span:fn generic<T: Copy>(T) -> Tstays refused. The new rule applies only to an argument of a generic type in the signature.Impact
instancestable, top-level and per crate. Nothing existing changes meaning.Record,Enum, andObjectgainargs, the instance's type arguments. An instance keeps its generic type'spath.instances", not in the exclude list.moq_hang_catalog_*andmoq_catalog_{config,config_extra,producer,reserved,timebase,hang_consumer}_*. It also gainsmoq_container_{consumer,group_consumer}_*,moq_import_container_*,moq_import_track_*,moq_import_track_stream_*, and the codec imports. No existing C name changes.No MIGRATE entry, since nothing is released.
Alternatives
rename("Producer<Extra>" = "catalog_producer"): one table fewer. But a rename renames something that exports anyway, while an instance creates an export. Keeping them separate keeps "names nothing exported" a clear error for each.usepath (hang::Extra) instead of the IR root (moq_mux::catalog::hang::Extra): shorter, but an item has several paths, while the IR root is the one every other key and message uses.Decisions
These are choices I made without asking. Each can be revisited.
instancestable with explicit C names, as above. Recommend keeping it.with_catalogneeds it, sinceProducer::newtakesConfig<Extra>and onlywith_catalogbuilds one. Recommend keeping it. The other option is to nameConfig<Extra>and require moq to add a non-generic constructor.renameof a method by its generic path names it at every instance. Two named instances then clash, which is a loud error. I did not add per-instance renames, since no consumer needs one yet.hang_catalog: the rule'smoq_catalogwould collide its accessors with thecatalogmodule's types.catalog_configforConfig<()>andcatalog_config_extraforConfig<Extra>.#defines, since a const's value may depend on the arguments. Recommend keeping this.hang::Catalog<()>is not named. Naming it would export a second full catalog type forDEFAULT_NAMEanddefault_subscription(1 partial item). Recommend leaving it out until a consumer needs more of it.Follow-ups
json_*/binary_*producers' type parameter;modify's borrowingGuard;Catalog<()>.Checks
just checkandjust testpass at 7dbf18f.Option<u32>bypassed the key's restrictions and ICEd. d687109 refuses it (reject fixture), reproduced against d0fa0b6.Pair::even), reproduced against d687109.just ffi moqpasses and printsok.just ffi censusruns at the pin.just ffi bench, which needs the bench shell.(written by Opus 5.5)
🤖 Generated with Claude Code
https://claude.ai/code/session_0115AT2ue6x9cT5e5J9SZtCn