Skip to content
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.

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

Bug: Parser.parse(string) fails if the string length is >= than the buffer size #199

Closed
kestred opened this issue May 11, 2024 · 5 comments · Fixed by #214
Closed

Bug: Parser.parse(string) fails if the string length is >= than the buffer size #199

kestred opened this issue May 11, 2024 · 5 comments · Fixed by #214

Comments

@kestred
Copy link

kestred commented May 11, 2024

In the README, it suggests you can pass a string to parse:

const sourcePath = '...';
const sourceCode =  readFileSync(sourcePath, 'utf-8');
const tree = parser.parse(source);

However if you pass a string longer than the default buffer size, parsing will fail with Error: Invalid Argument

const defaultBufferSize = 32 * 1024;
const sourceCode = ''.padStart(defaultBufferSize - 1, ' ');
const tree = parser.parse(source);  // --> OK

const defaultBufferSize = 32 * 1024;
const sourceCode = ''.padStart(defaultBufferSize, ' ');
const tree = parser.parse(source);  // --> ERROR
tree-sitter@0.21.1/node_modules/tree-sitter/index.js:361
    ? parse.call(
            ^

Error: Invalid argument
    at Parser.parse (tree-sitter@0.21.1/node_modules/tree-sitter/index.js:361:13)

Possible Solutions

Some ideas to improve the behavior:

  • Have Parser.prototype.parse's callback for strings be limited to buffer size:
     const DEFAULT_BUFFER_SIZE = 32 * 1024;
    
     const inputString = input;
     const maxSliceLength = (bufferSize || DEFAULT_BUFFER_SIZE) - 1;
     input = (offset, _position) => inputString.slice(index, index + maxSliceLength);
  • In CallbackInput::Read, throw an error with a clearer error message if the returned value from the callback has length >= buffer size
@kestred kestred changed the title Bug: Read from string fails if the string length is >= than the default buffer size Bug: Parser.parse(string) fails if the string length is >= than the default buffer size May 11, 2024
@kestred kestred changed the title Bug: Parser.parse(string) fails if the string length is >= than the default buffer size Bug: Parser.parse(string) fails if the string length is >= than the buffer size May 11, 2024
@Rannytheory
Copy link

Rannytheory commented May 15, 2024

parser.parse lets you pass in a buffer size.

const defaultBufferSize = 32 * 1024;
const sourceCode = ''.padStart(defaultBufferSize, ' ');
const tree = parser.parse(source, undefined, { bufferSize: defaultBufferSize + 1 });  // --> Not an error anymore

Might be worth putting a note somewhere about this in the readme though.

Edit: thanks @connor4312

@rien
Copy link

rien commented Jun 3, 2024

We are encountering this bug as well (dodona-edu/dolos#1544), it took a while to find the cause because of the error message. This seems to be a new problem (with the new NAPI?)

Maybe it is a good idea to improve the error message here by adding a manual check if the buffer size is enough? Or event prevent this from happening by increasing the buffer size if needed.

@connor4312
Copy link

Hit this as well -- seems like the bufferSize needs to be input.length + 1. (Yes, length in UTF-16 code units, not bytes.) Is there any downside to always setting that to be the buffer size?

@maxbrunsfeld
Copy link
Contributor

The buffer size should not affect the behavior - it should just control how much copying takes place. This is a really bad bug, it must have been introduced with the NAPI conversion.

@segevfiner
Copy link
Contributor

Trying to debug

segevfiner added a commit to segevfiner/node-tree-sitter that referenced this issue Jul 31, 2024
Caused by annoying limitation of Node-API where you can't create a reference to a String.

Fixes tree-sitter#199
segevfiner added a commit to swimmio/node-tree-sitter that referenced this issue Jul 31, 2024
Caused by annoying limitation of Node-API where you can't create a reference to a String.

Fixes tree-sitter#199
@amaanq amaanq closed this as completed in 9ea6558 Aug 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants