Remove fixes.tsv serialization from NullAway serialization service #1063
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1063 +/- ##
============================================
+ Coverage 87.66% 87.86% +0.20%
+ Complexity 2222 2211 -11
============================================
Files 86 85 -1
Lines 7271 7188 -83
Branches 1436 1425 -11
============================================
- Hits 6374 6316 -58
+ Misses 456 438 -18
+ Partials 441 434 -7 ☔ View full report in Codecov by Sentry. |
…imipour/NullAway into nimak/remove-fixes-serialization
| * serialize the fix separately. | ||
| * </ul> | ||
| */ | ||
| public class SerializationV4Adapter extends SerializationV3Adapter { |
There was a problem hiding this comment.
Can you say more on why we need to bump the serialization version? I feel like annotator will still work even with V3 serialized state so there is no compatibility issue? No big harm here, just want to understand
There was a problem hiding this comment.
I removed version 4, as I initially thought any serialization change would require a version update. However, Annotator works perfectly fine with version 3, and actually, updating to version 4 causes it to crash with an "unsupported version" error, which would require an update to Annotator. Overall, I believe this update is unnecessary.
This PR prepares for #1063 by moving the `initializeAdapter` declaration from `FixSerializationConfig` to `SerializationVersionAdapter`. This change ensures that version-specific serializations are handled directly by `SerializationVersionAdapter`, which will simplify test modifications in #1063. This transition also aligns with a clearer design, as `SerializationVersionAdapter` should be responsible for computing the appropriate adapter for each new version. This approach will help us maintain version updates within `SerializationVersionAdapter` while keeping `FixSerializationConfig` unchanged. I'm uncertain if maintaining backward compatibility is necessary in the long term. If it's not needed, we can consider removing it in the future. For now, however, this PR is required to keep #1063 concise.
|
@nimakarimipour please let me know when this is ready for another review |
|
@msridhar I just had an end to end test with this version of NullAway and Annotator and could produce the exact same result with NullAway version |
|
@msridhar One more note: I think the package name is currently |
I think it's better to leave as is? To make clear this is serializing state related to fixing of NullAway errors? |
This PR removes the now-redundant serialization of fixes.tsv following updates to errors.tsv (#643). Previously,
fixes.tsvrecorded the locations of all non-null variables that participated in pseudo-assignments from nullable to non-null types, triggering NullAway errors. However, there was no direct link between these non-null locations and the corresponding errors. #643 addressed this by enhancing error serialization to include the relevant non-null location with each reported error. As a result,fixes.tsvis no longer needed, and NullAwayAnnotator now relies solely onerrors.tsv.This PR removes all serialization related to
fixes.tsvand updates unit test expected output accordingly. Each unit test was adjusted to verify serialized errors, preserving their value and functionality.