Skip to content

Conversation

adnelson
Copy link
Collaborator

@adnelson adnelson commented Feb 19, 2024

  • When a string reaches a certain length, the parse fails with a stack overflow error. This is caused by a recursive step in the string parsing process. It's not an infinite loop and would succeed given a large enough stack.
  • This PR prevents this from happening by switching from the recursive parsing to use regex instead to achieve the same result.
  • Added a test case for the long string.
  • Also updates all files to .res instead of .re. I'll add comments to highlight what changed, since most of the diff is file renaming

Comment on lines 148 to 160
test("long string value", () => {
let longValue = "pzqtdxvnhzvmzzigsojovukwfssmzadolomslufahgjbuininzcexwrkvnncmktcqpdcsrcnerdjrwgcncswsoovrrplikznwfqbvmrkeequwohejmrdmwmfcdkkfwhmqqiqgpfpelmhutdpshonwbtkxmwfoccaqogmzulumtcywyplotpbsldrampwwtmjwsmfhnnzumyhpyforahmivaalkhrenxxvnhuwpovjnkdjbrxvhobpmffinjtuaegkqejfxfejiatxkpxvxboftretjleyxysfwlkiyjfdnvhjdtsopwuvznzpzrzvntiixdqifzsiktwhvgimwpgsgxrbczqhnycsalycgcngilyjwlxkhmaieffcpfptwzqffrlwpksmjbndftkjkjcnwqgbfwoucguujltbcfkerbwrsoeofdmmawmzlegojrujqbkftagqarwbvrlmsgwyxhpcxrdyynjrbltvrtiruhbsmroovmgqfgvogrjshjbhzgucsmavrnyuxqwaqpjhlrqargmuoixwnorvepyvtybqbjrjzyafgzwxedpyezprhtbfzrcmpysfirgemwihpzlmciehdlolhrszfqnserejqqwsazshizvyuuagitooleocilvlfmoriwzpudhqdcngayfyyptuggzloyamzxtrekqeegawxjddprqkrwepeynqifacltgbxsmpqinpaegwkuvbawuuimculerazmttvvqptbyjmnsxrpiwtkqitsoljjqgfsghckooyzdqxeagckeqmmnkpmxqsgvqvbuhlwzgumrslhyvtegfaosnbfgmqdpbkgipiequsjgyopmbiwhvuryzknwayxedhiimqqnddlzgaxbklsvwjigjqltzitphlzguzgljzvylilltqystjnbyafopkanphhuntwbffnslweajomjvgzpkcjznwpnyliymimrfvcdcbcjgwmjdpmlfjiqhizk"
let rawText = `
{
"query": "${longValue}",
"variables": {
"searchTerm": "bag",
"sortBy" : {{sortBy}},
}
}`
expectOkParse(rawText, Object([("variables", Object([("searchTerm", String("bag")), ("sortBy", Variable("sortBy"))]->JsMap.fromArray)),("query", String(longValue))]->JsMap.fromArray))
})
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the added test

Comment on lines 28 to 40
let escapedQuoteRegex = %re(`/\\\\"/gm`)
let inQuotes = %re(`/"(?:[^"\\\\]|\\\\.)*"/gm`)
// Parse a string. Allows for escaped quotes.
// NOTE: not to be confused with `parse`, which takes a raw string and parses
// a whole AST -- this parses string literal syntax.
let parseString: t<string> =
regex(inQuotes)
|> map(match => {
match
->Js.String2.substring(~from=1, ~to_=match->Js.String2.length - 1) // Remove quotes from start and end
->Js.String2.replaceByRe(escapedQuoteRegex, `"`)
})
|> lexeme
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the updated parser

@YoussefHenna YoussefHenna changed the title Debugging stack overflow failure Fix stack overflow parsing failure Feb 21, 2024
Copy link
Collaborator Author

@adnelson adnelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Let's get this in there

@YoussefHenna YoussefHenna merged commit 7661ed8 into master Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants