feat: add interface_id to AST, idl-gen, validation
#1156
Conversation
Summary of ChangesHello @vobradovich, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a fundamental change to the IDL specification by integrating a unique Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully adds interface_id to the IDL's Abstract Syntax Tree (AST), and correctly updates the IDL generator and validation logic. The introduction of the ServiceIdent struct is a good structural improvement. The new logic for deduplicating services by interface_id and the addition of cycle detection for base services are excellent enhancements. I have included a couple of performance-related suggestions for the builder logic to optimize lookups.
| let expos: Result<Vec<_>> = self | ||
| .service_expos | ||
| .into_iter() | ||
| .enumerate() | ||
| .map(|(idx, (route, meta))| { | ||
| let interface_id = meta.interface_id(); | ||
| let Some(service) = services | ||
| .iter() | ||
| .find(|s| s.name.interface_id == Some(interface_id)) | ||
| else { | ||
| return Err(Error::MetaIsInvalid(format!( | ||
| "service `{route}@{interface_id}` not defined" | ||
| ))); | ||
| }; | ||
| let route = if service.name.name == route { | ||
| None | ||
| } else { | ||
| Some(route.to_string()) | ||
| }; | ||
|
|
||
| Ok(ServiceExpo { | ||
| name: service.name.clone(), | ||
| route, | ||
| route_idx: (idx as u8) + 1, | ||
| docs: vec![], | ||
| annotations: vec![], | ||
| }) | ||
| }) | ||
| .collect(); |
There was a problem hiding this comment.
The find operation inside this loop results in a time complexity of O(N*M), where N is the number of exposed services and M is the total number of service definitions. This can be optimized by creating a HashMap of services keyed by their interface_id before iterating. This would reduce the complexity to O(N+M), improving performance for programs with many services.
let services_by_id: std::collections::HashMap<_, _> = services
.iter()
.filter_map(|s| s.name.interface_id.map(|id| (id, s)))
.collect();
let expos: Result<Vec<_>> = self
.service_expos
.into_iter()
.enumerate()
.map(|(idx, (route, meta))| {
let interface_id = meta.interface_id();
let Some(service) = services_by_id.get(&interface_id) else {
return Err(Error::MetaIsInvalid(format!(
"service `{route}@{interface_id}` not defined"
)));
};
let route = if service.name.name == route {
None
} else {
Some(route.to_string())
};
Ok(ServiceExpo {
name: (*service).name.clone(),
route,
route_idx: (idx as u8) + 1,
docs: vec![],
annotations: vec![],
})
})
.collect();| if let Some(service) = services | ||
| .iter() | ||
| .find(|s| s.name.interface_id == Some(interface_id)) | ||
| { |
There was a problem hiding this comment.
The find operation on the services vector inside build_inner is a linear search. Since build_inner is called recursively for each base service, this can lead to quadratic complexity in cases of services with many direct base services (e.g., a service extending many other services). To optimize this, consider using a HashMap<InterfaceId, ServiceUnit> instead of a Vec<ServiceUnit> to store the services. This would make the lookup for an already built service an O(1) operation on average.
No description provided.