fix(dart): use BigInt for proper dart web truncation#3600
fix(dart): use BigInt for proper dart web truncation#3600ayush00git wants to merge 6 commits intoapache:mainfrom
Conversation
|
Could you add a test to cover it, and is there any code example to reproduce it? |
@chaokunyang this may reproduce the bug and also show the verification of the fix void main() {
const value = 3000000000; // positive, fits in JS safe-integer range
// Old (buggy) web fallback — value >> 63 is evaluated as a 32-bit int:
// 3000000000 as int32 == -1294967296 (bit 31 set → negative)
// -1294967296 >> 63 == -1 (fills with sign bit)
final buggyZigZag = (BigInt.from(value) << 1) ^ BigInt.from(value >> 63);
print(buggyZigZag); // Web: large negative number — CORRUPT
// Native: 6000000000 — correct (native >> is 64-bit)
// Fixed path — shift the BigInt, no truncation:
final signed = BigInt.from(value);
final fixedZigZag = (signed << 1) ^ (signed >> 63);
print(fixedZigZag); // Always: 6000000000 — correct on both platforms
} |
|
The tests won't run actually, coudl you add |
my bad, let me update ci.yml |
|
@chaokunyang the new ci script exposed more failing tests in the |
No, that still doesn't fory dart on web work. It's just avoid real issue. |
@chaokunyang making |
|
for now we could add @TestOn('vm')
library;for just passing the ci checks ? or should I fix everything in this PR only ? |
|
Need a full fix, but that's a big work, and break existing fory dart object model. |
so how should I proceed ? Should I propose a proper fix in a github issue first ? and then begin with the PR? |
|
Such fix is not useful, the dart web support is a big work. Either support it fully, or don't support it. The dart int will be compiled to javascript numer, which is double, int64 can't be expressed using double. and if we use bigint, it's too slow and we paid cost for dart native ,which is unnecessary. So, we should not add any half dart web fix until we have a good design that can address both issues |
fair enough, so let's keep dart web support as a future feature and you can close this PR then. as i'm free i'll look into a design which could address both these issues. |
|
@chaokunyang we surely need a good design, the UnsupportedError: Uint64 accessor not supported by dart2js.do we have any plans to implement dart web in future? |
## Why? Dart web builds use JavaScript number semantics, so raw Dart `int` cannot safely represent the full 64-bit value space used by Fory xlang integer encodings. The Dart runtime needs browser-compatible buffer, generated serializer, and 64-bit wrapper implementations that preserve wire compatibility instead of silently corrupting JS-unsafe values. ## What does this PR do? - Adds Dart web support through conditional native/web implementations for `Buffer`, generated cursors, and `Int64` / `Uint64` value and list wrappers. - Updates code generation and serializers to handle full-range signed and unsigned 64-bit fields on web, while keeping Dart `int` fast paths for JS-safe values. - Adds explicit range checks so web builds reject JS-unsafe Dart `int` conversions instead of writing or reading imprecise bytes. - Extends Dart serializer, buffer, generated-field, numeric-wrapper, typed-array, and time tests for 64-bit edge cases and browser behavior. - Adds Chrome test coverage to CI with `dart test -p chrome` for the Dart package. - Documents Dart web support, 64-bit integer rules, custom serializer guidance, and troubleshooting steps. ## Related issues Closes apache#3600 ## AI Contribution Checklist - [ ] Substantial AI assistance was used in this PR: `yes` / `no` - [ ] If `yes`, I included a completed [AI Contribution Checklist](https://github.com/apache/fory/blob/main/AI_POLICY.md#9-contributor-checklist-for-ai-assisted-prs) in this PR description and the required `AI Usage Disclosure`. - [ ] If `yes`, my PR description includes the required `ai_review` summary and screenshot evidence of the final clean AI review results from both fresh reviewers on the current PR diff or current HEAD after the latest code changes. ## Does this PR introduce any user-facing change? Yes. Dart users can now use generated serializers and manual serializers on web/browser targets. Full-range 64-bit values should use `Int64` or `Uint64`; Dart `int` fields remain limited to JS-safe values on web. - [x] Does this PR introduce any public API change? - [ ] Does this PR introduce any binary protocol compatibility change? ## Benchmark <img width="4200" height="900" alt="throughput" src="https://github.com/user-attachments/assets/ca422bc6-5345-4560-a35a-2678f4c54b9c" /> | Datatype | Operation | Fory TPS | Protobuf TPS | Fastest | | ---------------- | ----------- | --------: | -----------: | ------------ | | Struct | Serialize | 5,041,693 | 2,073,839 | fory (2.43x) | | Struct | Deserialize | 6,395,290 | 4,991,881 | fory (1.28x) | | Sample | Serialize | 1,783,688 | 552,140 | fory (3.23x) | | Sample | Deserialize | 2,124,197 | 934,794 | fory (2.27x) | | MediaContent | Serialize | 952,498 | 438,419 | fory (2.17x) | | MediaContent | Deserialize | 1,649,039 | 737,340 | fory (2.24x) | | StructList | Serialize | 1,945,119 | 399,007 | fory (4.87x) | | StructList | Deserialize | 2,119,403 | 764,832 | fory (2.77x) | | SampleList | Serialize | 475,413 | 52,512 | fory (9.05x) | | SampleList | Deserialize | 508,939 | 116,236 | fory (4.38x) | | MediaContentList | Serialize | 224,925 | 84,860 | fory (2.65x) | | MediaContentList | Deserialize | 387,070 | 154,392 | fory (2.51x) |
Why?
In the web fallback path (
_useBigIntVarint64 == true), the code calculates the Zig-Zag encoding mask usingBigInt.from(value >> 63). On Dart Web, standard bitwise operators (>>) truncate 64-bit Dart integers to 32 bits before shifting. Therefore, if a positive 64-bit number (fitting within the 53-bit safe integer range) has the 31st bit set (e.g., 3,000,000,000), it is treated as a negative 32-bit integer. value >> 63 evaluates to -1 instead of 0. This XORs the signed << 1 value with all 1s, completely inverting the zig-zag value and encoding a positive number as a negative varint.What does this PR do?
Use the BigInt shift operator instead, which does not truncate, or a simple zero-check.
Related issues
AI Contribution Checklist
yes/noyes, I included a completed AI Contribution Checklist in this PR description and the requiredAI Usage Disclosure.yes, my PR description includes the requiredai_reviewsummary and screenshot evidence of the final clean AI review results from both fresh reviewers on the current PR diff or current HEAD after the latest code changes.Does this PR introduce any user-facing change?
Benchmark