-
Notifications
You must be signed in to change notification settings - Fork 86
Merge codegen fixes #379
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merge codegen fixes #379
Conversation
Right now this regenerates rustdoc for crux_ crates for every run.. Needs some optimizations but it works |
@StuartHarris should be ready.. Do you want me to update all the examples to use new typegen? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @fraschm1998 thank you ❤️ for your work on this so far. I'd love to incorporate some (if not all) of these changes, but had a few discussion points that we should align on first.
crux_cli/src/codegen/mod.rs
Outdated
return true; | ||
} | ||
|
||
// Workspace members are handled separately |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I understand why workspace crates are handled separately. Could you explain to me a bit why they are not treated like any other "foreign" crate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Workspace crates get special treatment because they represent your application's API surface vs external dependencies so they have different filtering parameters.
Workspace crates
// Line 190 in mod.rs - ALL public types become roots
filter.add_all_public_types_as_roots(&workspace_crate);
- Generate ALL public types - comprehensive API exposure
- Processed first in the pipeline
- You control these types and designed them for FFI
External crates (dependencies):
// Only types actually referenced get included
relation external_type_needed(SummaryNode);
external_type_needed(t) <--
remote_type_of(_, t), // Only if referenced
summary(t);
- Generate only referenced types - minimal inclusion
- Processed after workspace crates
- These types weren't necessarily designed for FFI
Practical Example:
If your workspace has:
- shared crate with 50 public types
- serde dependency with 200 types
Result:
- All 50 shared types are available in generated Java/Swift/TypeScript
- Only the 3 serde types you actually use get generated
- Prevents generating 197 unused serde types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @fraschm1998 this makes sense to me now. I was originally going with the concept of only following types referenced from types that implement the App
trait, and its associated Event
s and Effect
s (and ViewModel
). This has the upside that we don''t generate foreign types that we don't need (but are maybe accidentally public), but also has the downside that we don't generate foreign types that we may need (say for additional, non-crux, FFI support). @charypar not sure what you think here? — should we generate foreign types for our whole public API?
crux_cli/src/codegen/filter.rs
Outdated
use super::node::{CrateNode, ItemNode, SummaryNode}; | ||
|
||
/// Standard library crate names that should be skipped during type generation | ||
pub const STD_CRATES: &[&str] = &["std", "core", "alloc", "proc_macro", "test"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we already merged something like this. But I'm not quite sure why we need to exclude standard library crates. They weren't causing me a problem before. There is a test somewhere which points to the location of the json for the core libs, but it's something like share/doc/rust/json/
directory of the rustup toolchain.
I can imagine that there are serializable types in the core libs that we might want across the bridge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're being generated because we first generate all external types referenced in the type graph, then filter out what shouldn't become custom types. Without the STD_CRATES filter, std::String gets included in the initial collection and then treated as a custom type that conflicts with kotlin types.
- Type Discovery Phase: external_type_needed relation (filter.rs:78-82) identifies ALL external types referenced in dependencies
- Synthetic Type Creation: add_workspace_external_types() (filter.rs:243+) creates synthetic container types for these external references
- Code Generation: These synthetic types get passed to serde-generate which creates custom classes
Without filtering, std::String flows through this pipeline and becomes com.example.project.shared.String instead of mapping to kotlin.String. Without this I was getting hundreds of errors.
I can remove the constant and instead have a maps_to_primitive_format function like:
fn maps_to_primitive_format(type_name: &str) -> bool {
matches!(
type_name,
"String" | "Vec" | "HashMap" | "BTreeMap" | "Option" | "Box" | "Arc" | "Rc"
)
}
But then it would miss out on filtering:
However, you're right that there could be legitimate serializable types in core libs. The trade-off is:
- Broad filtering (STD_CRATES): Safer, prevents primitive conflicts, but might miss legitimate std types
- Targeted filtering (primitive list): More flexible, but requires maintaining the list of problematic types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't yet encountered any issues with the constant filtering.. lmk if you want me to switch it to the maps_to_primitive_format
911141b
to
547e355
Compare
547e355
to
97f9b83
Compare
97f9b83
to
721043a
Compare
Fixes
Error: unknown crate std
andError: unknown crate alloc
errorsAdds type alias support from #375
Adds external crate support