Skip to content

Conversation

@caspark
Copy link
Contributor

@caspark caspark commented Apr 4, 2025

The various Symbols' serde implementations had a bug that would cause their underlying index to increase by 1 each time they're serialized and deserialized, because the serialize impl didn't account for the fact that Symbol::new() adds one to guarantee that the resulting index is non-zero.

This fixes that and adds tests for it.

  • A less obtuse fix would be to move the + 1 operation out of Symbol::new() and into the deserialize impl for each Symbol, but I'm not sure if something else in this codebase relies on Symbol::new() adding 1.
  • Wasn't sure whether you want tests in serde_impl.rs or in the existing integration test binary; I opted for the former because it means the tests will neatly be automatically disabled if the serde feature is off.

Fixes #88 .

@codecov-commenter
Copy link

codecov-commenter commented Apr 4, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.34%. Comparing base (e92b6b6) to head (98b2451).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #91      +/-   ##
==========================================
+ Coverage   60.60%   61.34%   +0.74%     
==========================================
  Files          11       11              
  Lines         500      507       +7     
==========================================
+ Hits          303      311       +8     
+ Misses        197      196       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Owner

@Robbepop Robbepop 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 the intended fixes. I have reviewed the changes. Please fix the nits I have. Ideally we could even make the Symbol (de)serialization more generic by implementing (De)Serialize for all T: Symbol.

@caspark caspark force-pushed the fix-symbol-serde-roundtripping branch from f4efa20 to 656e3d6 Compare April 4, 2025 13:30
@caspark
Copy link
Contributor Author

caspark commented Apr 4, 2025

Feedback should be addressed now. Swapped to using from_usize() in the Deserialize impl too, to clearly mirror the Serialize impl.

We can't impl serde traits for all T: Symbol unfortunately because of the orphan rule: it would cause overlapping trait impls for those traits - e.g. usize would have our impls and serde's impls.

@caspark caspark force-pushed the fix-symbol-serde-roundtripping branch from 656e3d6 to 98b2451 Compare April 4, 2025 14:13
@caspark
Copy link
Contributor Author

caspark commented Apr 4, 2025

Latest feedback applied!

Copy link
Owner

@Robbepop Robbepop left a comment

Choose a reason for hiding this comment

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

@caspark Thanks a lot for this!

@Robbepop Robbepop merged commit 56a198c into Robbepop:master Apr 4, 2025
15 of 16 checks passed
@caspark
Copy link
Contributor Author

caspark commented Apr 4, 2025

No worries, thanks for the quick reviews!

@caspark caspark deleted the fix-symbol-serde-roundtripping branch April 4, 2025 14:45
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.

Symbols deserialize incorrectly.

3 participants