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

[Draft] Don't include PhantomData fields in IdlBuild impl #3433

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ssadler
Copy link

@ssadler ssadler commented Dec 17, 2024

Hi, I made this small change which addresses #3241 in order to be able to use a PhantomData field in an account.

It gets the IDL to build by filtering out PhantomData fields, however i have not tested it the other way, i.e., what happens if you want to encode a struct that includes a PhantomData field from a JS input.

Would like to know if this PR is acceptable as is, or what other changes need to be made also? For example:

  • Test that IDL definition does not include PhantomData fields
  • Signed Git commit
  • Some comments would be nice

Best

Copy link

vercel bot commented Dec 17, 2024

@ssadler is attempting to deploy a commit to the coral-xyz Team on Vercel.

A member of the Team first needs to authorize it.

@ssadler ssadler changed the title Don't include PhantomData fields in IDL structs [Draft] Don't include PhantomData fields in IDL structs Dec 17, 2024
@acheroncrypto acheroncrypto added idl related to the IDL, either program or client side feature labels Dec 21, 2024
Copy link
Collaborator

@acheroncrypto acheroncrypto left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to work on this!

I think we also need to remove the generics that are only used by the PhantomData to make the definition valid. Otherwise, using declare_program! with this IDL would result in a compile error because Rust doesn't allow unused generic parameters.

Would like to know if this PR is acceptable as is, or what other changes need to be made also?

It's helpful and has minimal changes, so yes, it's certainly acceptable. However, it would be great to have some tests for this in here. Tests should cover compilation of a definition with:

  • Existing generic parameters (other than the one used by the PhantomData field)
  • No existing generic parameters

The generated IDL should also be checked to make sure:

  • The PhantomData fields don't exist
  • Generic parameter definitions are correctly omitted.

Comment on lines +433 to +435
syn::Type::Path(path) if the_only_segment_is(path, "PhantomData") => {
Ok((TokenStream::new(), vec![]))
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: match arms more or less follow the order of IdlType, so it would be great if you could move this logic either inside the Defined impl:

// Defined
syn::Type::Path(path) => {

or just above it.

@ssadler ssadler changed the title [Draft] Don't include PhantomData fields in IDL structs [Draft] Don't include PhantomData fields in IdlBuild impl Dec 21, 2024
@ssadler
Copy link
Author

ssadler commented Dec 21, 2024

I think we also need to remove the generics that are only used by the PhantomData to make the definition valid. Otherwise, using declare_program! with this IDL would result in a compile error because Rust doesn't allow unused generic parameters.

How would I produce this error? The change is Works On My Machine(TM), because though the account struct has a generic param, it's only the impl that omits encoding the PhantomData field and rust allows that. Updated title.

@acheroncrypto
Copy link
Collaborator

I was going to push a minimal reproduction to this branch, but I was unable to do so because of this branch belonging to an organization fork. However, it's quite easy to reproduce; you can simply define a type that uses PhantomData:

#[derive(AnchorSerialize, AnchorDeserialize, Clone)]
pub struct PhantomStruct<T> {
    pub field: u32,
    _phantom: PhantomData<T>,
}

and use that type as a parameter to an instruction:

// Compilation test for whether `PhantomData` can be used with IDL
pub fn test_compilation_phantom_data(
    _ctx: Context<TestCompilation>,
    _phantom_struct: PhantomStruct<u8>,
) -> Result<()> {
    Ok(())
}

in this program.

Trying to build declare-program after updating external's IDL results in:

error[E0392]: type parameter `T` is never used
 --> programs/declare-program/src/lib.rs:5:1
  |
5 | declare_program!(external);
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^ unused type parameter
  |
  = help: consider removing `T`, referring to it in a field, or using a marker such as `PhantomData`
  = help: if you intended `T` to be a const parameter, use `const T: /* Type */` instead
  = note: this error originates in the macro `declare_program` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0282]: type annotations needed
 --> programs/declare-program/src/lib.rs:5:1
  |
5 | declare_program!(external);
  | ^ cannot infer type
  |
  = note: this error originates in the derive macro `Clone` which comes from the expansion of the macro `declare_program` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0282]: type annotations needed
 --> programs/declare-program/src/lib.rs:5:1
  |
5 | declare_program!(external);
  | ^ cannot infer type
  |
  = note: this error originates in the derive macro `Default` which comes from the expansion of the macro `declare_program` (in Nightly builds, run with -Z macro-backtrace for more info)

Some errors have detailed explanations: E0282, E0392.
For more information about an error, try `rustc --explain E0282`.
error: could not compile `declare-program` (lib) due to 3 previous errors

@ssadler
Copy link
Author

ssadler commented Dec 29, 2024

Ah, yes, I agree. I mentioned that possibility in my original comment. That still won't work. Let me know if it's acceptable to fix only the impls?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature idl related to the IDL, either program or client side
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants