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

program-error: Add option to specify solana_program crate #7112

Merged
merged 4 commits into from
Aug 13, 2024

Conversation

joncinque
Copy link
Contributor

Problem

There are going to be a lot of issues with people seeing errors on
spl_program_error derivation. When a build pulls in two version of
solana_program, the macro uses ::solana_program, which now becomes
ambiguous.

Summary of changes

I'm not sure if this is a good idea, but this PR gives the ability
to specify which version of solana_program to use when deriving the
various traits on your error type.

Borsh has this same functionality, and it's saved us when pulling in
multiple versions of borsh in the SDK.

Note: this PR defaults to solana_program instead of
::solana_program, which might cause downstream issues.

Problem

There are going to be a lot of issues with people seeing errors on
spl_program_error derivation. When a build pulls in two version of
solana_program, the macro uses `::solana_program`, which now becomes
ambiguous.

Summary of changes

I'm not sure if this is a good idea, but this PR gives the ability
to specify which version of `solana_program` to use when deriving the
various traits on your error type.

Borsh has this same functionality, and it's saved us when pulling in
multiple versions of borsh in the SDK.

Note: this PR defaults to `solana_program` instead of
`::solana_program`, which might cause downstream issues.
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.

Ahh clever, this could be useful. num-derive also lets you do this with num-traits, but it looks like it defaults to expecting the dependency to be in the crate's manifest.

https://github.com/rust-num/num-derive/blob/7cc33515dd2ae0eb43c5795c50ce49c554e8ba02/src/lib.rs#L152

However, they make use of a clever little trick - documented as being plucked from serde - to register the user's import as a local _num_traits module.

https://github.com/rust-num/num-derive/blob/7cc33515dd2ae0eb43c5795c50ce49c554e8ba02/src/lib.rs#L184-L191

Maybe we can do it this way for the default? I think that should avoid the ambiguous glob problem from #6590. What do you think?

@buffalojoec
Copy link
Contributor

Additionally, now that I'm looking through the num-traits macro attribute, we could also maybe offer that as a passthrough arg so that we're not blocking access to the #[num-traits] attribute for downstream users, since we're leveraging it for each derive.

@urb82

This comment was marked as spam.

@joncinque
Copy link
Contributor Author

Thanks for the feedback! Let me know how this looks.

Additionally, now that I'm looking through the num-traits macro attribute, we could also maybe offer that as a passthrough arg so that we're not blocking access to the #[num-traits] attribute for downstream users, since we're leveraging it for each derive.

Definitely makes sense, but I'd prefer to keep that for future work if that's ok with you

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.

This looks great! I think this will help with dependencies a lot. Thanks!

@joncinque joncinque merged commit 4c84992 into solana-labs:master Aug 13, 2024
8 checks passed
@joncinque joncinque deleted the progerrorspec branch August 13, 2024 12:40
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