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

Rust trait support in the AST/HIR/macro, and codegen for C #621

Merged
merged 70 commits into from
Sep 10, 2024

Conversation

emarteca
Copy link
Contributor

@emarteca emarteca commented Aug 7, 2024

This is built on top of the WIP pull request for callback support (#552).

It is a manual example of a C representation of a Rust trait TestingTrait, that can be passed to a Rust function expecting an object implementing this trait.

The files to look at are example/simple_testing/src/lib.rs for the Rust code (including what we think should be generated), and example/simple_testing/main.c for the C code (including what we think should be generated).

We've written our design out in this linked doc. Feedback welcome! :-)

Copy link
Contributor

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

From the HIR design I think types and traits should be treated as completely distinct beings, with separate lookups.

It's fine for the lookup to overlap in the AST since it's useful to be able to have the error "you typed Trait, did you mean impl Trait" during lowering, but otherwise I don't think traits should have TypeIds or live under TypeDef/etc

core/src/ast/modules.rs Outdated Show resolved Hide resolved
core/src/ast/traits.rs Outdated Show resolved Hide resolved
core/src/ast/types.rs Outdated Show resolved Hide resolved
core/src/ast/types.rs Outdated Show resolved Hide resolved
core/src/ast/types.rs Outdated Show resolved Hide resolved
core/src/hir/defs.rs Outdated Show resolved Hide resolved
core/src/hir/lowering.rs Outdated Show resolved Hide resolved
core/src/ast/traits.rs Outdated Show resolved Hide resolved
core/src/hir/lowering.rs Outdated Show resolved Hide resolved
core/src/hir/type_context.rs Outdated Show resolved Hide resolved
@emarteca
Copy link
Contributor Author

From the HIR design I think types and traits should be treated as completely distinct beings, with separate lookups.

It's fine for the lookup to overlap in the AST since it's useful to be able to have the error "you typed Trait, did you mean impl Trait" during lowering, but otherwise I don't think traits should have TypeIds or live under TypeDef/etc

Sounds good, that makes sense! I just pushed a refactor that splits traits out from this infra in the HIR -- we added PathTrait (instead of reusing PathType for traits), and TraitDef and TraitId are no longer part of the TypeDef and TypeId enums (resp.)

core/src/ast/types.rs Outdated Show resolved Hide resolved
core/src/hir/types.rs Outdated Show resolved Hide resolved
example/simple_testing/Cargo.toml Outdated Show resolved Hide resolved
@emarteca emarteca changed the title Manual example of Rust trait support in Diplomat (for C) Rust trait support in the AST/HIR/macro, and codegen for C Aug 22, 2024
@emarteca
Copy link
Contributor Author

emarteca commented Aug 26, 2024

Force-pushing to rebase onto main (now including the callbacks code that's in main)

@emarteca
Copy link
Contributor Author

Macro and C codegen are up -- tomorrow I'll add it to the proper code infrastructure and fix the lints errors/etc

@emarteca
Copy link
Contributor Author

Force pushing to rebase with main again

@emarteca emarteca marked this pull request as ready for review August 28, 2024 18:52
@emarteca
Copy link
Contributor Author

Macro and C codegen are up and tested, and all the github action tests are passing -- I think this code is ready for review!

core/src/ast/lifetimes.rs Outdated Show resolved Hide resolved
core/src/ast/lifetimes.rs Show resolved Hide resolved
core/src/ast/lifetimes.rs Outdated Show resolved Hide resolved
core/src/ast/lifetimes.rs Outdated Show resolved Hide resolved
core/src/ast/traits.rs Outdated Show resolved Hide resolved
macro/src/snapshots/diplomat__tests__traits.snap Outdated Show resolved Hide resolved
macro/src/lib.rs Show resolved Hide resolved
core/src/hir/lowering.rs Outdated Show resolved Hide resolved
core/src/hir/type_context.rs Outdated Show resolved Hide resolved
tool/src/c/formatter.rs Outdated Show resolved Hide resolved
@emarteca
Copy link
Contributor Author

emarteca commented Sep 4, 2024

Addressed code review + rebase with main (hence force push)

unsafe extern "C" fn(*const c_void, diplomat_runtime::DiplomatSlice<u8>) -> i32,
}
#[repr(C)]
pub struct DiplomatTraitStruct_TesterTrait {
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: an annoying thing about this is that someone wishing to use TesterTrait in a different module will have to manually import this type, which we haven't typically needed in the macro. It's tricky to avoid though, the main thing I can think of is scouring the imports for TesterTrait and tacking on an additional import of the trait struct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah that's a good point. I guess we can add a note about that in the docs; I can't think of how to avoid it

@Manishearth Manishearth merged commit 9dcee14 into rust-diplomat:main Sep 10, 2024
17 checks passed
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.

2 participants