-
Notifications
You must be signed in to change notification settings - Fork 547
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
Gazelle seems to not like PEP 695 – Type Parameter Syntax #2396
Comments
ah, dang: rules_python/gazelle/python/file_parser.go Lines 85 to 88 in 91e5751
I just tried this locally and got the same issue. @hunshcn do you have any additional insight for this? |
Opened smacker/go-tree-sitter#175. |
I think type parameter support was added to the tree-sitter grammer in tree-sitter/tree-sitter-python@71977ea, and should be included in grammer versions 0.20.4 and later. It looks like this was first included in go-tree-sitter in smacker/go-tree-sitter@8516e80. |
This is looking more and more like an issue with In our code base, I narrowed things down to a single line diff: # get_deps.py
# using this line is fine - tree-sitter correctly parses the module
def search_one_more_level(graph: dict[T, set[T]], seen: set[T], routes: list[list[T]], target: T) -> list[T] | None:
# using this line cause tree-sitter to fail parsing - see below
def search_one_more_level[T](graph: dict[T, set[T]], seen: set[T], routes: list[list[T]], target: T) -> list[T] | None: The only difference between the two lines is the addition of I added some debugging prints to --- a/gazelle/python/file_parser.go
+++ b/gazelle/python/file_parser.go
@@ -17,6 +17,7 @@ package python
import (
"context"
"fmt"
+ "log"
"os"
"path/filepath"
"strings"
@@ -184,6 +185,10 @@ func (p *FileParser) Parse(ctx context.Context) (*ParserOutput, error) {
if err != nil {
return nil, err
}
+ if strings.Contains(p.relFilepath, "get_deps.py") {
+ log.Printf("after parsing file, got rootNode=%q", rootNode)
+ }
p.output.HasMain = p.parseMain(ctx, rootNode) And ran gazelle.
So something is up with |
@maffoo yes I agree; the 3.12 grammar was added to tree-sitter a while ago. rules_python is currently pinning to the version just before I tried forking go-tree-sitter and hacking things to use the old |
As I understand it we are using the standard Lines 188 to 193 in 155efce
And then we use two targets which are created automatically by this import: rules_python/gazelle/python/BUILD.bazel Lines 45 to 46 in 155efce
Is it possible to define a more custom way to build that instead of relying on targets generated automatically by |
Actually, looks like the |
I checked out the
and then added a reference to this in the
This was enough to get |
Thanks Matthew! My next attempt was going to be to just move/copy those C flies into the |
While investigating #2396 and why #2413 doesn't appear to be working for us, I realized that one of the things I was making heavy use of was additional parser logging that I had added. This adds some of that logging. I also throw in some documentation because I found it helpful. Users can (attempt to) get additional parse failure information by setting the `GAZELLE_VERBOSE` environment variable to `1`. Eg: ```console $ GAZELLE_VERBOSE=1 bazel run //:gazelle ``` Here are some example logs: ```console $ GAZELLE_VERBOSE=1 bazel run //:gazelle INFO: Invocation ID: a4e026d8-17df-426c-b1cc-d3980690dd53 ... INFO: Running command line: bazel-bin/gazelle INFO: Streaming build results to: https://btx.cloud.google.com/invocations/a4e026d8-17df-426c-b1cc-d3980690dd53 gazelle: WARNING: failed to parse "hello/get_deps.py". The resulting BUILD target may be incorrect. gazelle: Parse error at {Row:1 Column:0}: def search_one_more_level[T](): gazelle: The above was parsed as: (ERROR (identifier) (call function: (list (identifier)) arguments: (argument_list))) gazelle: ERROR: failed to generate target "//hello:get_deps" of kind "py_library": a target of kind "pyle_py_binary" with the same name already exists. Use the '# gazelle:python_library_naming_convention' directive to change the naming convention. $ $ bazel run //:gazelle INFO: Invocation ID: 726c9fd6-f566-4c30-95ef-c4781ad155de ... INFO: Running command line: bazel-bin/gazelle INFO: Streaming build results to: https://btx.cloud.google.com/invocations/726c9fd6-f566-4c30-95ef-c4781ad155de gazelle: WARNING: failed to parse "hello/get_deps.py". The resulting BUILD target may be incorrect. gazelle: ERROR: failed to generate target "//hello:get_deps" of kind "py_library": a target of kind "pyle_py_binary" with the same name already exists. Use the '# gazelle:python_library_naming_convention' directive to change the naming convention. ``` --------- Co-authored-by: Richard Levasseur <[email protected]> Co-authored-by: Richard Levasseur <[email protected]>
… Parameter Syntax) by using dougthor42's fork of go-tree-sitter (#2496) Replaces #2413. Fixes #2396. This updates the `go-tree-sitter` dependency to use my fork that includes `BUILD.bazel` files. Specifically, the `BUILD.bazel` files in the fork include references to top-level code like `array.h` which the original Gazelle-generated files for `go-tree-sitter` were not able to handle. I also include the test cases that @maffoo created in #2413 and verified that they (a) fail before the fix and (b) pass after the fix. The fork is: https://github.com/dougthor42/go-tree-sitter The branch that includes all changes is: https://github.com/dougthor42/go-tree-sitter/tree/for-rules-python-gazelle-plugin A couple notes: + I have a PR open to get `go-tree-sitter` into BCR [here](bazelbuild/bazel-central-registry#3366). However: 1. I'm having trouble getting tests to pass and to get things running locally to validate it 2. Using BCR would not fix things for people who still use WORKSPACE (right?) + The fork is _mostly_ [autogenerated BUILD.bazel files from gazelle](smacker/go-tree-sitter@cfa9bdf) but also contains: + [manual updates so that build files reference the toplevel `array.h` and other files](smacker/go-tree-sitter@63f89cd) + [replace all `smacker` with `dougthor42` so that `go build` works](smacker/go-tree-sitter@8a73cbd) + various other more minor things. + I was unable to get `go mod edit -replace` to work, so I've just manually updated `go.mod` and whatnot everywhere. If someone with more go knowledge has a suggestion I'm happy to hear it. --------- Co-authored-by: Matthew Neeley <[email protected]> Co-authored-by: Ignas Anikevicius <[email protected]>
TL;DR: I think we need to bump go-tree-sitter for python 3.12 support.
🐞 bug report
Affected Rule
Gazelle
Is this a regression?
I could argue it either way, lol. I'm going to go with "probably" because it's related to #1895.
Description
PEP 695 added a Type Parameter syntax that looks like:
If a python file uses this new
def func[T](...)
syntax, and the file hasif __name__ == "__main__"
, then Gazelle will incorrectly generate apy_library
target rather thanpy_binary
.🔬 Minimal Reproduction
bazel run //:gazelle
Expected Result
Actual Result
🔥 Exception or Error
None
🌍 Your Environment
Operating System:
gLinux
Output of
bazel version
:Rules_python version:
Anything else relevant?
I haven't been able to find a human-readable changelog of
tree-sitter
orgo-tree-sitter
that explicitly says when it added support for python 3.12...The text was updated successfully, but these errors were encountered: