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

Prevent move-out on pointer holding structs #475

Merged
merged 2 commits into from
Aug 30, 2024
Merged

Prevent move-out on pointer holding structs #475

merged 2 commits into from
Aug 30, 2024

Conversation

R1kM
Copy link
Collaborator

@R1kM R1kM commented Aug 30, 2024

As described in the title. There currently is some special-casing to avoid move-out when, e.g., accessing an array of non-Copy elements.
This PR extends what is considered as a non-Copy element to also consider pointer holding structs.

@msprotz
Copy link
Contributor

msprotz commented Aug 30, 2024

I take it pointer_holding_structs only contains mutable pointers? I guess we don't know at that stage so we have to be conservative...

@msprotz msprotz merged commit c96fb69 into master Aug 30, 2024
2 checks passed
@msprotz msprotz deleted the afromher_rust branch August 30, 2024 15:11
@R1kM
Copy link
Collaborator Author

R1kM commented Aug 30, 2024

Actually, since we do not derive the Copy or Clone traits for structures, the check should probably be more restrictive, and check for any struct type.

For instance, despite using a struct that does not contain pointers, the following Rust code does not compile

struct T {
    x: u32,
    y: u32
}

fn g(s: &[T]) {
    let x: T = s[0];
}

Coming back to a simplified version of the HACL-rs example that inspired this PR, the following code would indeed work without the additional reference added by karamel, while taking a mutable pointer in struct T would lead to a compilation failure.

#[derive(Copy, Clone)]
struct T <'a> {
    x: u32,
    y: &'a [u32]
}

fn g(s: &[T]) {
    let x: T = s[0];
}

However, adding support for Clone/Copy on generated struct types does not seem needed at the moment, and this conservative pattern is sufficient for compilation. I would suggest to revisit only if needed.

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