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

Made arrays never encode their length #625

Merged
merged 3 commits into from
Mar 30, 2023

Conversation

VictorKoenders
Copy link
Contributor

@VictorKoenders VictorKoenders commented Mar 30, 2023

Closes #613

Turns out serde never encodes a fixed array length with 32 elements or less. If an array is 33 elements or more, it silently falls back to the &[T] implementation (which does encode the length). Our test cases were only tested with large arrays.

Because the config flags write_fixed_array_length and skip_fixed_array_length were based on a wrong assumption on how serde/bincode 1 worked, these flags have been removed. They may be re-added in the future.

…his was based on incorrect assumptions on how serde and bincode 1 works
@VictorKoenders VictorKoenders changed the title Made arrays with 32 elements or less never encode their length Made arrays never encode their length Mar 30, 2023
@codecov
Copy link

codecov bot commented Mar 30, 2023

Codecov Report

Patch coverage: 84.61% and project coverage change: +0.48 🎉

Comparison is base (ebc3b6b) 53.89% compared to head (c9fd18d) 54.37%.

Additional details and impacted files
@@            Coverage Diff             @@
##            trunk     #625      +/-   ##
==========================================
+ Coverage   53.89%   54.37%   +0.48%     
==========================================
  Files          51       51              
  Lines        4457     4454       -3     
==========================================
+ Hits         2402     2422      +20     
+ Misses       2055     2032      -23     
Impacted Files Coverage Δ
src/de/impls.rs 59.77% <ø> (+1.53%) ⬆️
src/enc/impls.rs 88.77% <ø> (-0.12%) ⬇️
src/features/serde/de_borrowed.rs 20.61% <ø> (-0.18%) ⬇️
src/features/serde/de_owned.rs 76.61% <ø> (+1.99%) ⬆️
src/features/serde/mod.rs 30.43% <ø> (-17.40%) ⬇️
src/features/serde/ser.rs 76.72% <ø> (+2.53%) ⬆️
compatibility/src/lib.rs 50.00% <40.00%> (-1.79%) ⬇️
tests/utils.rs 47.36% <52.94%> (+2.13%) ⬆️
src/config.rs 78.94% <87.50%> (-3.67%) ⬇️
compatibility/src/misc.rs 100.00% <100.00%> (ø)
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@VictorKoenders VictorKoenders merged commit b34ee95 into trunk Mar 30, 2023
@VictorKoenders VictorKoenders deleted the fixed_array_inconsistency branch March 30, 2023 13:09
@TommyLike
Copy link

@VictorKoenders hello, I use bincode in my project as below:

 signed.write_all(&bincode::encode_to_vec(
            &sig_struct,
            config::standard()
                .skip_fixed_array_length()
                .with_fixed_int_encoding()
                .with_big_endian(),
        )?)?;

it works well for the version of bincode = "2.0.0-rc.2" but when upgrade into rc.3 I found the skip_fixed_array_length has been removed, so what should I do for now, How to remove the length when serializing?

emk added a commit to faradayio/geocode-csv that referenced this pull request May 11, 2023
We remove the call to `skip_fixed_array_length`, which no longer
exists, and which never apparently did anything:

bincode-org/bincode#625
@VictorKoenders
Copy link
Contributor Author

@TommyLike currently there is no such option. Bincode 1 and 2 should work as you'd expect without the configuration. Is there a particular issue you're running into?

@TommyLike
Copy link

@TommyLike currently there is no such option. Bincode 1 and 2 should work as you'd expect without the configuration. Is there a particular issue you're running into?

No, I realized it works as I expected, thanks!

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.

bincode::encode_to_vec is incompatible with bincode::serde::encode_to_vec in standard configuration
2 participants