Conversation
Changes the current text-based dump of the tsg-python graph to the native JSON output that tree-sitter-graph provides. This saves us the hassle of parsing the output ourselves, including the recently added bug fix that allowed us to interpret Rust-style string escapes in Python. As an added bonus, relying on the json module instead of our own parser roughly cuts the AST reconstruction time in half compared to previously, an extraction speedup in the neighbourhood of 3-4% (when tested on an extraction of `python/cpython` using only the tsg-python parser).
Rerun has been triggered: 6 restarted 🚀 |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The updated parser test fails independently because unittest.mock is not explicitly imported.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
 |
python/extractor/tests/test_tsg_parser.py — Importing unittest alone does not load its mock submodule, so the new test raises… |
What changed in this PR
Switches tsg-python graph interchange from custom text parsing to native JSON serialization.
Changes:
- Serializes TSG graphs with
serde_json. - Decodes typed graph values in Python.
- Updates dependencies, tests, and extractor version.
| File | Description |
|---|---|
MODULE.bazel |
Registers the JSON dependency. |
misc/bazel/3rdparty/py_deps/BUILD.bazel |
Adds dependency aliases. |
misc/bazel/3rdparty/py_deps/defs.bzl |
Configures the vendored crate. |
python/extractor/semmle/python/parser/tsg_parser.py |
Replaces text parsing with JSON decoding. |
python/extractor/semmle/util.py |
Bumps the extractor version. |
python/extractor/tests/test_tsg_parser.py |
Tests JSON graph decoding. |
python/extractor/tsg-python/Cargo.lock |
Locks the direct JSON dependency. |
python/extractor/tsg-python/Cargo.toml |
Declares serde_json. |
python/extractor/tsg-python/src/main.rs |
Emits the graph as JSON. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: tausbn <1104778+tausbn@users.noreply.github.com>
Rerun has been triggered: 6 restarted 🚀 |
There was a problem hiding this comment.
Looks good to me. One comment on recursion, but I assume it will not become a problem, so approving the PR.
Reading the format of the Json passed between rust and python, it seems quite to be quite redundant; could we not derive the type based on the actual value? e.g.
{ "value": 123 } hold exactly the same information as { type:"int", "int": 123 } (since we are not handling floats as far as I can see).
I have not looked where this structure is defined though - it may be well outside our control.
| if value_type in ("bool", "int", "string"): | ||
| return value[value_type] | ||
| if value_type == "list": | ||
| return [_decode_tsg_value(item) for item in value["values"]] |
There was a problem hiding this comment.
Could recursion level become too deep? The previous implementation does not use recursion.
Changes the current text-based dump of the
tsg-pythongraph to the native JSON output thattree-sitter-graphprovides. This saves us the hassle of parsing the output ourselves, including the recently added bug fix that allowed us to interpret Rust-style string escapes in Python.As an added bonus, relying on the
jsonmodule instead of our own parser roughly cuts the AST reconstruction time in half compared to previously, an extraction speedup in the neighbourhood of 3-4% (when tested on an extraction ofpython/cpythonusing only thetsg-pythonparser).