search

theseion / Fuel

Fuel, the Smalltalk object serializer
https://theseion.github.io/Fuel
MIT License
26 stars 12 forks source link

Serialization of Classes (and Traits) just stores commentSourcePointer: invalid in other image #270

Closed MarcusDenker closed 1 year ago

MarcusDenker commented 1 year ago

If you look at

FLCreateClassSerializationTest>>testCreateWithComment FLCreateTraitSerializationTest>>testCreateWithComment

then Fuel expects that it saves class comments if you serialize a class (or traits).

But what it does it to store the ClassOrganization, which contains the commentSourcePointer to the .sources or .changes file.

The test is green because we run it in the same image, the pointer would be invalid or point to a random offset in the file if we load in another image (even if the code of the image is identical, the offsets in the .changes are not the same).

I found this as we are (slowly) removing ClassOrganization in Pharo12. One step is to move the commentSourcePointer to ClassDescription, see https://github.com/pharo-project/pharo/pull/13176

The two tests fail there because the commentSourcePointer in Class is not stored by Fuel, but in some way not storing it is better than storing a random pointer...

I would, for pharo12, change the tests for now to test for nil (with a comment to explain why), then we can improve this for Fuel in general slowly.

MarcusDenker commented 1 year ago

(the random pointer is not that bad as the offset is not used to write, just to read. Changing a comment adds an entry to the end of the .changes, which then is ok again for that image). This means this random pointer should not lead to corruption, other then not showing the comment of the serialized class)

theseion commented 1 year ago

Thanks @MarcusDenker!

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will remain open but will probably not come into focus. If you still think this should receive some attention, leave a comment. Thank you for your contributions.

theseion commented 1 year ago

I've implemented proper serialization of class comment and stamp. We might need a way to configure storing those two strings (at least for the comment it could make sense).

theseion commented 1 year ago

In https://github.com/pharo-project/pharo/pull/14132