Skip to content

fix(generator): handle indirect recursion via DFS cycle detection#569

Merged
Byron merged 2 commits into
Byron:mainfrom
raushan728:fix/indirect-recursion
Feb 8, 2026
Merged

fix(generator): handle indirect recursion via DFS cycle detection#569
Byron merged 2 commits into
Byron:mainfrom
raushan728:fix/indirect-recursion

Conversation

@raushan728

@raushan728 raushan728 commented Feb 6, 2026

Copy link
Copy Markdown
Contributor
  • Fix: Added DFS-based cycle detection in util.py to identify indirect type cycles (e.g., A -> B -> A).
  • Boxing: Recursive references are now correctly wrapped in Option<Box<T>>.
  • Tests: Added unit tests in util_test.py for cycle detection and type shape verification.
  • Impact: Unblocks slides, chat, analyticsadmin, and dialogflow APIs.

@Byron Byron left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contributing, but please do remove the generated code (the one generated in the traditional sense at least).

@raushan728 raushan728 force-pushed the fix/indirect-recursion branch 2 times, most recently from eb40641 to 7c5a10d Compare February 6, 2026 14:37
@raushan728

Copy link
Copy Markdown
Contributor Author

thanks for the feedback @Byron!

i have Removed all generated code from the PR as requested. plz take a look

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds indirect-recursion detection to the Rust type generator so recursive schema references can be boxed to avoid Rust “infinite size” type errors, and updates the shared API blacklist accordingly.

Changes:

  • Introduces DFS-based reference traversal helpers (get_all_refs, points_to) to detect indirect recursion cycles.
  • Updates $ref handling in to_rust_type_inner() to box references participating in detected cycles.
  • Removes several APIs from etc/api/shared.yaml blacklist (previously excluded due to recursive-type generation failures).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
src/generator/lib/util.py Adds DFS reachability checks and applies them during $ref type generation to break indirect recursion cycles.
etc/api/shared.yaml Unblacklists multiple APIs that were previously excluded due to recursive-type generation issues.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread etc/api/shared.yaml Outdated
Comment thread src/generator/lib/util.py Outdated
Comment thread src/generator/lib/util.py
Comment thread src/generator/lib/util.py

@Byron Byron left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot, I think this is a great improvement!

A few things to change came up, let's resolve them and merge.

Comment thread src/generator/lib/util.py
Comment thread src/generator/lib/util.py
Comment thread src/generator/lib/util.py Outdated
Comment thread src/generator/lib/util.py
Comment thread src/generator/lib/util.py
@raushan728 raushan728 force-pushed the fix/indirect-recursion branch from 7c5a10d to fad825d Compare February 7, 2026 06:12
@raushan728 raushan728 force-pushed the fix/indirect-recursion branch from fad825d to fc4a30f Compare February 7, 2026 06:21
@raushan728

raushan728 commented Feb 7, 2026

Copy link
Copy Markdown
Contributor Author

@Byron everything is updated u can review it now. If this any problems, plz leave a comment.

@Byron

Byron commented Feb 8, 2026

Copy link
Copy Markdown
Owner

Great, thanks a lot!

I am now running make cargo-api ARGS=check locally and will merge if it all passes.

Tasks

  • update all APIs

@Byron Byron merged commit 37b328c into Byron:main Feb 8, 2026
6 checks passed
@raushan728

Copy link
Copy Markdown
Contributor Author

Thanks for merging the PR, @Byron! It was great working on this.

While diving into the generator code, I noticed a couple of other improvements that could be valuable:

  1. Builder Ergonomics: Changing setters to accept impl Into<String>. This would make the library much more idiomatic and easier to use by removing the need for manual .to_string() calls.
  2. Error Handling: Modernizing the google-apis-common crate by adopting thiserror for cleaner and more maintainable error definitions.

Let me know if you’d like me to open Pull Requests for these as well. I'd be happy to contribute further!

@Byron

Byron commented Feb 8, 2026

Copy link
Copy Markdown
Owner

Thanks for offering, it's much appreciated.

  1. I think we should do, even though that convenience will be paid for with larger binaries. But that's fine for now, and there are mitigations that can be implemented if needed.
  2. Modernising errors, yes, please, but not with thiserror. And the motivation should be something graspable, like making it possible to know if the error is permanent or transient.

@Byron

Byron commented Feb 8, 2026

Copy link
Copy Markdown
Owner

Oh, and lastly, if you use AI please give it a Co-authored-by field. I am trying to make AI transparent across my projects. Thanks for considering.

@raushan728

Copy link
Copy Markdown
Contributor Author

Thanks for the feedback, @Byron!

I’m a 2nd-year student and I use AI to help with research, project summaries, and refining my technical discussions. I'll make sure to include the Co-authored-by tag in future commits for transparency.

I'll focus on the ergonomic improvements next without using thiserror. Please let me know if you have any other concerns!

@raushan728

Copy link
Copy Markdown
Contributor Author

Submitted two PRs regarding our discussion. I’ll keep watch for any comments. Thanks, @Byron

@raushan728 raushan728 deleted the fix/indirect-recursion branch February 14, 2026 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants