fix(cli): Restore 64-bit default handling in the proto target - #2409
Open
Jaybhade wants to merge 1 commit into
Open
fix(cli): Restore 64-bit default handling in the proto target#2409Jaybhade wants to merge 1 commit into
Jaybhade wants to merge 1 commit into
Conversation
buildFieldOptions still calls util.longNeq, which was removed in 39bc103. Any proto2 field of a 64-bit type carrying an explicit default therefore makes pbjs -t proto2 / -t proto3 abort with a TypeError, including on tests/data/mapbox/vector_tile.proto. Compare the resolved default against the type default through longToHash instead, which handles the number/Long mix the old helper existed for, so a default equal to the implicit one is dropped just as it already is for 32-bit fields and any other default is emitted.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
pbjs -t proto2(and-t proto3) aborts withTypeError: util.longNeq is not a functionfor any proto2 field of a 64-bit type that carries an explicitdefault:buildFieldOptionswas added in 4affa1b (Nov 2016) and callsutil.longNeqto decide whether a resolved default is just the type's implicit default and can be dropped. 39bc103 (Jan 2017) removedutil.longNeqas "now unused" — the call site here was missed, and it is the only remaining reference to it in the tree. So the branch has never been reachable without throwing on any released 6.x/7.x/8.x.tests/data/mapbox/vector_tile.proto, already in this repo, is enough to trigger it (optional uint64 id = 1 [ default = 0 ];), so the failure hits a very ordinary real-world schema. This was reported in #809; it was closed as a stale-static-code problem, but @jhherren pointed at this exact line afterwards and it stayed.Fix
Compare the resolved default against the type default with
util.longToHash. The old helper existed because the two sides are not the same shape —field.defaultValueis aLongfor 64-bit fields whiletypes.defaults[field.type]is the number0— andlongToHashnormalises both throughLongBits, so the comparison keeps working for signed, unsigned and negative values.Behaviour after the fix matches what 32-bit fields already do: a default equal to the implicit default is omitted, anything else is emitted.
Tests
Added
proto2 64 bit field defaultstotests/cli-proto.js, covering the implicit-default case (dropped), positive and negative 64-bit defaults (emitted) and a 32-bit default alongside them (unchanged). It fails with theTypeErrorabove onmasterand passes with the fix.npm run lintandnpm testare green (3164 assertions, up from 3162).One thing this deliberately does not address: a 64-bit default larger than
Number.MAX_SAFE_INTEGERis already lost before it reaches the target, becauseparse.jsstores option values as JS numbers —[default = 18446744073709551615]comes out as[default=18446744073709552000]. That is a separate problem in the parser rather than in this branch, so I left it alone.