Skip to content

lexer: fix expression to decode surrogate pairs #3278

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 19, 2021
Merged

lexer: fix expression to decode surrogate pairs #3278

merged 1 commit into from
Oct 19, 2021

Conversation

Cito
Copy link
Member

@Cito Cito commented Sep 28, 2021

When calculating the code point for a surrogate pair, the base code point of 0x10000 must be added, not bitwise ORed. Added some tests for catching this problem.

@Cito Cito force-pushed the fix-decode-surrogate-pair branch from 3bf6197 to b1d1338 Compare September 28, 2021 20:29
@IvanGoncharov
Copy link
Member

@Cito Sorry for the long review time.
Can you please give a link to some document referencing this is the correct way to calculate surrogate pair?
Tried reviewing it a week ago and was stuck at google.com 😞

@IvanGoncharov IvanGoncharov added the PR: bug fix 🐞 requires increase of "patch" version number label Oct 12, 2021
@Cito
Copy link
Member Author

Cito commented Oct 13, 2021

Can you please give a link to some document referencing this is the correct way

Hi @IvanGoncharov - have a look at the C function decode_utf16_pair at the bottom of this page.

It doesn't matter whether you use '|' or '+' to combine the trailing and shifted leading values, because their bit patterns do not overlap. But you can't use '|' to add the 0x10000 offset, because its bit pattern overlaps with the shifted leading value.

@IvanGoncharov IvanGoncharov force-pushed the fix-decode-surrogate-pair branch from 544591d to b1d1338 Compare October 19, 2021 09:40
@IvanGoncharov
Copy link
Member

@Cito Thanks for the PR and explanation.
Looking at the source code, I learned we don't need this function, and we can use standard JS stuff.
But it is beyond the scope of this PR, and I will remove it in a separate PR.
This PR is still helpful since it adds more test and provide context why we added those tests.

@IvanGoncharov IvanGoncharov merged commit 7da9cd3 into graphql:main Oct 19, 2021
IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Oct 19, 2021
@IvanGoncharov
Lexer: use standard JS functions to handle Unicode
34d27e9 
Context graphql#3278
@IvanGoncharov IvanGoncharov mentioned this pull request Oct 19, 2021
Merged
yaacovCR added a commit to yaacovCR/graphql-js that referenced this pull request Nov 1, 2021
@yaacovCR @IvanGoncharov @Cito
Update dependencies (#50)
9e9653f 
* Export OperationTypeNode as value (graphql#3316)

* Convert const "enum-like" maps to TS enums (graphql#3317)

* Deprecate 'ASTKindToNode' (graphql#3318)

* Add message that we only support TS >= 4.1.0 (graphql#3319)

* Update deps (graphql#3320)

* 16.0.0-rc.5

* jsutils: add test for Path functions (graphql#2478)

Co-authored-by: Ivan Goncharov <[email protected]>

* lexer: fix expression to decode surrogate pairs (graphql#3278)

* Lexer: use standard JS functions to handle Unicode (graphql#3322)

* GraphQLError-test: merge check of source with rest of the properties (graphql#3324)

* GraphQLError: fix empty `locations` if error got nodes without locations (graphql#3325)

* GraphQLError: enumerate only spec prescribed properties (graphql#3326)

* GraphQLField: relax default value of TArgs to `any` (graphql#3328)

* 16.0.0-rc.6

* Fix release instructions (graphql#3329)

* use GITHUB_TOKEN (graphql#3330)

* GraphQLError: Add test to check order of fields in JSON.stringify (graphql#3336)

* Fix TS error caused by importing internal files directly (graphql#3339)

* Use default GITHUB_ACTOR if unset (graphql#3331)

Co-authored-by: Ivan Goncharov <[email protected]>

* 16.0.0-rc.7

* version: force proper typing on version literals

Types should be generic and stay the same across different versions
E.g. `preReleaseTag` should be typed `string | null` even if it's a
`null` literal for this particular release

* build-npm: fix type inference during TS declarations build

* version-test: fix validation of `versionInfo.preReleaseTag` (graphql#3345)

* 16.0.0

* checkgit.sh: Added a check for local modifications (graphql#3347)

* GraphQLError-test: text how extensions is inherited from originalError (graphql#3348)

* fix lockfile

* update graphql version

* Remove unused upstream file

* Upgrade deps

* add changeset

Co-authored-by: Ivan Goncharov <[email protected]>
Co-authored-by: Christoph Zwerschke <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
PR: bug fix 🐞 requires increase of "patch" version number
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Cito @IvanGoncharov