fix: fix injection filter in object properties #1013
Conversation
📝 WalkthroughWalkthroughThis set of changes introduces a new, modular, and extensible typegraph expansion system, replacing the previous monolithic conversion logic. The refactor introduces configurable duplication and naming engines, restructures type linking and conversion steps, and updates synchronization primitives to use Changes
Sequence Diagram(s)sequenceDiagram
participant Schema as tg_schema::Typegraph
participant ExpansionConfig
participant DupEngine as DuplicationEngine
participant NamingEngine
participant TypegraphExpansion
participant ConversionMap
participant Registry
participant Typegraph
Schema->>ExpansionConfig: expand(schema)
ExpansionConfig->>DupEngine: create from schema
ExpansionConfig->>NamingEngine: create from schema
ExpansionConfig->>TypegraphExpansion: new(config, schema, engines)
TypegraphExpansion->>Registry: extract runtimes, materializers, policies
loop until all steps done
TypegraphExpansion->>ConversionMap: run_conversion_steps()
TypegraphExpansion->>TypegraphExpansion: run_link_steps()
end
TypegraphExpansion->>Typegraph: build expanded Typegraph (with registry, types, names, disconnected_types)
Typegraph->>ExpansionConfig: return Arc<Typegraph>
sequenceDiagram
participant Generator
participant Typegraph
participant ExpansionConfig
participant ManifestBuilder
participant Renderer
Generator->>Typegraph: receive (raw or from input)
Generator->>ExpansionConfig: expand(typegraph)
ExpansionConfig->>Typegraph: produce expanded typegraph
Generator->>ManifestBuilder: build manifest (with expanded typegraph)
ManifestBuilder->>Renderer: render output files
sequenceDiagram
participant CLI
participant Tracing
participant ExpansionConfig
participant Generator
CLI->>Tracing: (if feature enabled) instrument functions
CLI->>Generator: run (with config, args)
Generator->>ExpansionConfig: expand typegraph
Generator->>...: proceed with manifest building and rendering
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/typegraph/graph/src/types/object.rs (1)
24-28: Focused correction to filter only leaf injectionsThis change modifies the
is_injectedmethod to only returntruewhen the injection is a leaf node, correctly matching the PR objective to filter out only leaf nodes of the injection tree.Consider adding a doc comment to explain the concept of injection and the significance of filtering only leaf nodes, which would help future developers understand the reasoning behind this specific implementation.
impl ObjectProperty { + /// Returns true if this property has a leaf injection. + /// Only leaf nodes are considered "injected" as they contain the actual injection value. pub fn is_injected(&self) -> bool { self.injection .as_ref() .filter(|inj| matches!(inj.as_ref(), InjectionNode::Leaf { .. })) .is_some() } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/typegraph/graph/src/types/object.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/typegraph/graph/src/types/object.rs (1)
src/typegate/src/typegraph/types.ts (1)
InjectionNode(84-86)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test-full
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: pre-commit
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1013 +/- ##
=======================================
Coverage 81.15% 81.15%
=======================================
Files 143 143
Lines 18045 18046 +1
Branches 1970 1970
=======================================
+ Hits 14645 14646 +1
Misses 3382 3382
Partials 18 18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/metagen/src/tests/fixtures.rs (1)
122-132:⚠️ Potential issueAvoid potential panic on empty CLI output
tg.pop().unwrap()will panic if the CLI returns zero typegraphs (e.g., compilation error, wrong path).
Return an error instead to keep tests from aborting unexpectedly:- let tg = Arc::new(tg.pop().unwrap()); - Ok(tg) + let tg = tg + .pop() + .map(Arc::new) + .ok_or_else(|| eyre!("no typegraph returned by meta-cli"))?; + Ok(tg)
🧹 Nitpick comments (9)
src/metagen/src/fdk_rs/types.rs (1)
1026-1028: Avoid shadowing thetgvariable to improve readability
tgfirst refers to the schema graph (Arc<tg_schema::Typegraph>) and is immediately re-used for the expanded graph (Arc<typegraph::Typegraph>). Shadowing with two different concrete types makes the code harder to skim and can trip up future refactors.-let tg = create_typegraph(name.into(), nodes)?; -let tg = TypegraphExpansionConfig::default().expand_with_default_params(tg)?; +let tg_schema = create_typegraph(name.into(), nodes)?; +let tg = TypegraphExpansionConfig::default().expand_with_default_params(tg_schema)?;Explicit names make it obvious which instance is which and prevent accidental type mix-ups.
src/metagen/src/client_py/mod.rs (1)
85-91: Eliminate the extraArcclone and unify the matching armsEach match arm clones the
rawArconly to pass it immediately toexpand_with_default_params. You can avoid the duplication and the extra reference increment with a single binding:- let tg = match inputs - .get(Self::INPUT_TG) - .context("missing generator input")? - { - GeneratorInputResolved::TypegraphFromTypegate { raw } => raw.clone(), - GeneratorInputResolved::TypegraphFromPath { raw } => raw.clone(), - _ => bail!("unexpected input type"), - }; + let raw_schema = match inputs + .get(Self::INPUT_TG) + .context("missing generator input")? + { + GeneratorInputResolved::TypegraphFromTypegate { raw } + | GeneratorInputResolved::TypegraphFromPath { raw } => raw, + _ => bail!("unexpected input type"), + }; + + // Expand once + let tg = TypegraphExpansionConfig::default().expand_with_default_params(raw_schema.clone())?;The intent stays the same and we avoid an unnecessary clone on an
Arc.src/typegraph/graph/tests/expanded.rs (1)
22-25: Print stderr only on failure to keep CI output cleanThe unconditional
eprintln!floods CI logs even when the test passes. Consider gating it behind the status check (or an env flag) so it only aids debugging when something actually fails:-if !output.stderr.is_empty() { - eprintln!("---stderr-start---\n{}\n---stderr-end---", - String::from_utf8(output.stderr)?); -} +if !output.status.success() && !output.stderr.is_empty() { + eprintln!("---stderr-start---\n{}\n---stderr-end---", + String::from_utf8(output.stderr)?); +}This keeps the happy-path quiet while still surfacing useful diagnostics on failure.
src/typegraph/graph/src/lib.rs (2)
70-76: Consider documenting and validatingdisconnected_types
The newdisconnected_typesfield is a public collection but there is no accompanying documentation or helper API.
- Why does the key use the raw schema index (
u32) instead of the internalTypeKeyabstraction used elsewhere?- Under which conditions can this map be empty vs. partially-populated?
A short comment or accessor that clarifies the intended life-cycle would avoid misuse and future refactors.
52-55: Re-export is handy, but expose through prelude as well?
TypegraphExpansionConfigis now public-re-exported here, but most downstream code seems to rely onprelude::*.
If that is the common import path, consider also adding the type topub mod preludefor consistency.src/typegraph/graph/src/conv/step.rs (2)
552-565: ExposeStepResult::newpublicly or addFrom<Type>implementation
Several conversion helpers now callStepResult::<G>::new(ty.clone()), which is readable but slightly noisy.
Implementingimpl<G: DupKeyGen> From<Type> for StepResult<G>(or makingnewpub) would let callers writeStepResult::from(ty.clone()), improving ergonomics and reducing boiler-plate.
206-233: Minor optimisation: avoid an extra clone ofty
StepResult::new(ty.clone())immediately clonesty, and a second clone happens insideMapItem::new.
You can move theMapItem::newcall before constructingStepResultand reuse the original value, e.g.:- let ty = Type::Optional(ty_inner.clone()); - let mut result = StepResult::<G>::new(ty.clone()); + let ty = Type::Optional(ty_inner.clone()); + let map_item = MapItem::new(&ty, self.rpath.clone(), self.dkey.clone())?; + let mut result = StepResult::<G>::new(ty);This shaves one
Arcclone per node.src/typegraph/graph/src/conv/mod.rs (2)
144-156: Left-overeprintln!debug output
eprintln!("another round needed for #{idx}");will print on every expansion whenalways_convert_originalis enabled, polluting stdout/stderr of libraries and larger CLI tools.
Recommend gating behindcfg!(debug_assertions)or using thetracingcrate with an appropriate log level.
193-211:converting_disconnected_typesflag never reset
After the first “original” round the flag remainstrue, so every subsequent call torun_conversion_steps(even if no steps are added) will keep inserting converted types intoself.disconnected_types.
Although harmless right now, resetting the flag tofalseafter the round completes would avoid surprises if the loop ever iterates again for other reasons.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
src/meta-cli/src/cli/gen.rs(2 hunks)src/metagen/src/client_py/mod.rs(2 hunks)src/metagen/src/client_rs/mod.rs(2 hunks)src/metagen/src/client_ts/mod.rs(4 hunks)src/metagen/src/fdk_py/mod.rs(2 hunks)src/metagen/src/fdk_rs/mod.rs(2 hunks)src/metagen/src/fdk_rs/stubs.rs(0 hunks)src/metagen/src/fdk_rs/types.rs(2 hunks)src/metagen/src/fdk_ts/mod.rs(4 hunks)src/metagen/src/fdk_ts/types.rs(2 hunks)src/metagen/src/lib.rs(1 hunks)src/metagen/src/tests/fixtures.rs(5 hunks)src/metagen/src/tests/mod.rs(2 hunks)src/typegraph/core/src/utils/metagen_utils.rs(1 hunks)src/typegraph/graph/src/conv/key.rs(1 hunks)src/typegraph/graph/src/conv/map.rs(1 hunks)src/typegraph/graph/src/conv/mod.rs(2 hunks)src/typegraph/graph/src/conv/step.rs(13 hunks)src/typegraph/graph/src/lib.rs(2 hunks)src/typegraph/graph/tests/expanded.rs(1 hunks)
💤 Files with no reviewable changes (1)
- src/metagen/src/fdk_rs/stubs.rs
✅ Files skipped from review due to trivial changes (5)
- src/typegraph/core/src/utils/metagen_utils.rs
- src/typegraph/graph/src/conv/map.rs
- src/typegraph/graph/src/conv/key.rs
- src/metagen/src/lib.rs
- src/metagen/src/tests/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/metagen/src/fdk_ts/types.rs
- src/metagen/src/client_ts/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test-full
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: pre-commit
🔇 Additional comments (11)
src/meta-cli/src/cli/gen.rs (2)
174-174: Simplified typegraph handling.The code now directly uses the
rawtypegraph without unnecessary conversions.
196-196: Fixed inconsistent return type.This change correctly returns a
TypegraphFromTypegatevariant for typegraphs loaded from path, maintaining consistency with the broader codebase changes that now consistently useArc<tg_schema::Typegraph>directly.src/metagen/src/fdk_rs/mod.rs (2)
20-20: Added import for typegraph expansion.The import of
TypegraphExpansionConfigis necessary for the expansion functionality added below.
144-147: Added typegraph expansion to process the injection tree properly.This change adds the explicit expansion of the typegraph before further processing. The
always_convert_original()configuration ensures that the original typegraph is converted regardless of any caching mechanisms.This expansion step is crucial for correctly filtering injection nodes, as it ensures that the typegraph structure is fully expanded with default parameters, which helps address the injection filtering issue mentioned in the PR objectives.
src/metagen/src/client_rs/mod.rs (2)
15-15: Added import for typegraph expansion.The import of
TypegraphExpansionConfigis necessary for the expansion functionality added below.
158-160: Added cloning and expansion of the typegraph for consistent processing.The code now explicitly clones the raw typegraph and applies expansion with default parameters. This ensures consistent preprocessing across different generators and properly handles the injection tree.
This expansion is important for correctly handling nested function types, addressing part of the MET-862 issue mentioned in the PR objectives.
Also applies to: 162-163
src/metagen/src/fdk_py/mod.rs (2)
12-12: Added import for typegraph expansion.The import of
TypegraphExpansionConfigis necessary for the expansion functionality added below.
205-208: Added typegraph expansion for consistent processing across generators.This addition ensures the typegraph is properly expanded with default parameters, adding the
always_convert_original()option to ensure consistent conversion regardless of caching.This change aligns with similar modifications in other generator modules, ensuring consistent handling of the injection tree and properly typing nested functions.
src/metagen/src/fdk_ts/mod.rs (1)
118-120: Handle missing cached-type look-ups gracefully
map.get(&fun.input().key().original()).unwrap()will panic if the original input type was not cached—e.g., ifalways_convert_original()is removed in the future or if the cache is filtered. Prefer an explicit error to avoid hiding this edge case behind a hard-to-trace panic:-let inp_ty = map.get(&fun.input().key().original()).unwrap(); +let inp_ty = map + .get(&fun.input().key().original()) + .expect("cached type for materializer input not found");The
expectmessage documents the invariant and yields a clearer error in the unlikely event it is violated.src/typegraph/graph/src/conv/step.rs (1)
38-53:disconnectedpath segment is hard-coded toInput
For truly disconnected nodes we cannot assume they always belong to the input branch; some may be outputs or other value kinds in the future.
Consider parameterisingValueTypeKindor inferring it from context instead of fixing it toInput, or at least add aTODOexplaining the limitation.src/metagen/src/tests/fixtures.rs (1)
80-106:create_typegraphhelper: missing validation fortest_typesindices
The function blindly assigns indicesidx + 4for the additionaltest_types.
If any of the supplied nodes internally reference indices ≥types.len()this will produce an invalid schema.
Consider validating references or documenting the expectation to callers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/typegraph/graph/src/conv/mod.rs (1)
138-203: Missing error handling for potential unwrap failureThe
runmethod contains anunwrapat line 161 that could panic if the conversion map's first item is unexpectedly missing.Consider adding a fallback or proper error handling:
- let map_item = self.conversion_map.direct.first().unwrap(); - match map_item { - MapItem::Namespace(root, _) => root.clone(), - _ => unreachable!(), + self.conversion_map.direct.first() + .and_then(|map_item| match map_item { + MapItem::Namespace(root, _) => Some(root.clone()), + _ => None, + }) + .ok_or_else(|| anyhow::anyhow!("Missing root namespace in conversion map"))?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
src/meta-cli/Cargo.toml(1 hunks)src/meta-cli/src/cli/completion.rs(1 hunks)src/meta-cli/src/cli/deploy.rs(3 hunks)src/meta-cli/src/cli/dev.rs(1 hunks)src/meta-cli/src/cli/doctor.rs(1 hunks)src/meta-cli/src/cli/fdk_template.rs(1 hunks)src/meta-cli/src/cli/gen.rs(6 hunks)src/meta-cli/src/cli/list.rs(1 hunks)src/meta-cli/src/cli/new.rs(1 hunks)src/meta-cli/src/cli/serialize.rs(1 hunks)src/meta-cli/src/cli/undeploy.rs(1 hunks)src/meta-cli/src/cli/upgrade.rs(2 hunks)src/meta-cli/src/config.rs(2 hunks)src/meta-cli/src/deploy/actors/typegate.rs(1 hunks)src/meta-cli/src/main.rs(2 hunks)src/metagen/src/client_py/mod.rs(2 hunks)src/metagen/src/client_rs/mod.rs(2 hunks)src/metagen/src/client_ts/mod.rs(4 hunks)src/metagen/src/fdk_py/mod.rs(3 hunks)src/metagen/src/fdk_rs/mod.rs(4 hunks)src/metagen/src/fdk_ts/mod.rs(4 hunks)src/typegraph/graph/src/conv/mod.rs(2 hunks)src/typegraph/graph/src/lib.rs(2 hunks)
✅ Files skipped from review due to trivial changes (10)
- src/meta-cli/Cargo.toml
- src/meta-cli/src/cli/fdk_template.rs
- src/meta-cli/src/cli/list.rs
- src/meta-cli/src/cli/doctor.rs
- src/meta-cli/src/cli/undeploy.rs
- src/meta-cli/src/cli/upgrade.rs
- src/meta-cli/src/cli/serialize.rs
- src/meta-cli/src/config.rs
- src/meta-cli/src/cli/deploy.rs
- src/meta-cli/src/deploy/actors/typegate.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- src/metagen/src/client_rs/mod.rs
- src/metagen/src/client_py/mod.rs
- src/metagen/src/fdk_py/mod.rs
- src/metagen/src/fdk_rs/mod.rs
- src/typegraph/graph/src/lib.rs
- src/metagen/src/fdk_ts/mod.rs
- src/meta-cli/src/cli/gen.rs
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test-full
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: pre-commit
🔇 Additional comments (16)
src/meta-cli/src/cli/dev.rs (1)
51-51: Improved build configuration with conditional tracing instrumentationThe change from direct
#[tracing::instrument]to conditional#[cfg_attr(feature = "tracing-instrument", tracing::instrument)]is a good improvement that makes tracing instrumentation optional based on feature flags.src/meta-cli/src/cli/completion.rs (1)
25-25: Conditional tracing based on feature flagMaking the tracing instrumentation conditional via
#[cfg_attr(feature = "tracing-instrument", tracing::instrument)]is consistent with the approach applied across the codebase, allowing for more flexible build configurations.src/meta-cli/src/cli/new.rs (2)
45-45: Conditional tracing based on feature flagApplying
#[cfg_attr(feature = "tracing-instrument", tracing::instrument)]makes tracing instrumentation optional, which is consistent with changes elsewhere in the codebase.
53-53: Improved logging with semantic level macrosThe changes from
println!to proper logging macros (debug!andinfo!) improve the code by:
- Using appropriate log levels for different messages
- Making logs filterable in production
- Ensuring consistent logging patterns across the codebase
Also applies to: 59-59
src/meta-cli/src/main.rs (2)
60-60: Conditional tracing instrumentationMaking the tracing instrumentation conditional using
#[cfg_attr(feature = "tracing-instrument", tracing::instrument)]is a good practice that allows for more flexible build configurations.
85-92: Enhanced logging configurationThe improved RUST_LOG environment variable configuration now properly covers additional modules (
meta,metagen,tg_schema,typegraph) for consistent logging levels across related components.src/typegraph/graph/src/conv/mod.rs (4)
58-97: Well-designed configuration API for typegraph expansionThe new
TypegraphExpansionConfigstruct provides a clean, fluent interface for configuring typegraph expansion with sensible defaults. The documentation explains the purpose of each option clearly.
99-110: Good separation of configuration from implementationThe
TypegraphExpansionstruct properly separates configuration from implementation details. The fields are well-organized with clear responsibilities, including tracking of disconnected types.
205-231: Well implemented recursive conversion stepsThe implementation handles conversion steps recursively while maintaining state in a clean way. The conditional logic for disconnected types is correctly implemented.
233-239: Good use of memory optimization withstd::mem::takeThe
run_link_stepsmethod efficiently usesstd::mem::taketo avoid unnecessary cloning when processing link steps.src/metagen/src/client_ts/mod.rs (6)
11-15: Imports updated to accommodate new architectureThe imports have been properly updated to reference the new modules and types, including
TypegraphExpansionConfig.
45-46: Improved memory management with Arc-wrapped IndexMapConverting the
typesfield from a plainIndexMapto anArc<IndexMap>allows for more efficient sharing of the type cache without unnecessary cloning.
59-63: Added FDK parameter for conditional behaviorThe
TsClientManifest::newmethod now accepts anfdkparameter that's passed toTsTypesPage::new, allowing for conditional behavior during type page creation. The types are now correctly wrapped in anArcfor shared ownership.
68-68: Updated function call to pass shared type referencesThe call to
selections::manifest_pagenow correctly passes theArc-wrapped types memo, ensuring the selection manifest can efficiently share type references.
128-134: Explicit typegraph expansion using new APIThe code now properly clones the raw typegraph and expands it using the new
TypegraphExpansionConfigAPI before passing it to manifest creation. This ensures consistent expansion behavior across different code generators.
135-141: Added helpful logging around manifest building and renderingThe added log statements provide useful information about the progress of manifest building and rendering, which will help with debugging and monitoring.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/typegraph/graph/src/conv/mod.rs (1)
142-162: Consider adding more detailed logging for conservative expansionThe
runmethod includes good high-level logging for the expansion process, but consider adding more detailed logging when processing disconnected types in conservative mode to aid debugging.if self.config.conservative { + tracing::debug!("adding disconnected types for conservative expansion"); for (idx, map_item) in self.conversion_map.direct.iter().enumerate() { if let MapItem::Value(vtype) = map_item { if vtype.default.is_none() { + tracing::trace!("adding disconnected type at index {}", idx); self.conversion_steps .push_back(ConversionStep::disconnected(idx as u32)) } } } self.converting_disconnected_types = true; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/metagen/src/fdk_py/mod.rs(3 hunks)src/metagen/src/fdk_rs/mod.rs(4 hunks)src/metagen/src/fdk_ts/mod.rs(4 hunks)src/typegraph/graph/src/conv/mod.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/metagen/src/fdk_rs/mod.rs
- src/metagen/src/fdk_py/mod.rs
- src/metagen/src/fdk_ts/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test-full
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: pre-commit
🔇 Additional comments (10)
src/typegraph/graph/src/conv/mod.rs (10)
58-65: Well-documented configuration structThe new
TypegraphExpansionConfigprovides clear documentation for theconservativeflag, explaining its purpose for converting original types even when unreachable. This is crucial for understanding the expansion behavior.
67-74: Good builder pattern implementationThe implementation of
conservative()follows a proper builder pattern by returningself, allowing for method chaining. The documentation clearly explains what "original types" means in this context.
75-84: Convenient default parameters expansion methodThe
expand_with_default_paramsmethod provides a convenient way to expand a typegraph with default parameters, simplifying the API for common use cases.
85-101: Well-designed generic expansion methodThe
expandmethod is well-designed with appropriate generic type constraints, allowing for customization of duplication key generation and naming engine while maintaining type safety.
103-114: Clear struct fields for expansion stateThe
TypegraphExpansionstruct has a well-organized set of fields to track the state of the expansion process. The inclusion ofconverting_disconnected_typesflag helps manage the conservative expansion mode.
120-140: Well-implemented constructorThe
newmethod properly initializes all fields with sensible defaults and takes all necessary dependencies as parameters.
164-191: Good performance trackingThe code includes performance tracking that's conditionally compiled only in debug builds, which is a good practice to avoid overhead in production.
193-207: Efficient resource management for final Typegraph assemblyThe code efficiently manages memory by using
std::mem::taketo move resources when assembling the finalTypegraphstructure.
209-235: Well-structured conversion steps executionThe
run_conversion_stepsmethod has a good structure with clear control flow and proper error handling. The approach of returning a boolean to indicate whether more work is needed is clean and effective.
237-243: Efficient link steps executionThe
run_link_stepsmethod efficiently takes the accumulated link steps and processes them, providing clean error handling.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/typegraph/graph/src/expansion/step.rs (1)
489-491: 🛠️ Refactor suggestion
TODO outjectionis still unresolvedOutjection support for function outputs is acknowledged but not implemented:
injection: None, // TODO outjectionBecause outjection can modify output typing, leaving this unhandled may yield an incorrect duplication key and an incomplete expansion.
Either implement outjection propagation now or add a tracking issue so the gap is not forgotten.
🧹 Nitpick comments (7)
src/typegraph/graph/src/types/list.rs (1)
4-5: Avoid*imports to preserve API clarityWildcard imports (
use super::interlude::*) hide the concrete dependencies of this module and make it harder to track the public surface when reading or reviewing the code. Re-export “interlude” items explicitly or keep them grouped in apreludeif you want ergonomic shorthand, but still list the identifiers.src/typegraph/graph/src/types/union.rs (1)
4-5: Prefer explicit imports over wildcardsSame comment as for
list.rs: hiding dependencies via*imports makes grepping and IDE “find usages” less effective. Consider enumerating the symbols or creating a small prelude module with only what you actually need.src/typegraph/graph/src/expansion/map.rs (1)
7-11: Import list can be trimmed
NameRegistryis only used inassign_names; if that method moves behind a feature-gate, the import becomes dead code. Optional: add#[cfg(feature = "naming")]around both the import and the function to avoid unconditional pulls.src/typegraph/graph/src/types/function.rs (2)
4-5: Wild-card imports hide dependenciesRepeated here for completeness: replacing
*imports with explicit names helps maintenance (rust-analyzer, grep, docs). Consider splittinginterludeinto a curatedpreludewith only the handful of items every file needs.Also applies to: 7-8
60-96: Factor out duplicatedOnceLock::set+ error danceThe same pattern (
map.get_ex→OnceLock::set→ custom error) appears in everylinkimplementation. You could DRY this up with a tiny helper:fn set_once_lock<T>( lock: &OnceLock<T>, value: T, ctx: impl std::fmt::Display, ) -> Result<()> { lock.set(value).map_err(|_| eyre!("OnceLock: {ctx} already set")) }Which makes the critical path clearer and reduces boilerplate:
set_once_lock(&self.ty.input, obj.clone(), format!("function input; key={:?}", self.ty.key()))?;Not blocking, but saves ~15 lines per type node.
src/typegraph/graph/src/engines/duplication.rs (1)
78-80:key_for_fn_outputalways returns the default key – outjections ignoredCurrently:
fn key_for_fn_output(&self, _fn_type: &FunctionType) -> Self::Key { Self::Key { injection: None } }If/when outjection support is added (see TODO in
step.rs), the duplication engine will need to honour output-level injections as well.
Placing a FIXME here or wiring the futureoutjectionfield now will prevent silent bugs later.src/typegraph/graph/src/expansion/mod.rs (1)
203-214: Potential quadratic growth when conservative expansion is enabledInside the outer
whileloop we keep re-scanning the wholeconversion_mapand push aConversionStep::disconnectedfor every type whosedefaultisNone, every iteration:for (idx, map_item) in self.conversion_map.direct.iter().enumerate() { … self.conversion_steps.push_back(ConversionStep::disconnected(idx as u32)) }Although
plan()will skip duplicates, we still:
- allocate identical
ConversionSteps repeatedly,- pay hashing / look-ups, and
- keep
run_conversion_stepsreturningtrueeven when nothing new is actually produced.A simple guard (
if !self.converting_disconnected_types) around the push loop or retaining aHashSet<u32>of already-queued indices would cap the complexity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
src/metagen/src/client_py/mod.rs(2 hunks)src/metagen/src/client_rs/mod.rs(2 hunks)src/metagen/src/client_ts/mod.rs(4 hunks)src/metagen/src/fdk_py/mod.rs(3 hunks)src/metagen/src/fdk_rs/mod.rs(4 hunks)src/metagen/src/fdk_rs/types.rs(2 hunks)src/metagen/src/fdk_ts/mod.rs(4 hunks)src/typegate/src/runtimes/substantial/filter_utils.ts(2 hunks)src/typegraph/graph/src/engines/duplication.rs(4 hunks)src/typegraph/graph/src/engines/mod.rs(1 hunks)src/typegraph/graph/src/engines/naming.rs(4 hunks)src/typegraph/graph/src/expansion/map.rs(2 hunks)src/typegraph/graph/src/expansion/mod.rs(1 hunks)src/typegraph/graph/src/expansion/step.rs(18 hunks)src/typegraph/graph/src/key.rs(1 hunks)src/typegraph/graph/src/lib.rs(3 hunks)src/typegraph/graph/src/path.rs(3 hunks)src/typegraph/graph/src/policies.rs(2 hunks)src/typegraph/graph/src/type_registry.rs(4 hunks)src/typegraph/graph/src/types/boolean.rs(1 hunks)src/typegraph/graph/src/types/file.rs(1 hunks)src/typegraph/graph/src/types/float.rs(1 hunks)src/typegraph/graph/src/types/function.rs(2 hunks)src/typegraph/graph/src/types/integer.rs(1 hunks)src/typegraph/graph/src/types/list.rs(2 hunks)src/typegraph/graph/src/types/mod.rs(2 hunks)src/typegraph/graph/src/types/object.rs(3 hunks)src/typegraph/graph/src/types/optional.rs(2 hunks)src/typegraph/graph/src/types/string.rs(1 hunks)src/typegraph/graph/src/types/union.rs(2 hunks)src/typegraph/graph/tests/expanded.rs(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- src/typegraph/graph/src/types/file.rs
- src/typegraph/graph/src/types/string.rs
- src/typegraph/graph/src/types/boolean.rs
- src/typegraph/graph/src/type_registry.rs
- src/typegraph/graph/src/engines/mod.rs
- src/typegraph/graph/src/types/mod.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- src/metagen/src/fdk_rs/types.rs
- src/typegraph/graph/tests/expanded.rs
- src/metagen/src/client_py/mod.rs
- src/typegraph/graph/src/lib.rs
- src/metagen/src/fdk_py/mod.rs
- src/metagen/src/client_rs/mod.rs
- src/metagen/src/fdk_ts/mod.rs
- src/metagen/src/fdk_rs/mod.rs
- src/typegraph/graph/src/types/object.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/typegate/src/runtimes/substantial/filter_utils.ts (1)
src/typegate/src/runtimes/substantial/deno_context.ts (1)
result(254-286)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: build-docker (linux/arm64, ubuntu-22.04-arm)
- GitHub Check: test-full
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
🔇 Additional comments (28)
src/typegraph/graph/src/types/float.rs (1)
4-4: Improved import organization.The import has been simplified by using the new centralized
interludemodule. This change aligns with the broader pattern across type modules and improves consistency without affecting any functionality.src/typegraph/graph/src/types/integer.rs (1)
4-4: Import consolidation looks good.The change to use
super::interlude::*simplifies imports and aligns with the broader refactoring effort across type modules. This improves consistency without changing functionality.src/typegate/src/runtimes/substantial/filter_utils.ts (2)
73-73: Good defensive programming.Adding the nullish check on
resultprevents potential runtime errors when accessing properties on undefined/null values. This increases the robustness of the code.
131-131: Syntax cleanup looks good.Removing the trailing comma improves code consistency and follows best practices.
src/typegraph/graph/src/key.rs (2)
4-4: Import path update looks good.Changing from a relative import to an absolute crate import for
DupKeyaligns with the module restructuring in the codebase.
9-13: Well-implemented TypeKey.original method.This new method correctly creates a TypeKey with the same type index but resets the variant/duplicate ID to zero. This supports the injection filter fix by allowing access to the original type key without variant offsets, which is essential for filtering only leaf nodes in the injection tree.
src/typegraph/graph/src/engines/naming.rs (4)
6-6: Import path update is correct.The import path change from
crate::convtocrate::expansionproperly reflects the new module structure after the expansion system refactoring.
29-29: Clone trait bound addition is appropriate.Adding the
Clonetrait bound toNamingEngineFactoryensures consistency with other engine factories in the codebase and supports the new expansion system architecture.
46-47: Derive Clone implementation is consistent.Adding
Cloneto the derive list forDefaultNamingEngineFactoryproperly implements the new trait bound requirement. This change is aligned with similar updates to other engine factories.
97-98: Import ordering looks good.Moving the
DupKeyimport after the public exports maintains a clean separation between public API and internal implementation details.src/typegraph/graph/src/policies.rs (1)
28-52: Well-implemented policy mapping functionality!The
PolicySpec::newimplementation provides a clean and efficient way to construct policy specifications from indices using the registry. It handles both simple and conditional policies appropriately while maintaining good performance with pre-allocated capacity.src/typegraph/graph/src/path.rs (3)
181-182: Good simplification of path equality check.Removing the
branchfield comparison simplifies the equality check, making it more maintainable while preserving the essential comparison logic.
271-272: Clean simplification of ValueTypePath construction.The simplified constructor for input path improves readability by removing the unnecessary
branchfield.
275-276: Clean simplification of ValueTypePath construction.Similar to the input path, the output path construction is now more concise without the
branchfield.src/typegraph/graph/src/types/optional.rs (2)
11-11: Improved synchronization with OnceLock.Replacing
OncewithOnceLockis a good modernization that provides better thread-safe lazy initialization capabilities.
51-51: Updated trait bound to use the new DuplicationEngine.Changing from
DupKeyGentoDuplicationEnginealigns with the broader refactoring of the duplication system, providing more flexibility and control over the duplication process.src/metagen/src/client_ts/mod.rs (5)
45-46: Improved type sharing with Arc.Using
Arc<IndexMap>instead of justIndexMapfor the types enables efficient sharing of the type mapping across different components, reducing memory usage and potentially improving performance.
59-63: Added support for FDK-specific type generation.The new
fdkparameter allows the client manifest to behave differently when generating code for FDK vs. client usage, addressing part of the PR objective to support conservative expansion for FDK handlers.
68-68: Enhanced type sharing in manifest page.Passing the shared
types_memoto the selections manifest page improves consistency and reduces redundant type mapping operations.
133-133: Explicit typegraph expansion with the new system.Using the new
ExpansionConfig::with_default_engines().expand(tg)approach ensures the typegraph is properly expanded with the modular expansion system before generating client code.
135-140: Improved logging for better debugging.Adding informational logging statements before and after key processing stages improves observability and aligns with the PR objective to enhance logging clarity.
src/typegraph/graph/src/types/list.rs (2)
11-11: Confirm MSRV ≥ 1.70 now thatOnceLockis used
std::sync::OnceLockis only stable starting with Rust 1.70. If the project has an MSRV lower than that, builds will break.
Action items
- Add/adjust the
rust-versionfield in the rootCargo.toml.- Mention the new MSRV in the
CHANGELOG/ release notes.
29-32: 👍 Clean trait migration toDuplicationEngineThe new signature is concise and keeps the link-time API symmetrical with the rest of the expansion machinery. Nothing to change here.
src/typegraph/graph/src/types/union.rs (2)
11-11: MSRV check forOnceLock
UnionType::variantsmoved toOnceLock, which requires Rust ≥ 1.70. Please ensure the crate’s stated MSRV reflects this.
29-32: Good consistency with the newDuplicationEngineabstractionThe refactor keeps the linking API uniform across all type nodes.
src/typegraph/graph/src/types/function.rs (1)
14-15: MSRV reminder forOnceLockFollow-up from other files: ensure MSRV is bumped and documented.
src/typegraph/graph/src/expansion/step.rs (2)
40-52: Check semantic correctness ofRelativePath::Inputfor disconnected types
disconnected()fabricates a step withrpath: RelativePath::Input(ValueTypePath { owner: Default::default(), path: vec![], }),Disconnected types are not necessarily related to function inputs, yet we hard-wire them to the
Inputvariant.
Down-stream code (e.g.ConversionMap::next_keyand name assignment) relies on theRelativePathvariant to decide into which bucket (value/function/namespace) the converted type is stored. Mis-categorising the path could:
- cause the type to be inserted in the wrong map entry, breaking look-ups,
- give the type an “input”-style canonical name although it is not a function argument.
Please double-check whether
Inputis really the intended category here.
If the goal is merely “some value type”, consider introducing a dedicatedRelativePath::Detached(or reuseNsObject/Value) to avoid overloading semantics.
470-492:MapItem::newgets a freshRelativePath::Function(self.idx)– is that valid?Unlike other
convert_*helpers that passself.rpath.clone(),convert_functionbuilds a brand new path:MapItem::new(&ty, RelativePath::Function(self.idx), …)If the current
ConversionStepis not the root (e.g. a function nested inside an object property),self.rpathalready tracks the full path.
Replacing it withRelativePath::Function(self.idx)might:
- lose the surrounding context,
- create two distinct map keys for the very same type, and
- make deduplication/naming inconsistent.
Unless this was intentional, consider keeping the accumulated path:
- MapItem::new(&ty, RelativePath::Function(self.idx), self.dkey.clone())? + MapItem::new(&ty, self.rpath.clone(), self.dkey.clone())?Please verify with a nested-function test case.
tracing-instrumentfeature is enabled (CLI). This will make the output more readable by default.meta(CLI):typegraph,tg_schema,metagen.Migration notes
N/A
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor
Chores