Skip to content

Conversation

@lorisleiva
Copy link
Member

@lorisleiva lorisleiva commented Oct 15, 2024

This PR adds the following TraitOptions configuration object to the Rust renderer allowing users to tweak how traits are rendered in their clients.

export type TraitOptions = {
    /** The default traits to implement for all types. */
    baseDefaults?: string[];
    /**
     * The default traits to implement for data enums only — on top of the base defaults.
     * Data enums are enums with at least one non-unit variant.
     */
    dataEnumDefaults?: string[];
    /**
     * The mapping of feature flags to traits.
     * For each entry, the traits will be rendered within a
     * `#[cfg(feature = "feature_name", derive(Traits))]` attribute.
     */
    featureFlags?: Record<string, string[]>;
    /** The complete trait overrides of specific types. */
    overrides?: Record<string, string[]>;
    /**
     * The default traits to implement for scalar enums only — on top of the base defaults.
     * Scalar enums are enums with no variants or only unit variants.
     */
    scalarEnumDefaults?: string[];
    /** The default traits to implement for structs only — on top of the base defaults. */
    structDefaults?: string[];
    /** Whether or not to use the fully qualified name for traits, instead of importing them. */
    useFullyQualifiedName?: boolean;
};

The default values for these options are:

export const DEFAULT_TRAIT_OPTIONS: Required<TraitOptions> = {
    baseDefaults: [
        'borsh::BorshSerialize',
        'borsh::BorshDeserialize',
        'serde::Serialize',
        'serde::Deserialize',
        'Clone',
        'Debug',
        'Eq',
        'PartialEq',
    ],
    dataEnumDefaults: [],
    featureFlags: { serde: ['serde::Serialize', 'serde::Deserialize'] },
    overrides: {},
    scalarEnumDefaults: ['Copy', 'PartialOrd', 'Hash', 'num_derive::FromPrimitive'],
    structDefaults: [],
    useFullyQualifiedName: false,
};

See readme changes for more details.

@changeset-bot
Copy link

changeset-bot bot commented Oct 15, 2024

🦋 Changeset detected

Latest commit: 91cb4c8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@codama/renderers-rust Patch
@codama/renderers Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

febo
febo previously approved these changes Oct 16, 2024
Copy link
Contributor

@febo febo left a comment

Choose a reason for hiding this comment

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

Looks great! Couple of micro nits on the comments/docs.

@lorisleiva lorisleiva merged commit d4736da into main Oct 16, 2024
3 checks passed
@lorisleiva lorisleiva deleted the loris/rust-trait-overrides branch October 16, 2024 11:52
@github-actions github-actions bot mentioned this pull request Oct 16, 2024
Copy link
Contributor

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

Post-humous review here!

I love the change. I think it's going to make working with the generated types a lot easier and less restrictive. A couple of thoughts from my side:

  • Technically speaking, what these configs are representing are traits that are derived on the types. There's nothing stopping developers from implementing traits on generated types in a file outside of the generated dir. I guess this would just be a naming nit then.
  • Some traits often rely on other traits being implemented on the type as well. for example, bytemuck's Pod requires Copy. I think it's okay to not really worry about this hiccup, since devs can just add the trait necessary and regenerate.
  • Some traits like bytemuck's Pod also require #[repr(C)]. Even without bytemuck, some types may want to use this (like enums). Should we include a config for repr?

Comment on lines +67 to +81
const traitOptions = {
baseDefaults: [
'borsh::BorshSerialize',
'borsh::BorshDeserialize',
'serde::Serialize',
'serde::Deserialize',
'Clone',
'Debug',
'Eq',
'PartialEq',
],
dataEnumDefaults: [],
scalarEnumDefaults: ['Copy', 'PartialOrd', 'Hash', 'num_derive::FromPrimitive'],
structDefaults: [],
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's a case to be made for the default being zero traits and zero scalar enum configurations, but I don't feel too strongly about it.

Copy link
Member Author

Choose a reason for hiding this comment

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

We had to keep them like that in order to avoid introducing breaking changes. This produces code in the exact same way we were before. But I think these are sensible defaults for most clients, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, it doesn't really hurt to just let people change the base defaults anyway, and avoid the breaking change.


### Default traits

Using the `traitOptions` attribute, you may configure the default traits that will be applied to every Rust type. These default traits can be configured using 4 different attributes:
Copy link
Contributor

Choose a reason for hiding this comment

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

micro-micro-nit:

Suggested change
Using the `traitOptions` attribute, you may configure the default traits that will be applied to every Rust type. These default traits can be configured using 4 different attributes:
Using the `traitOptions` attribute, you may configure the default traits that will be applied to every generated Rust type. These default traits can be configured using 4 different attributes:


### Overridden traits

In addition to configure the default traits, you may also override the traits for specific types. This will completely replace the default traits for the given type. To do so, you may use the `overrides` attribute of the `traitOptions` object.
Copy link
Contributor

Choose a reason for hiding this comment

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

Slick, I like it!


### Feature Flags

You may also configure which traits should be rendered under a feature flag by using the `featureFlags` attribute. This attribute is a map where the keys are feature flag names and the values are the traits that should be rendered under that feature flag. Here is an example:
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this even more!

@lorisleiva
Copy link
Member Author

Some traits like bytemuck's Pod also require #[repr(C)]. Even without bytemuck, some types may want to use this (like enums). Should we include a config for repr?

@buffalojoec Would this be a blocker for you right now on some programs? If not, let's wait until more people ask for it. Otherwise, we can brainstorm a new representation option inside this traitOptions object asap.

@buffalojoec
Copy link
Contributor

Some traits like bytemuck's Pod also require #[repr(C)]. Even without bytemuck, some types may want to use this (like enums). Should we include a config for repr?

@buffalojoec Would this be a blocker for you right now on some programs? If not, let's wait until more people ask for it. Otherwise, we can brainstorm a new representation option inside this traitOptions object asap.

Nope, not a blocker for me. I think if anyone tries to use the new trait configs to set up bytemuck traits then they'll hit the repr problem, but we can wait for that yep.

@Dodecahedr0x
Copy link

Hello, I'm trying to use Codama with a pinocchio+bytemuck program and I'm hitting this issue of the generated code not having the #[repr(C)]

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.

6 participants