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

WASM cannot use tree-sitter-swift for tests #8

Open
mattmassicotte opened this issue Oct 5, 2022 · 10 comments
Open

WASM cannot use tree-sitter-swift for tests #8

mattmassicotte opened this issue Oct 5, 2022 · 10 comments

Comments

@mattmassicotte
Copy link
Contributor

I'm not 100% sure what the issue is, but something in CI is unhappy with the submodule I added to get access to the swift parser. @fjtrujy can I enlist some help?

@fjtrujy
Copy link
Contributor

fjtrujy commented Oct 6, 2022

Let me investigate it

@fjtrujy
Copy link
Contributor

fjtrujy commented Oct 6, 2022

I need to ask the guys from the WASM toolchain but looks like we are generating a function with massive local variables and it exceeds the limit (not sure if there is an easy way of increasing it).
You have more info here:
https://github.com/GoodNotes/SwiftTreeSitter/actions/runs/3195179311/jobs/5215534738

I suppose that this is happening because you have added tree-sitter-swift which contains huge enums, arrays, and so on...
https://github.com/tree-sitter/tree-sitter-swift/blob/master/src/parser.c

By the way, why are you adding that dependency for the tests? I don't get it.

Cheers

@fjtrujy
Copy link
Contributor

fjtrujy commented Oct 6, 2022

More concretely the error is being triggered in this function:
https://github.com/tree-sitter/tree-sitter-swift/blob/master/src/parser.c#L1653

Cheers

@mattmassicotte
Copy link
Contributor Author

Very interesting issue.

I've been resisting for a long time, but I need a real parser for some testing. I deliberately did not add the SPM package as a dependency, to try to avoid knock-on dependency issues for consumers. I did a little investigating with what SPM was building, and it looked to me like the test target dependency was being ignored for regular usage.

Is this because you run the tests as part of the WASM CI? Does this mean that some tree-sitter parsers aren't compilable by WASM, or is this a unique issue?

@fjtrujy
Copy link
Contributor

fjtrujy commented Oct 6, 2022

Very interesting issue.

I've been resisting for a long time, but I need a real parser for some testing. I deliberately did not add the SPM package as a dependency, to try to avoid knock-on dependency issues for consumers. I did a little investigating with what SPM was building, and it looked to me like the test target dependency was being ignored for regular usage.

Is this because you run the tests as part of the WASM CI? Does this mean that some tree-sitter parsers aren't compilable by WASM, or is this a unique issue?

So basically, such kind of C code where we have tons of variables, switches, if-else if, gotos, huge functions.... are candidates to generate WASM code that exceed the interpreter limits.
We have created this parser for being able to parse RTF content, and it works without any problem, it depends on how crazy is the generated code.

https://github.com/GoodNotes/tree-sitter-rtf

Cheers

@mattmassicotte
Copy link
Contributor Author

That makes total sense. And the reason the WASM compiler sees this code is because you are running the tests? Maybe we can conditionalize the dependency on os?

@fjtrujy
Copy link
Contributor

fjtrujy commented Oct 6, 2022

That makes total sense. And the reason the WASM compiler sees this code is because you are running the tests? Maybe we can conditionalize the dependency on os?

Exactly, I'm running the same tests for WASM as for iOS, MacOS....
I'd rather prefer to keep running the tests, this is the way to identify potential issues in the Swift WASM toolchain, and then try to fix, that's the best path to keep improving the toolchain.

@mattmassicotte
Copy link
Contributor Author

Ok, sounds good. I'll leave this issue open.

@mattmassicotte mattmassicotte changed the title Having trouble with WASM and new tests WASM cannot use tree-sitter-swift for tests Oct 6, 2022
@mattmassicotte
Copy link
Contributor Author

Hello @fjtrujy ! The WASM tests broke again after I pulled in a tree-sitter change. It looks like there was a subtle change to how they are importing some C functions. But I'm not 100% sure I understand why what they've done is significantly difference from before. Do you have any interest in having a look?

@fjtrujy
Copy link
Contributor

fjtrujy commented Jul 5, 2023

Ok, I will try to take a look in the next couple days

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

No branches or pull requests

2 participants