Skip to content

Conversation

@cpubot
Copy link
Collaborator

@cpubot cpubot commented Dec 6, 2025

struct_extensions currently uses field.target_resolved() to determine member projection types. This is incorrect for cases where a struct member is annotated with a container type, as target_resolved will return the fully inferred container type, rather than the underlying type.

For example

struct A {
    #[wincode(with = "containers::Pod<_>")]
    signature: [u8; u32]
}

target_resolved() returns containers::Pod<[u8; 32]>, whereas we actually want the [u8; 32] directly.

Note that target_resolved is still the correct type to use for read_<field>, as read should use the type's read implementation directly (and read_<field> implementation is unchanged). But for write_<field> or uninit_<field>_mut, we need to use the destination type (Dst) since they circumvent the SchemaRead::read impl.

The fix here is simple -- just use <field.ty as SchemaRead>::Dst rather than field.target_resolved(). This works for mapped types, container types, and plain built-in types.

Example mapped type

struct A {
    signature: [u8; u32]
}

#[derive(SchemaWrite, SchemaRead)]
#[wincode(from = "A", struct_extensions)]
struct AMapped {
    signature: containers::Pod<[u8; u32]>,
}

Added tests that will fail to compile on current master.

@cpubot cpubot force-pushed the struct-extensions-mapped-type branch from 020c952 to bc0c8f8 Compare December 6, 2025 18:10
.write_c(test.c);
prop_assert!(builder.is_init());
builder.finish();
let init = unsafe { uninit.assume_init() };
Copy link
Collaborator

Choose a reason for hiding this comment

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

could use int_uninit_mut() here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fn test_struct_extensions_with_container() {
#[derive(SchemaWrite, SchemaRead, Debug, PartialEq, Eq, proptest_derive::Arbitrary)]
#[wincode(internal, struct_extensions)]
struct Test {
Copy link
Collaborator

@kskalski kskalski Dec 8, 2025

Choose a reason for hiding this comment

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

since there is a non-trivial SchemaRead lifetime handling, could you add a test that works on struct with lifetimes, say

struct TestRef<'a> {
    a: &'a [u8],
    b: Option<'a u8>
}

it should be possible to initialize it with a (locally scoped) slice, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, 5ed3c6b

kskalski
kskalski previously approved these changes Dec 9, 2025
@kskalski
Copy link
Collaborator

kskalski commented Dec 9, 2025

Interestingly, tests show UB now?

@cpubot
Copy link
Collaborator Author

cpubot commented Dec 9, 2025

Interestingly, tests show UB now?

Yeah, looks like miri didn't like ptr::read before moving self into mem::forget. Fixed with ManuallyDrop 4914cd1

@cpubot cpubot merged commit e0e3c4f into master Dec 9, 2025
2 checks passed
@cpubot cpubot deleted the struct-extensions-mapped-type branch December 9, 2025 11:32
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