Skip to content

Prefer explicit indexing over automatically assigned indices #21

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jun 5, 2025
Merged

Prefer explicit indexing over automatically assigned indices #21

merged 3 commits into from
Jun 5, 2025

Conversation

robin-nitrokey
Copy link
Member

@robin-nitrokey robin-nitrokey commented Jun 1, 2025 •
edited
Loading

This PR adds an index attribute for explicit index assignment and makes the old automatic index assignment opt-in with the auto_index attribute.

This means that this old code

#[derive(DeserializeIndexed, SerializedIndex)]
struct S {
    a: int,
    b: int,
}

now has to be written like this:

#[derive(DeserializeIndexed, SerializedIndex)]
#[serde(auto_index)]
struct S {
    a: int,
    b: int,
}

#[derive(DeserializeIndexed, SerializedIndex)]
struct S {
    #[serde(index = 0)]
    a: int,
    #[serde(index = 1)]
    b: int,
}

Fixes: #17

@robin-nitrokey
Copy link
Member Author

@AlfioEmanueleFresta Would this work for you?

@robin-nitrokey robin-nitrokey mentioned this pull request Jun 1, 2025
Merged
@robin-nitrokey
Copy link
Member Author

Alternatively, we could use #[serde(rename = 1)] to make the migration to serde easier once it supports renaming to integers.

@AlfioEmanueleFresta
Copy link
Contributor

@robin-nitrokey This is great, thank you! TL;DR, both options look good to me.

Whilst not my favourite naming option, I would prefer adopting #[serde(rename = 42)] just in case serde end up supporting this. However, the migration would be trivial, so I'm not sure it's worth the extra effort, or risk of incompatibility/conflicts - if there's any.

sosthene-nitrokey
sosthene-nitrokey approved these changes Jun 3, 2025
View reviewed changes
Copy link
Contributor

@sosthene-nitrokey sosthene-nitrokey left a comment

Choose a reason for hiding this comment

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

Some small nits.

I like using the #[serde(index = ...)] rather than rename since if there's some subtle change with the upstream implementation it forces to pay attention when migrating.

@sosthene-nitrokey
Copy link
Contributor

You'd want this to replace #14 ?

@robin-nitrokey
Copy link
Member Author

You'd want this to replace #14 ?

No. It’s still required for the auto_index case and also useful for the default case with explicit indices.

@sosthene-nitrokey
Copy link
Contributor

Just FYI it seems unlikely that upstream serde will support it: serde-rs/serde#1697

@robin-nitrokey robin-nitrokey mentioned this pull request Jun 4, 2025
Merged
@robin-nitrokey
Require auto_index attribute to enable automatic index assigment
504b48c 
Automatically assigning indices based on the order of the fields is
error prone, for example when used with feature flags, when fields are
skipped or when the fields of the struct are changed or reordered.
With this patch, we require an explicit opt-in to this potentially
problematic behavior.  See also:
     #17
@robin-nitrokey
parse: Move field parsing into separate method
3deb892
@robin-nitrokey
Add index attribute for explicit index assignment
07586db 
Fixes: #17
@robin-nitrokey robin-nitrokey merged commit 07586db into main Jun 5, 2025
8 checks passed
@robin-nitrokey robin-nitrokey deleted the auto branch June 5, 2025 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daringer daringer daringer approved these changes

@sosthene-nitrokey sosthene-nitrokey sosthene-nitrokey approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Consider requiring explicit indices
4 participants
@robin-nitrokey @AlfioEmanueleFresta @sosthene-nitrokey @daringer