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

Split parser attempt #2 #192

Merged
merged 1 commit into from
Jan 6, 2024
Merged

Conversation

calebdw
Copy link
Collaborator

@calebdw calebdw commented Oct 23, 2023

Hello!

Please see #180 for previous discussions.

This is a rough draft at splitting the parser into two separate parsers: one for just PHP (php_only which allows top level statements, start/end tags are optional), and one that parsers both PHP and HTML (normal php parser). The same pattern as tree-sitter-typescript which uses a common grammar definition for the two dialects.

On another note, I can't seem to get the highlight tests to pass---it keeps giving me the following message...any ideas?

syntax highlighting:
No language found for path ".../tree-sitter-php/php_only/test/highlight/literals.php"

CC: @cfroystad

Closes #168

Checklist:

  • All tests pass in CI
  • There are sufficient tests for the new fix/feature
  • Grammar rules have not been renamed unless absolutely necessary (x rules renamed)
  • The conflicts section hasn't grown too much (x new conflicts)
  • The parser size hasn't grown too much (master: STATE_COUNT, PR: STATE_COUNT) (check the value of STATE_COUNT in src/parser.c)

@cfroystad
Copy link
Collaborator

Thanks for sticking with this and more than that your patience! I'm hoping to be able to allocate time to be more responsive in the near future.

If you add the tree-sitter section to the package.json files in php and php_only, highlight tests will run:

For php_only: (for php, use the same as in main package.json)

"tree-sitter": [
    {
        "scope": "source.php",
        "file-types": [
            "php"
        ],
        "highlights": "../queries/highlights.scm",
        "tags": "../queries/tags.scm"
    }
  ]

In general I like the approach, but would like to have some of bindings experts look over it as well as have some of the organizations depending on the parser test the result before final merge. However, let's try to get this to a fully passing test suite in CI before pinging them if we can 🙂

@calebdw calebdw force-pushed the common_parser branch 2 times, most recently from eee67f9 to 8de0912 Compare November 18, 2023 16:42
@calebdw
Copy link
Collaborator Author

calebdw commented Nov 18, 2023

@cfroystad, no worries---I understand :)

Thanks for the highlight test fix! That made sense and it worked.

The pipeline passed once, but now it's failing for some reason...

@amaanq
Copy link
Member

amaanq commented Nov 18, 2023

MacOS is scuffed because some runners are upgrading to python3.12, where distutils was removed but that's necessary for gyp, I'll upgrade the actions here so that windows/ubuntu don't depend on mac

@amaanq
Copy link
Member

amaanq commented Nov 18, 2023

Alright, done. Rebasing might be a pain - sorry about that 😬 but I had a bunch of tidying/release stuff I wanted added anyways and I released 0.20.0 since 0.19.1 was over 2 years old

@calebdw calebdw force-pushed the common_parser branch 4 times, most recently from b00ae77 to 81a55e4 Compare November 21, 2023 14:30
@calebdw
Copy link
Collaborator Author

calebdw commented Nov 21, 2023

@cfroystad, this has been rebased onto master and tests are passing with the exception of macos due to a dependency issue

@calebdw
Copy link
Collaborator Author

calebdw commented Jan 2, 2024

@cfroystad, I hope you had a great holiday season!

It looks like #198 fixes the macos pipeline---can we ping some of the experts to get eyes on this issue?

Thanks!

@cfroystad
Copy link
Collaborator

Thanks for your patience! I'm here at last 🙂

I've merged the CI fix and am happy to see all tests passing!

@aryx It seems to me like we're preserving backwards compatibility here, but do you see any issues for your semgrep integration? It's a while since I've been down that (very interesting) rabbit hole, but I guess it should work since it's the same approach as used by the JS and Typescript parsers?

@aryx
Copy link
Contributor

aryx commented Jan 4, 2024

Hopefully it's backward compatible and using the php parser works as before.

@calebdw
Copy link
Collaborator Author

calebdw commented Jan 4, 2024

@aryx, the grammar has not been changed, so php should continue to work

@EmranMR
Copy link

EmranMR commented Jan 6, 2024

Yess 🥳 Amazing work @calebdw , thank you so much for implementing this!

@calebdw calebdw deleted the common_parser branch January 6, 2024 14:25
tancnle added a commit to tancnle/py-tree-sitter-languages that referenced this pull request Jan 23, 2024
PHP grammar has been split since
tree-sitter/tree-sitter-php#192 and
'vendor/tree-sitter-php/src/parser.c' no longer exists.
tancnle added a commit to tancnle/py-tree-sitter-languages that referenced this pull request Jan 23, 2024
PHP grammar has been split since
tree-sitter/tree-sitter-php#192 and
'vendor/tree-sitter-php/src/parser.c' no longer exists.
grantjenks pushed a commit to grantjenks/py-tree-sitter-languages that referenced this pull request Jan 24, 2024
* Deprecate Python 3.6 and 3.7 builds

Both versions are already EOF.

* Bump build dependencies

* Bump grammar versions

* Fall back to earlier PHP version

PHP grammar has been split since
tree-sitter/tree-sitter-php#192 and
'vendor/tree-sitter-php/src/parser.c' no longer exists.

* Move erlang grammar to a maintained fork

* Increase build verbosity for easier debugging

* Exclude some grammars from upgrade

One the grammar is causing segmentation fault in the build.

* Update grammar references on README

* Add back support for Python 3.7
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 this pull request may close these issues.

Inject PHP code without PHP tags
5 participants