Skip to content
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

refactor!: move Rust code generation logic from candid_parser. #480

Merged
merged 3 commits into from
Apr 12, 2024

Conversation

lwshang
Copy link
Contributor

@lwshang lwshang commented Apr 11, 2024

SDK-1589

Description

We are going to make ic-cdk-bindgen support more use cases, e.g. generating types only, generating bindings for canister implementation.

The first step is to move the Rust code generation logic from the candid_parser crate so that we can have complete control of Rust codegen.

The added code_generator.rs file was the rust/candid_parser/src/bindings/rust.rs in the candid crate.

The only difference:

diff cdk-rs/src/ic-cdk-bindgen/src/code_generator.rs candid/rust/candid_parser/src/bindings/rust.rs
0a1
> use super::analysis::{chase_actor, infer_rec};
3d3
< use candid_parser::bindings::analysis::{chase_actor, infer_rec};

How Has This Been Tested?

Example tests

Checklist:

  • The title of this PR complies with Conventional Commits.
  • I have edited the CHANGELOG accordingly.
  • I have made corresponding changes to the documentation.

@lwshang lwshang marked this pull request as ready for review April 11, 2024 22:54
@lwshang lwshang requested a review from a team as a code owner April 11, 2024 22:54
@lwshang lwshang enabled auto-merge (squash) April 12, 2024 00:17
@chenyan-dfinity
Copy link
Contributor

How do you make sure the bindgen stays in sync with candid_parser::bindings::rust? candid_parser is currently going through a major revamp, likely the Rust binding module will look completely different soon.

@lwshang
Copy link
Contributor Author

lwshang commented Apr 12, 2024

How do you make sure the bindgen stays in sync with candid_parser::bindings::rust?

There is no need to keep sync. This PR is just setting a starting point for my following rewrite of ic-cdk-bindgen crate. I will soon make it very different from candid_parser::bindings::rust.


Currently, we keep the language bindings in candid_parser crate in the candid repo. While some of the language codegen logic was for some particular downstream projects, e.g. cdk-rs. agent-rs, agent-js, etc. This introduced a circular dependency which makes it really hard to maintain both sides.

There were a few cases illustrated why it is not a good idea to have such circular dependency:

Case 1

When we needed to change the generated Rust code in ic-cdk-bindgen. We had to go through following steps:

  1. Open a PR in the candid repo which change the codegen logic.
  2. Open a cdk-rs PR which updated the candid dependency (with a temporary git commit ref) to see if the change is as expected.
  3. Merge the candid PR and make a new release of candid_parser.
  4. Change the cdk-rs PR to use the new release of candid_parser and merge the PR.

Case 2

When there was a change in the candid_parser crate that accidentally altered the Rust codegen, a patch release was made. However, this change was blindly passed to the ic-cdk-bindgen, causing it to break. To fix the issue, we had to go through the cumbersome steps in case 1.


I'm envisioning a more clear separation of responsibilities which will make the projects more cohesive and reduce coupling.

  • The candid repo (candid_parser, didc bind) can still keep some language codegen functionality.
    • But such codegen should be usage agnostic, hence no circular dependency.
  • ic-cdk-bindgen is in charge of bindgen from Candid to Rust only for the usage inside Rust CDK.
    • If we want to change the generated Rust code, we will only need one cdk-rs PR and have proper CI check there.

Copy link
Contributor

@chenyan-dfinity chenyan-dfinity left a comment

Choose a reason for hiding this comment

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

resolved offline

@lwshang lwshang disabled auto-merge April 12, 2024 18:42
@lwshang lwshang enabled auto-merge (squash) April 12, 2024 18:42
@lwshang lwshang merged commit dc582ae into main Apr 12, 2024
18 checks passed
@lwshang lwshang deleted the lwshang/sdk-1589 branch April 12, 2024 18:45
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