From a060f8fbd3fe3d9561cd8a574f00b1af523fe124 Mon Sep 17 00:00:00 2001 From: Brian Thorne Date: Wed, 3 Jun 2026 12:05:51 +1200 Subject: [PATCH 1/3] Fix Python submodule import ordering --- CHANGELOG.md | 5 + reflectapi-demo/src/tests/namespace.rs | 54 +++++++ reflectapi-demo/tests/codegen_coverage.rs | 46 ++++++ reflectapi/src/codegen/python.rs | 142 +++++++++++++++--- .../codegen/rust-dependency-stubs/Makefile | 15 +- reflectapi/src/lib.rs | 3 + 6 files changed, 240 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b53fdc86..009172b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,10 @@ # Changelog +## Unreleased + +- Python package codegen now emits sibling submodule imports in dependency order, so Python 3.14/Pydantic can import generated packages where one sibling model annotation references another sibling namespace. +- Rust codegen snapshot typechecking now handles macOS proc-macro library names instead of assuming Linux-style `.so` files. + ## 0.17.5 Python client generation improvements: diff --git a/reflectapi-demo/src/tests/namespace.rs b/reflectapi-demo/src/tests/namespace.rs index a92b2abd..cf7b3455 100644 --- a/reflectapi-demo/src/tests/namespace.rs +++ b/reflectapi-demo/src/tests/namespace.rs @@ -656,6 +656,60 @@ fn test_python_split_modules_import_parent_for_top_level_refs() { ); } +#[test] +fn test_python_split_modules_order_sibling_imports_by_references() { + #[derive( + Debug, serde::Serialize, serde::Deserialize, reflectapi::Input, reflectapi::Output, + )] + struct Issue164Rule { + id: String, + } + + #[derive( + Debug, serde::Serialize, serde::Deserialize, reflectapi::Input, reflectapi::Output, + )] + struct Issue164Group { + rule: Issue164Rule, + } + + async fn group_get(_s: S, _: reflectapi::Empty, _: reflectapi::Empty) -> Issue164Group { + unimplemented!() + } + + let (schema, _) = reflectapi::Builder::<()>::new() + .route(group_get, |b| b.name("commerce.group.get")) + .rename_types( + "reflectapi_demo::tests::namespace::Issue164Group", + "commerce::group::Group", + ) + .rename_types( + "reflectapi_demo::tests::namespace::Issue164Rule", + "commerce::rule::Rule", + ) + .build() + .unwrap(); + + let files = reflectapi::codegen::python::generate_files( + schema, + &reflectapi::codegen::python::Config::default(), + ) + .unwrap(); + + let commerce_file = files.get("commerce/__init__.py").unwrap(); + let group_import = commerce_file.find("from . import group").unwrap(); + let rule_import = commerce_file.find("from . import rule").unwrap(); + assert!( + rule_import < group_import, + "`rule` must import before `group` because `group` references `commerce.rule`:\n{commerce_file}" + ); + + let group_file = files.get("commerce/group/__init__.py").unwrap(); + assert!( + group_file.contains("rule: commerce.rule.Rule"), + "{group_file}" + ); +} + #[test] fn test_python_split_modules_handles_sys_root_and_sanitized_class_names() { #[derive( diff --git a/reflectapi-demo/tests/codegen_coverage.rs b/reflectapi-demo/tests/codegen_coverage.rs index 138e0074..fffd9c07 100644 --- a/reflectapi-demo/tests/codegen_coverage.rs +++ b/reflectapi-demo/tests/codegen_coverage.rs @@ -48,6 +48,16 @@ async fn codegen_coverage( Ok(coverage::CoverageResponse { ok: true }) } +async fn commerce_group( + _: Arc, + _request: reflectapi::Empty, + _headers: reflectapi::Empty, +) -> Result { + Ok(commerce::group::Group { + rule: commerce::rule::Rule { id: String::new() }, + }) +} + fn builder() -> reflectapi::Builder> { reflectapi::Builder::new() .name("Codegen coverage") @@ -60,6 +70,10 @@ fn builder() -> reflectapi::Builder> { b.name("coverage.edges") .description("Coverage fixtures for codegen edge cases") }) + .route(commerce_group, |b| { + b.name("coverage.commerce_group") + .description("Coverage fixture for sibling package import ordering") + }) } #[test] @@ -136,6 +150,38 @@ mod order { } } +mod commerce { + pub mod rule { + #[derive( + Debug, + Clone, + serde::Serialize, + serde::Deserialize, + reflectapi::Input, + reflectapi::Output, + )] + pub struct Rule { + pub id: String, + } + } + + pub mod group { + use super::rule::Rule; + + #[derive( + Debug, + Clone, + serde::Serialize, + serde::Deserialize, + reflectapi::Input, + reflectapi::Output, + )] + pub struct Group { + pub rule: Rule, + } + } +} + mod coverage { use std::collections::HashMap; diff --git a/reflectapi/src/codegen/python.rs b/reflectapi/src/codegen/python.rs index d01f6cc0..0b39d417 100644 --- a/reflectapi/src/codegen/python.rs +++ b/reflectapi/src/codegen/python.rs @@ -2411,6 +2411,125 @@ fn module_rendered_code(module: &templates::Module) -> String { .join("\n\n") } +fn module_subtree_rendered_code(module: &templates::Module) -> String { + let mut chunks = Vec::new(); + let rendered = module_rendered_code(module); + if !rendered.is_empty() { + chunks.push(rendered); + } + for sub in module.submodules.values().filter(|sub| !sub.is_empty()) { + let rendered = module_subtree_rendered_code(sub); + if !rendered.is_empty() { + chunks.push(rendered); + } + } + chunks.join("\n\n") +} + +fn ordered_child_modules( + module: &templates::Module, + ns_path: &[String], +) -> anyhow::Result> { + let children: Vec<(&templates::Module, String)> = module + .submodules + .values() + .filter(|sub| !sub.is_empty()) + .map(|sub| (sub, safe_python_module_segment(&sub.name))) + .collect(); + let child_names: Vec = children.iter().map(|(_, name)| name.clone()).collect(); + + let mut deps: HashMap> = HashMap::new(); + for (child, child_name) in &children { + let rendered_code = module_subtree_rendered_code(child); + for sibling_name in &child_names { + if sibling_name == child_name { + continue; + } + let mut sibling_path = ns_path.to_vec(); + sibling_path.push(sibling_name.clone()); + let sibling_qualified_name = module_qualified_name(&sibling_path); + if contains_qualified_name(&rendered_code, &sibling_qualified_name) { + deps.entry(child_name.clone()) + .or_default() + .push(sibling_name.clone()); + } + } + } + + fn has_cycle( + name: &str, + deps: &HashMap>, + visited: &mut HashSet, + stack: &mut Vec, + ) -> bool { + if visited.contains(name) { + return false; + } + if stack.iter().any(|stacked| stacked == name) { + return true; + } + + stack.push(name.to_string()); + let cycle = deps.get(name).is_some_and(|child_deps| { + child_deps + .iter() + .any(|dep| has_cycle(dep, deps, visited, stack)) + }); + stack.pop(); + visited.insert(name.to_string()); + cycle + } + + let mut cycle_visited = HashSet::new(); + let mut cycle_stack = Vec::new(); + for child_name in &child_names { + if has_cycle(child_name, &deps, &mut cycle_visited, &mut cycle_stack) { + return Ok(child_names); + } + } + + fn visit( + name: &str, + deps: &HashMap>, + visited: &mut HashSet, + stack: &mut Vec, + ordered: &mut Vec, + ) -> anyhow::Result<()> { + if visited.contains(name) { + return Ok(()); + } + if let Some(start) = stack.iter().position(|stacked| stacked == name) { + let mut cycle = stack[start..].to_vec(); + cycle.push(name.to_string()); + anyhow::bail!( + "Python submodule import cycle while ordering generated package imports: {}", + cycle.join(" -> ") + ); + } + + stack.push(name.to_string()); + if let Some(child_deps) = deps.get(name) { + for dep in child_deps { + visit(dep, deps, visited, stack, ordered)?; + } + } + stack.pop(); + + visited.insert(name.to_string()); + ordered.push(name.to_string()); + Ok(()) + } + + let mut visited = HashSet::new(); + let mut ordered = Vec::with_capacity(child_names.len()); + let mut stack = Vec::new(); + for child_name in &child_names { + visit(child_name, &deps, &mut visited, &mut stack, &mut ordered)?; + } + + Ok(ordered) +} + fn referenced_namespace_roots( generation: &PythonGeneration, rendered_code: &str, @@ -2711,12 +2830,7 @@ fn render_module_file( body.push('\n'); } - let child_modules: Vec = module - .submodules - .values() - .filter(|sub| !sub.is_empty()) - .map(|sub| safe_python_module_segment(&sub.name)) - .collect(); + let child_modules = ordered_child_modules(module, ns_path)?; if !child_modules.is_empty() { if !body.trim().is_empty() { body.push('\n'); @@ -2913,13 +3027,7 @@ fn render_client_file( ) -> anyhow::Result { let mut parts = vec![generation.preamble.clone()]; - for root in generation - .module_tree - .submodules - .values() - .filter(|sub| !sub.is_empty()) - .map(|sub| safe_python_module_segment(&sub.name)) - { + for root in ordered_child_modules(&generation.module_tree, &[])? { parts.push(format!("from . import {root}")); } @@ -3016,13 +3124,7 @@ fn render_root_init_py(generation: &PythonGeneration, config: &Config) -> anyhow format!("from ._client import {}", imports.join(", ")), ]; - for root in generation - .module_tree - .submodules - .values() - .filter(|sub| !sub.is_empty()) - .map(|sub| safe_python_module_segment(&sub.name)) - { + for root in ordered_child_modules(&generation.module_tree, &[])? { lines.push(format!("from . import {root}")); imports.push(root); } diff --git a/reflectapi/src/codegen/rust-dependency-stubs/Makefile b/reflectapi/src/codegen/rust-dependency-stubs/Makefile index df85aa5d..21b260a8 100644 --- a/reflectapi/src/codegen/rust-dependency-stubs/Makefile +++ b/reflectapi/src/codegen/rust-dependency-stubs/Makefile @@ -1,4 +1,10 @@ RUSTC = rustc +HOST = $(shell $(RUSTC) -vV | sed -n 's/^host: //p') +PROC_MACRO_EXT = so +ifneq (,$(findstring apple-darwin,$(HOST))) +PROC_MACRO_EXT = dylib +endif +SERDE_DERIVE_DYLIB = libserde_derive.$(PROC_MACRO_EXT) .PHONY: check deps clean @@ -20,7 +26,7 @@ check: libreflectapi.rlib \ generated.rs clean: - rm -f *.rlib *.rmeta *.so + rm -f *.rlib *.rmeta *.so *.dylib *.dll # Make sure to edit `typecheck` in `codegen/rust.rs` to copy all the files into the right place libreflectapi.rlib: libserde.rlib \ @@ -39,13 +45,12 @@ libreflectapi.rlib: libserde.rlib \ --extern futures_util=libfutures_util.rlib \ reflectapi.rs -libserde.rlib: libserde_derive.rmeta libserde_derive.so - $(RUSTC) --edition 2021 --crate-type=rlib --crate-name=serde --emit=metadata,link -L . --extern serde_derive=libserde_derive.rmeta --extern serde_derive=libserde_derive.so serde.rs +libserde.rlib: libserde_derive.rmeta $(SERDE_DERIVE_DYLIB) + $(RUSTC) --edition 2021 --crate-type=rlib --crate-name=serde --emit=metadata,link -L . --extern serde_derive=libserde_derive.rmeta --extern serde_derive=$(SERDE_DERIVE_DYLIB) serde.rs -libserde_derive.rmeta libserde_derive.so: serde_derive.rs +libserde_derive.rmeta $(SERDE_DERIVE_DYLIB): serde_derive.rs $(RUSTC) --edition 2021 --crate-type=proc-macro --crate-name=serde_derive --emit=metadata,link -L . serde_derive.rs lib%.rlib: %.rs $(RUSTC) --edition 2021 --crate-type=rlib --crate-name=$* --emit=metadata,link -L . $*.rs - diff --git a/reflectapi/src/lib.rs b/reflectapi/src/lib.rs index be844ef3..71e550ea 100644 --- a/reflectapi/src/lib.rs +++ b/reflectapi/src/lib.rs @@ -7,6 +7,8 @@ //! (complete main.rs example can be found in [https://github.com/thepartly/reflectapi/tree/main/reflectapi-demo](https://github.com/thepartly/reflectapi/tree/main/reflectapi-demo)) //! //! ```rust +//! # #[cfg(feature = "builder")] +//! # mod quick_start { //! //! #[derive(Debug)] //! pub struct AppState { @@ -89,6 +91,7 @@ //! } //! } //! } +//! # } //! ``` //! //! Generated client in Typescript (one of the languages supported by the codegen) example: From 25d8318413e2a12420f04b3877e334c9b509df0d Mon Sep 17 00:00:00 2001 From: Brian Thorne Date: Wed, 3 Jun 2026 13:16:06 +1200 Subject: [PATCH 2/3] Avoid false root Python import dependencies --- reflectapi-demo/src/tests/namespace.rs | 71 ++++++++++++++++++++++++++ reflectapi/src/codegen/python.rs | 22 +++++++- 2 files changed, 92 insertions(+), 1 deletion(-) diff --git a/reflectapi-demo/src/tests/namespace.rs b/reflectapi-demo/src/tests/namespace.rs index cf7b3455..83c37fb6 100644 --- a/reflectapi-demo/src/tests/namespace.rs +++ b/reflectapi-demo/src/tests/namespace.rs @@ -710,6 +710,77 @@ fn test_python_split_modules_order_sibling_imports_by_references() { ); } +#[test] +fn test_python_split_modules_root_sibling_order_ignores_nested_suffixes() { + #[derive( + Debug, serde::Serialize, serde::Deserialize, reflectapi::Input, reflectapi::Output, + )] + struct Issue164NestedLeaf { + id: String, + } + + #[derive( + Debug, serde::Serialize, serde::Deserialize, reflectapi::Input, reflectapi::Output, + )] + struct Issue164XRoot { + leaf: Issue164NestedLeaf, + } + + #[derive( + Debug, serde::Serialize, serde::Deserialize, reflectapi::Input, reflectapi::Output, + )] + struct Issue164CommonUsesX { + root: Issue164XRoot, + } + + async fn common_get( + _s: S, + _: reflectapi::Empty, + _: reflectapi::Empty, + ) -> Issue164CommonUsesX { + unimplemented!() + } + + let (schema, _) = reflectapi::Builder::<()>::new() + .route(common_get, |b| b.name("common.get")) + .rename_types( + "reflectapi_demo::tests::namespace::Issue164NestedLeaf", + "x::common::Leaf", + ) + .rename_types( + "reflectapi_demo::tests::namespace::Issue164XRoot", + "x::Root", + ) + .rename_types( + "reflectapi_demo::tests::namespace::Issue164CommonUsesX", + "common::UsesX", + ) + .build() + .unwrap(); + + let files = reflectapi::codegen::python::generate_files( + schema, + &reflectapi::codegen::python::Config::default(), + ) + .unwrap(); + + let init_py = files.get("__init__.py").unwrap(); + let init_common_import = init_py.find("from . import common").unwrap(); + let init_x_import = init_py.find("from . import x").unwrap(); + assert!( + init_x_import < init_common_import, + "root `x` must import before root `common`; nested `x.common` references must not create a false root `common` dependency:\n{init_py}" + ); + + let client_py = files.get("_client.py").unwrap(); + let client_common_import = client_py.find("from . import common").unwrap(); + let client_x_import = client_py.find("from . import x").unwrap(); + assert!( + client_x_import < client_common_import, + "client imports must also load root `x` before root `common`:\n{client_py}" + ); +} + #[test] fn test_python_split_modules_handles_sys_root_and_sanitized_class_names() { #[derive( diff --git a/reflectapi/src/codegen/python.rs b/reflectapi/src/codegen/python.rs index 0b39d417..fc98dca7 100644 --- a/reflectapi/src/codegen/python.rs +++ b/reflectapi/src/codegen/python.rs @@ -2342,6 +2342,21 @@ fn contains_qualified_name(code: &str, name: &str) -> bool { }) } +fn contains_bare_qualified_name(code: &str, name: &str) -> bool { + let needle = format!("{name}."); + code.match_indices(&needle).any(|(index, _)| { + let previous = code[..index].chars().next_back(); + let before_is_boundary = + previous.is_none_or(|c| !(c.is_ascii_alphanumeric() || c == '_' || c == '.')); + let after_index = index + needle.len(); + let after_is_boundary = code[after_index..] + .chars() + .next() + .is_none_or(|c| c.is_ascii_alphabetic() || c == '_'); + before_is_boundary && after_is_boundary + }) +} + /// Strip the current module's `.` from references to its own types so /// they read as bare local names. A colliding leaf is instead rewritten to its /// disambiguated flat name (passed in `collisions`) rather than the bare leaf, @@ -2448,7 +2463,12 @@ fn ordered_child_modules( let mut sibling_path = ns_path.to_vec(); sibling_path.push(sibling_name.clone()); let sibling_qualified_name = module_qualified_name(&sibling_path); - if contains_qualified_name(&rendered_code, &sibling_qualified_name) { + let references_sibling = if ns_path.is_empty() { + contains_bare_qualified_name(&rendered_code, &sibling_qualified_name) + } else { + contains_qualified_name(&rendered_code, &sibling_qualified_name) + }; + if references_sibling { deps.entry(child_name.clone()) .or_default() .push(sibling_name.clone()); From 509b29684fb3b06d4478fff5492309204d814f60 Mon Sep 17 00:00:00 2001 From: Brian Thorne Date: Wed, 3 Jun 2026 13:47:45 +1200 Subject: [PATCH 3/3] Ignore Python prose in import dependency scan --- reflectapi-demo/src/tests/namespace.rs | 1 + reflectapi/src/codegen/python.rs | 62 +++++++++++++++++++++++++- 2 files changed, 62 insertions(+), 1 deletion(-) diff --git a/reflectapi-demo/src/tests/namespace.rs b/reflectapi-demo/src/tests/namespace.rs index 83c37fb6..7a908c69 100644 --- a/reflectapi-demo/src/tests/namespace.rs +++ b/reflectapi-demo/src/tests/namespace.rs @@ -722,6 +722,7 @@ fn test_python_split_modules_root_sibling_order_ignores_nested_suffixes() { #[derive( Debug, serde::Serialize, serde::Deserialize, reflectapi::Input, reflectapi::Output, )] + /// This prose mentions common.UsesX but must not affect import ordering. struct Issue164XRoot { leaf: Issue164NestedLeaf, } diff --git a/reflectapi/src/codegen/python.rs b/reflectapi/src/codegen/python.rs index fc98dca7..9100294d 100644 --- a/reflectapi/src/codegen/python.rs +++ b/reflectapi/src/codegen/python.rs @@ -2357,6 +2357,66 @@ fn contains_bare_qualified_name(code: &str, name: &str) -> bool { }) } +fn strip_python_comments_and_strings(code: &str) -> String { + let chars: Vec = code.chars().collect(); + let mut stripped = String::with_capacity(code.len()); + let mut index = 0; + + while index < chars.len() { + let ch = chars[index]; + if ch == '#' { + stripped.push(' '); + index += 1; + while index < chars.len() && chars[index] != '\n' { + stripped.push(' '); + index += 1; + } + } else if ch == '\'' || ch == '"' { + let quote = ch; + let is_triple = + index + 2 < chars.len() && chars[index + 1] == quote && chars[index + 2] == quote; + if is_triple { + stripped.push_str(" "); + index += 3; + while index < chars.len() { + if index + 2 < chars.len() + && chars[index] == quote + && chars[index + 1] == quote + && chars[index + 2] == quote + { + stripped.push_str(" "); + index += 3; + break; + } + stripped.push(if chars[index] == '\n' { '\n' } else { ' ' }); + index += 1; + } + } else { + stripped.push(' '); + index += 1; + let mut escaped = false; + while index < chars.len() { + let current = chars[index]; + stripped.push(if current == '\n' { '\n' } else { ' ' }); + index += 1; + if escaped { + escaped = false; + } else if current == '\\' { + escaped = true; + } else if current == quote { + break; + } + } + } + } else { + stripped.push(ch); + index += 1; + } + } + + stripped +} + /// Strip the current module's `.` from references to its own types so /// they read as bare local names. A colliding leaf is instead rewritten to its /// disambiguated flat name (passed in `collisions`) rather than the bare leaf, @@ -2455,7 +2515,7 @@ fn ordered_child_modules( let mut deps: HashMap> = HashMap::new(); for (child, child_name) in &children { - let rendered_code = module_subtree_rendered_code(child); + let rendered_code = strip_python_comments_and_strings(&module_subtree_rendered_code(child)); for sibling_name in &child_names { if sibling_name == child_name { continue;