From 10229373529354dc9ba5cc651bfa8814bd163246 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sat, 22 Aug 2026 20:01:30 -0700 Subject: [PATCH] refactor(py): harden the unsafe soundness argument MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The module doc of big-code-analysis-py/src/node.rs is this workspace's canonical justification for its only sanctioned `unsafe` block. Two things weakened it, neither a live soundness bug. The argument reasons about a *named* tree-sitter release — `Tree(NonNull )`, `Node<'tree>(ffi::TSNode, PhantomData<&'tree ()>)`, `Tree::edit(&mut self)`, `Send + Sync` — and the literal had drifted two patch releases behind the pin before anyone noticed. It is correct today only because the 0.26.12 bump updated it by hand, which is the step this gate removes. `utils/check-safety-doc-pin.py` fails when a backticked `=X.Y.Z` in that module doc is not the `[workspace.dependencies]` pin, when the citation is dropped altogether (the version-free phrasing was considered and declined: it removes the stale number by making the claim unfalsifiable at read time), and when the requirement is not an exact pin at all. The gate cannot verify the argument; it forces a diff on the line at bump time, which is the prompt to re-read it. Wired into `make lint` / `pre-commit` / `ci` beside check-versions, with a pre-commit hook pair on Cargo.toml + node.rs, defensive twins in ci.yml, and 17 self- tests — including both directions of the module-doc stop condition, since a scanner that stops reading early reports a clean file. `detach`'s precondition is that a node is stored alongside the `Py` that keeps its tree alive, and three sites built `PyNode { ast, node }` by struct literal. Field privacy does not constrain them: all three were in the defining module. `PyNode` and `PyNodeWalk` now live in an inner `owned` module, so the literal is unavailable to the `#[pymethods]` blocks outside it and every handle comes from `wrap`, `rewrap`, `rooted_at`, or `next_node`. `rooted_at` takes the `PyNode` rather than an `(Ast, node)` pair, so a walk's cursor, seed and keep-alive all derive from one already-valid handle. Verified by perturbation rather than a test: reintroducing either literal is now E0451, and a runtime test would pass vacuously because the mismatch is unconstructible from Python. That last fact, and the rest of the adversarial-harness results from the issue — no GC tracking, no Python-reachable constructor, the per-yield refcount bump, the 3,000-cycle and 8-thread runs — are recorded in the module doc, where an auditor of the `unsafe` block will find them. The gate is recorded under `## [Unreleased]`, beside the sibling `check-ruff-lockstep` entry. No `STABILITY.md` entry: no library behaviour moves and `PyNode` is not part of the Rust public API. Everything in `owned` except `wrap` is `pub(super)`. `wrap` is the one entry point another module needs — `ast.rs` builds the root handle — while `node` + `rewrap` together are the mispairing primitive (`a.rewrap(py, b.node())` pairs one tree's node with another tree's keep-alive), so keeping them file-local is what makes the boundary true beyond this file. Fixes #1057 --- .github/workflows/ci.yml | 13 ++ .pre-commit-config.yaml | 25 ++ AGENTS.md | 25 +- CHANGELOG.md | 12 + Makefile | 41 +++- big-code-analysis-py/src/node.rs | 351 ++++++++++++++++++----------- utils/check-safety-doc-pin-test.py | 216 ++++++++++++++++++ utils/check-safety-doc-pin.py | 182 +++++++++++++++ 8 files changed, 732 insertions(+), 133 deletions(-) create mode 100644 utils/check-safety-doc-pin-test.py create mode 100755 utils/check-safety-doc-pin.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a02cf585f..502995322 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -613,6 +613,19 @@ jobs: run: python3 utils/check-diagnostic-prefix.py - name: check-diagnostic-prefix self-tests (explicit) run: python3 -m unittest -q utils/check-diagnostic-prefix-test.py + # Defensive twin for the safety-doc pin gate (#1057): the + # module doc of big-code-analysis-py/src/node.rs is the + # canonical soundness argument for this workspace's only + # sanctioned `unsafe` block, and it reasons about a named + # tree-sitter release. A bump that leaves the literal behind + # leaves an argument that reads as verified against a crate + # nobody compiles. Its self-tests run as their own step for the + # usual reason: a source-scanning gate that stops matching + # reports a clean tree. + - name: check-safety-doc-pin (explicit) + run: python3 utils/check-safety-doc-pin.py + - name: check-safety-doc-pin self-tests (explicit) + run: python3 -m unittest -q utils/check-safety-doc-pin-test.py # Defensive twin for the grammar-marker-sync gate (#400): bumping # the notification-only marker in tree-sitter-{mozjs,mozcpp}/ # Cargo.toml without re-running the matching generate-*.sh diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 6fac50cbd..bf3287928 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -411,6 +411,31 @@ repos: entry: python3 -m unittest -q utils/check-diagnostic-prefix-test.py pass_filenames: false + # Safety-doc pin gate — the module doc of + # big-code-analysis-py/src/node.rs is the canonical soundness + # argument for this workspace's only sanctioned `unsafe` block, + # and it reasons about a named tree-sitter release. #1057: the + # pin moved twice while the literal stayed at `=0.26.9`, leaving + # an argument that read as verified against a crate nobody was + # compiling. Fires on the root manifest as well as the file, so + # a bump cannot land without the citation being re-checked. + - id: check-safety-doc-pin + name: check-safety-doc-pin + language: system + files: '^(Cargo\.toml|big-code-analysis-py/src/node\.rs|utils/check-safety-doc-pin\.py)$' + entry: python3 utils/check-safety-doc-pin.py + pass_filenames: false + + # Self-tests for the safety-doc-pin gate, for the same reason as + # the diagnostic-prefix pair above: a source-scanning gate that + # stops matching reports a clean tree. + - id: check-safety-doc-pin-test + name: check-safety-doc-pin-test + language: system + files: '^utils/check-safety-doc-pin(-test)?\.py$' + entry: python3 -m unittest -q utils/check-safety-doc-pin-test.py + pass_filenames: false + # The `enums/` crate is workspace-excluded, so the workspace # clippy/test hooks above never touch it. Add a dedicated check # so warnings here cannot drift (see #164). diff --git a/AGENTS.md b/AGENTS.md index fd0b7e5f5..6f7c3441f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -62,7 +62,7 @@ and `cargo run -p big-code-analysis-web --`. by `make pre-commit` / `make ci`: `check-versions.py`, `check-snapshot-anchors.py`, `check-rustfmt-bail.py`, `check-manpage-assets.py`, `check-manpage-drift.py`, - `check-diagnostic-prefix.py`, + `check-diagnostic-prefix.py`, `check-safety-doc-pin.py`, `check-grammar-marker-sync.py`, `check-enums-codegen-drift.sh`, `check-grammar-crate.py`, `check-grammars-crates.sh`, `check-excluded-manifests.py`, `check-ruff-lockstep.py`, @@ -200,7 +200,12 @@ and `cargo run -p big-code-analysis-web --`. keeping the owning tree alive through a strong `Py<...>` handle. The canonical soundness argument lives at `big-code-analysis-py/src/node.rs` (the `# Safety` module doc and - `detach`). Any `unsafe` outside this exact pattern remains banned and + `detach`), and the node/`Ast` pairing that argument depends on is + enforced by the `owned` module boundary there — private fields, so a + handle can only be built by `wrap` / `rewrap` / `rooted_at` and not by + a struct literal that pairs a node with the wrong tree (#1057). The + version the doc names is gated by `make check-safety-doc-pin`. Any + `unsafe` outside this exact pattern remains banned and needs a deliberate amendment to this rule. (The PyO3-macro-generated FFI shims under `#![allow(unsafe_op_in_unsafe_fn)]` in `src/lib.rs` are source-level `unsafe`-free and not covered by this exception.) @@ -274,7 +279,12 @@ modified, deleted, **and** newly added pages, the last of which `git diff` alone cannot see, #1249), the diagnostic-prefix gate (`make check-diagnostic-prefix`, which blocks a capitalised `Warning:` / `Error:` / `Note:` string literal — see "Rust -conventions"), the bca self-scan threshold gate at both +conventions"), the safety-doc pin gate +(`make check-safety-doc-pin`, which fails when the `tree-sitter` +version cited by the `unsafe` soundness argument in +`big-code-analysis-py/src/node.rs` is not the version +`[workspace.dependencies]` pins, or when the citation is dropped +altogether — #1057), the bca self-scan threshold gate at both tiers (`make self-scan` mirroring the `Threshold gate` step in `.github/workflows/pages.yml`, plus `make self-scan-headroom` which scales every limit by `BCA_HEADROOM` — default `0.95` — so @@ -722,6 +732,15 @@ pins it at `=0.26.12` with the workspace resolving. Its ABI version is also what each vendored `parser.c` was generated against, so an accidental bump is precisely the drift the gate exists to catch. +A runtime bump also has to carry the `unsafe` soundness argument with +it. `big-code-analysis-py/src/node.rs` reasons about a *named* +tree-sitter release — `Tree(NonNull)`, +`Node<'tree>(ffi::TSNode, PhantomData<&'tree ()>)`, +`Tree::edit(&mut self)`, `Send + Sync` — and `make +check-safety-doc-pin` fails until the literal in that doc matches the +new pin. Re-read the argument against the new release before editing +the line; the forced diff is the prompt, not the fix (#1057). + Treat the pinned version as fixed: - Do not loosen pins to a range without explicit user approval. diff --git a/CHANGELOG.md b/CHANGELOG.md index 72af0e069..b9a1adbfe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -89,6 +89,18 @@ for historical reference. by a comment alone and had already drifted silently once (`v0.15.14` against a lockfile resolving 0.15.22), which stays invisible until the two versions disagree and then presents as "works locally, red in CI" (#1230). +- A `check-safety-doc-pin` gate (`make check-safety-doc-pin`, wired into + `make lint` / `pre-commit` / `ci`, the pre-commit hooks, and its own CI + step) holds the `tree-sitter` version cited by the `unsafe` soundness + argument in `big-code-analysis-py/src/node.rs` equal to the version + `[workspace.dependencies]` pins. That module doc is the canonical + justification for the workspace's only sanctioned `unsafe` block and + reasons about a *named* release — the `Node<'tree>` layout, `Tree::edit` + taking `&mut self`, `Send + Sync` — so a pin that moves while the literal + does not leaves an argument reading as verified against a crate nobody + compiles. The gate also fails when the literal is dropped altogether, + since a version-free phrasing hides the staleness rather than fixing it. + No library behaviour changes (#1057). - **web:** `error_kind` token `vcs_invalid_author_hash_key`. The `STABILITY.md` vocabulary list also gains `not_acceptable` and diff --git a/Makefile b/Makefile index 933e4613f..1c6d4b058 100644 --- a/Makefile +++ b/Makefile @@ -88,7 +88,7 @@ find-by-ext = $(if $(FD),$(FD) --extension $(1) $(FD_EXCLUDE) $(2),find . -name NEXTEST := $(shell command -v cargo-nextest 2>/dev/null) TEST_CMD = $(if $(NEXTEST),$(NEXTEST) nextest run --workspace --all-features,cargo test --workspace --all-features --lib --bins --tests) -.PHONY: help check-tools worktree-setup worktree-setup-test build build-release check test test-doc chain-audit fmt fmt-check markdown-fmt markdown-lint shellcheck sh-fmt sh-fmt-check toml-fmt toml-fmt-check toml-lint makefile-check actionlint snapshot-anchors snapshot-anchors-test rustfmt-bail rustfmt-bail-test grammar-marker-sync grammar-marker-sync-test check-versions check-excluded-manifests check-excluded-manifests-test check-ruff-lockstep check-ruff-lockstep-test check-publish-metadata check-publish-metadata-test check-manpage-assets check-manpage-drift-test check-diagnostic-prefix check-diagnostic-prefix-test gate-status-test check-tools-test enums-check enums-codegen-drift enums-codegen-drift-test self-scan self-scan-headroom self-scan-write-baseline self-scan-write-baseline-headroom vcs lint clippy udeps insta-review insta-accept clean distclean install install-cli install-web doc doc-open doc-check doc-check-docsrs book book-serve book-pot book-po-update book-ja book-deploy all pre-commit ci release-check verify-changelog pkg-deb-local pkg-rpm-local dev-env-build dev-env-run dev-env-shell dev-env-rm py-bootstrap py-sync py-relock py-clean py-fmt py-fmt-check py-lint py-typecheck py-test py-stubtest smoke smoke-cli smoke-lib bench bench-scaling bench-walk fuzz-check fuzz-smoke fuzz-replay fuzz-run fuzz-tmin _check-find _pc-all _pc-fmt _pc-clippy _pc-test _pc-doc-check _pc-udeps _pc-shellcheck _pc-markdown-lint _pc-toml-lint _pc-makefile-check _pc-actionlint _pc-snapshot-anchors _pc-snapshot-anchors-test _pc-rustfmt-bail _pc-rustfmt-bail-test _pc-grammar-marker-sync _pc-grammar-marker-sync-test _pc-check-versions _pc-check-versions-test _pc-check-grammar-crate-test _pc-check-excluded-manifests _pc-check-excluded-manifests-test _pc-check-ruff-lockstep _pc-check-ruff-lockstep-test _pc-check-publish-metadata _pc-check-publish-metadata-test _pc-check-manpage-assets _pc-check-manpage-drift-test _pc-check-diagnostic-prefix _pc-check-diagnostic-prefix-test _pc-worktree-setup-test _pc-gate-status-test _pc-check-tools-test _pc-enums-check _pc-enums-codegen-drift _pc-enums-codegen-drift-test _pc-self-scan _pc-self-scan-headroom _pc-py-fmt _pc-py-typecheck _pc-py-test _pc-py-stubtest _ci-all _ci-fmt-check _ci-clippy _ci-test _ci-doc-check _ci-build _ci-udeps _ci-shellcheck _ci-markdown-lint _ci-toml-lint _ci-makefile-check _ci-actionlint _ci-snapshot-anchors _ci-snapshot-anchors-test _ci-rustfmt-bail _ci-rustfmt-bail-test _ci-grammar-marker-sync _ci-grammar-marker-sync-test _ci-check-versions _ci-check-versions-test _ci-check-grammar-crate-test _ci-check-excluded-manifests _ci-check-excluded-manifests-test _ci-check-ruff-lockstep _ci-check-ruff-lockstep-test _ci-check-publish-metadata _ci-check-publish-metadata-test _ci-check-manpage-assets _ci-check-manpage-drift-test _ci-check-diagnostic-prefix _ci-check-diagnostic-prefix-test _ci-worktree-setup-test _ci-gate-status-test _ci-check-tools-test _ci-enums-check _ci-enums-codegen-drift _ci-enums-codegen-drift-test _ci-enums-codegen-drift-test _ci-self-scan _ci-self-scan-headroom _ci-cargo-pipeline _ci-py-fmt-check _ci-py-lint _ci-py-typecheck _ci-py-test _ci-py-stubtest +.PHONY: help check-tools worktree-setup worktree-setup-test build build-release check test test-doc chain-audit fmt fmt-check markdown-fmt markdown-lint shellcheck sh-fmt sh-fmt-check toml-fmt toml-fmt-check toml-lint makefile-check actionlint snapshot-anchors snapshot-anchors-test rustfmt-bail rustfmt-bail-test grammar-marker-sync grammar-marker-sync-test check-versions check-excluded-manifests check-excluded-manifests-test check-ruff-lockstep check-ruff-lockstep-test check-publish-metadata check-publish-metadata-test check-manpage-assets check-manpage-drift-test check-diagnostic-prefix check-diagnostic-prefix-test check-safety-doc-pin check-safety-doc-pin-test gate-status-test check-tools-test enums-check enums-codegen-drift enums-codegen-drift-test self-scan self-scan-headroom self-scan-write-baseline self-scan-write-baseline-headroom vcs lint clippy udeps insta-review insta-accept clean distclean install install-cli install-web doc doc-open doc-check doc-check-docsrs book book-serve book-pot book-po-update book-ja book-deploy all pre-commit ci release-check verify-changelog pkg-deb-local pkg-rpm-local dev-env-build dev-env-run dev-env-shell dev-env-rm py-bootstrap py-sync py-relock py-clean py-fmt py-fmt-check py-lint py-typecheck py-test py-stubtest smoke smoke-cli smoke-lib bench bench-scaling bench-walk fuzz-check fuzz-smoke fuzz-replay fuzz-run fuzz-tmin _check-find _pc-all _pc-fmt _pc-clippy _pc-test _pc-doc-check _pc-udeps _pc-shellcheck _pc-markdown-lint _pc-toml-lint _pc-makefile-check _pc-actionlint _pc-snapshot-anchors _pc-snapshot-anchors-test _pc-rustfmt-bail _pc-rustfmt-bail-test _pc-grammar-marker-sync _pc-grammar-marker-sync-test _pc-check-versions _pc-check-versions-test _pc-check-grammar-crate-test _pc-check-excluded-manifests _pc-check-excluded-manifests-test _pc-check-ruff-lockstep _pc-check-ruff-lockstep-test _pc-check-publish-metadata _pc-check-publish-metadata-test _pc-check-manpage-assets _pc-check-manpage-drift-test _pc-check-diagnostic-prefix _pc-check-diagnostic-prefix-test _pc-check-safety-doc-pin _pc-check-safety-doc-pin-test _pc-worktree-setup-test _pc-gate-status-test _pc-check-tools-test _pc-enums-check _pc-enums-codegen-drift _pc-enums-codegen-drift-test _pc-self-scan _pc-self-scan-headroom _pc-py-fmt _pc-py-typecheck _pc-py-test _pc-py-stubtest _ci-all _ci-fmt-check _ci-clippy _ci-test _ci-doc-check _ci-build _ci-udeps _ci-shellcheck _ci-markdown-lint _ci-toml-lint _ci-makefile-check _ci-actionlint _ci-snapshot-anchors _ci-snapshot-anchors-test _ci-rustfmt-bail _ci-rustfmt-bail-test _ci-grammar-marker-sync _ci-grammar-marker-sync-test _ci-check-versions _ci-check-versions-test _ci-check-grammar-crate-test _ci-check-excluded-manifests _ci-check-excluded-manifests-test _ci-check-ruff-lockstep _ci-check-ruff-lockstep-test _ci-check-publish-metadata _ci-check-publish-metadata-test _ci-check-manpage-assets _ci-check-manpage-drift-test _ci-check-diagnostic-prefix _ci-check-diagnostic-prefix-test _ci-check-safety-doc-pin _ci-check-safety-doc-pin-test _ci-worktree-setup-test _ci-gate-status-test _ci-check-tools-test _ci-enums-check _ci-enums-codegen-drift _ci-enums-codegen-drift-test _ci-enums-codegen-drift-test _ci-self-scan _ci-self-scan-headroom _ci-cargo-pipeline _ci-py-fmt-check _ci-py-lint _ci-py-typecheck _ci-py-test _ci-py-stubtest # Default target help: @@ -146,6 +146,8 @@ help: @echo " check-manpage-drift-test Self-tests for the man-page drift gate" @echo " check-diagnostic-prefix Block capitalised Warning:/Error:/Note: literals" @echo " check-diagnostic-prefix-test Self-tests for the diagnostic-prefix gate" + @echo " check-safety-doc-pin Assert node.rs's unsafe soundness doc cites the live tree-sitter pin" + @echo " check-safety-doc-pin-test Self-tests for the safety-doc-pin gate" @echo " worktree-setup-test Self-tests for the worktree-setup submodule classifier" @echo " gate-status-test Self-tests for the pre-commit/ci BCA_GATE verdict line" @echo " check-tools-test Self-tests for the check-tools tool probes and hints" @@ -632,6 +634,23 @@ check-diagnostic-prefix-test: @echo "Running check-diagnostic-prefix self-tests..." @(cd $(BASE_DIR) && python3 -m unittest -q utils/check-diagnostic-prefix-test.py) +# Safety-doc pin gate (#1057). The module doc of +# big-code-analysis-py/src/node.rs is the canonical soundness argument +# for this workspace's only sanctioned `unsafe` block, and it reasons +# about a named tree-sitter release. When the pin moves and that literal +# does not, the argument keeps reading as verified while describing a +# crate nobody compiles against. Static lint - no network, no cargo. +check-safety-doc-pin: + @echo "Checking the unsafe soundness doc's tree-sitter citation..." + @python3 $(BASE_DIR)utils/check-safety-doc-pin.py + +# Self-tests for the safety-doc-pin gate. Separate target for the same +# reason as the other gate self-tests: a source-scanning gate that stops +# matching reports a clean tree. +check-safety-doc-pin-test: + @echo "Running check-safety-doc-pin self-tests..." + @(cd $(BASE_DIR) && python3 -m unittest -q utils/check-safety-doc-pin-test.py) + # Sync gate for check-grammar-crate.py's EXTENSIONS table. Re-derives # the grammar -> extension mapping from src/langs.rs `mk_langs!` and # fails if the hand-maintained table has drifted (#869). Static — no @@ -1330,7 +1349,7 @@ lint: $(MAKE) -j --output-sync=target \ _ci-clippy \ _ci-shellcheck _ci-markdown-lint _ci-toml-lint _ci-makefile-check \ - _ci-actionlint _ci-snapshot-anchors _ci-snapshot-anchors-test _ci-rustfmt-bail _ci-rustfmt-bail-test _ci-grammar-marker-sync _ci-grammar-marker-sync-test _ci-check-versions _ci-check-versions-test _ci-check-grammar-crate-test _ci-check-excluded-manifests _ci-check-excluded-manifests-test _ci-check-ruff-lockstep _ci-check-ruff-lockstep-test _ci-check-publish-metadata _ci-check-publish-metadata-test _ci-check-manpage-assets _ci-check-manpage-drift-test _ci-check-diagnostic-prefix _ci-check-diagnostic-prefix-test _ci-worktree-setup-test _ci-gate-status-test _ci-check-tools-test _ci-enums-check _ci-enums-codegen-drift _ci-enums-codegen-drift-test + _ci-actionlint _ci-snapshot-anchors _ci-snapshot-anchors-test _ci-rustfmt-bail _ci-rustfmt-bail-test _ci-grammar-marker-sync _ci-grammar-marker-sync-test _ci-check-versions _ci-check-versions-test _ci-check-grammar-crate-test _ci-check-excluded-manifests _ci-check-excluded-manifests-test _ci-check-ruff-lockstep _ci-check-ruff-lockstep-test _ci-check-publish-metadata _ci-check-publish-metadata-test _ci-check-manpage-assets _ci-check-manpage-drift-test _ci-check-diagnostic-prefix _ci-check-diagnostic-prefix-test _ci-check-safety-doc-pin _ci-check-safety-doc-pin-test _ci-worktree-setup-test _ci-gate-status-test _ci-check-tools-test _ci-enums-check _ci-enums-codegen-drift _ci-enums-codegen-drift-test # --------------------------------------------------------------------------- # Maintenance @@ -1502,7 +1521,7 @@ _pc-all: $(MAKE) -j --output-sync=target \ _pc-test \ _pc-shellcheck _pc-markdown-lint _pc-toml-lint _pc-makefile-check \ - _pc-actionlint _pc-snapshot-anchors _pc-snapshot-anchors-test _pc-rustfmt-bail _pc-rustfmt-bail-test _pc-grammar-marker-sync _pc-grammar-marker-sync-test _pc-check-versions _pc-check-versions-test _pc-check-grammar-crate-test _pc-check-excluded-manifests _pc-check-excluded-manifests-test _pc-check-ruff-lockstep _pc-check-ruff-lockstep-test _pc-check-publish-metadata _pc-check-publish-metadata-test _pc-check-manpage-assets _pc-check-manpage-drift-test _pc-check-diagnostic-prefix _pc-check-diagnostic-prefix-test _pc-worktree-setup-test _pc-gate-status-test _pc-check-tools-test _pc-enums-check _pc-enums-codegen-drift _pc-enums-codegen-drift-test \ + _pc-actionlint _pc-snapshot-anchors _pc-snapshot-anchors-test _pc-rustfmt-bail _pc-rustfmt-bail-test _pc-grammar-marker-sync _pc-grammar-marker-sync-test _pc-check-versions _pc-check-versions-test _pc-check-grammar-crate-test _pc-check-excluded-manifests _pc-check-excluded-manifests-test _pc-check-ruff-lockstep _pc-check-ruff-lockstep-test _pc-check-publish-metadata _pc-check-publish-metadata-test _pc-check-manpage-assets _pc-check-manpage-drift-test _pc-check-diagnostic-prefix _pc-check-diagnostic-prefix-test _pc-check-safety-doc-pin _pc-check-safety-doc-pin-test _pc-worktree-setup-test _pc-gate-status-test _pc-check-tools-test _pc-enums-check _pc-enums-codegen-drift _pc-enums-codegen-drift-test \ _pc-manpages \ _pc-self-scan _pc-self-scan-headroom \ _pc-py-fmt _pc-py-typecheck _pc-py-test _pc-py-stubtest @@ -1512,7 +1531,7 @@ _ci-all: $(MAKE) -j --output-sync=target \ _ci-cargo-pipeline \ _ci-shellcheck _ci-markdown-lint _ci-toml-lint _ci-makefile-check \ - _ci-actionlint _ci-snapshot-anchors _ci-snapshot-anchors-test _ci-rustfmt-bail _ci-rustfmt-bail-test _ci-grammar-marker-sync _ci-grammar-marker-sync-test _ci-check-versions _ci-check-versions-test _ci-check-grammar-crate-test _ci-check-excluded-manifests _ci-check-excluded-manifests-test _ci-check-ruff-lockstep _ci-check-ruff-lockstep-test _ci-check-publish-metadata _ci-check-publish-metadata-test _ci-check-manpage-assets _ci-check-manpage-drift-test _ci-check-diagnostic-prefix _ci-check-diagnostic-prefix-test _ci-worktree-setup-test _ci-gate-status-test _ci-check-tools-test _ci-enums-check _ci-enums-codegen-drift _ci-enums-codegen-drift-test \ + _ci-actionlint _ci-snapshot-anchors _ci-snapshot-anchors-test _ci-rustfmt-bail _ci-rustfmt-bail-test _ci-grammar-marker-sync _ci-grammar-marker-sync-test _ci-check-versions _ci-check-versions-test _ci-check-grammar-crate-test _ci-check-excluded-manifests _ci-check-excluded-manifests-test _ci-check-ruff-lockstep _ci-check-ruff-lockstep-test _ci-check-publish-metadata _ci-check-publish-metadata-test _ci-check-manpage-assets _ci-check-manpage-drift-test _ci-check-diagnostic-prefix _ci-check-diagnostic-prefix-test _ci-check-safety-doc-pin _ci-check-safety-doc-pin-test _ci-worktree-setup-test _ci-gate-status-test _ci-check-tools-test _ci-enums-check _ci-enums-codegen-drift _ci-enums-codegen-drift-test \ _ci-py-fmt-check _ci-py-lint _ci-py-typecheck _ci-py-test _ci-py-stubtest # --------------------------------------------------------------------------- @@ -1552,6 +1571,8 @@ _ci-all: # ├── _pc-check-publish-metadata-test # ├── _pc-check-diagnostic-prefix # ├── _pc-check-diagnostic-prefix-test +# ├── _pc-check-safety-doc-pin +# ├── _pc-check-safety-doc-pin-test # ├── _pc-worktree-setup-test # ├── _pc-gate-status-test # ├── _pc-check-tools-test @@ -1675,6 +1696,12 @@ _pc-check-diagnostic-prefix: _pc-fmt _pc-check-diagnostic-prefix-test: _pc-fmt $(MAKE) check-diagnostic-prefix-test +_pc-check-safety-doc-pin: _pc-fmt + $(MAKE) check-safety-doc-pin + +_pc-check-safety-doc-pin-test: _pc-fmt + $(MAKE) check-safety-doc-pin-test + _pc-worktree-setup-test: _pc-fmt $(MAKE) worktree-setup-test @@ -1872,6 +1899,12 @@ _ci-check-diagnostic-prefix: _ci-check-diagnostic-prefix-test: $(MAKE) check-diagnostic-prefix-test +_ci-check-safety-doc-pin: + $(MAKE) check-safety-doc-pin + +_ci-check-safety-doc-pin-test: + $(MAKE) check-safety-doc-pin-test + _ci-worktree-setup-test: $(MAKE) worktree-setup-test diff --git a/big-code-analysis-py/src/node.rs b/big-code-analysis-py/src/node.rs index bd9f11539..4e8ba76b1 100644 --- a/big-code-analysis-py/src/node.rs +++ b/big-code-analysis-py/src/node.rs @@ -57,6 +57,38 @@ //! `tree_sitter::Tree`, `Node`, and `TreeCursor` are `Send + Sync` under //! the pinned `=0.26.12`, so the pyclasses are sendable (no `unsendable`) //! and compose with `ThreadPoolExecutor` fan-out like [`PyAst`] itself. +//! That version literal is a premise, not decoration — the layouts and +//! the `&mut self` on `Tree::edit` are what make the argument above +//! checkable, and only at a named release. `utils/check-safety-doc-pin.py` +//! fails the build when the pin moves and this line does not (#1057). +//! +//! ## What has been checked +//! +//! The invariants above were probed against a built `_native.abi3.so`, +//! not argued from the source alone. Nothing below broke; it is recorded +//! here because a future auditor of this `unsafe` block cannot cheaply +//! re-derive it (#1057). +//! +//! * `Node` is not GC-tracked. It declares no `__traverse__`, so +//! `Py_TPFLAGS_HAVE_GC` is unset and the collector has no mechanism to +//! clear a keep-alive; `PyAst` holds no Python-object fields, so a +//! cycle through one is not constructible either. +//! * No Python-reachable constructor exists — `Node.__new__` rejects, and +//! copy / deepcopy / pickle all raise — so a `Node` cannot exist +//! without an `Ast`. A mispaired handle is therefore unconstructible +//! from Python, which is why the pairing is enforced at the Rust module +//! boundary ([`owned`]) rather than by a runtime test that could only +//! pass vacuously. +//! * [`PyNodeWalk::next_node`] bumps the refcount per yielded node rather +//! than sharing the iterator's own handle, so a yielded node safely +//! outlives the walk that produced it. +//! * 3,000 parse-and-drop cycles under `gc.set_threshold(1, 1, 1)` with a +//! survivor pool, and 8 threads x 4,000 iterations plus 6 threads +//! racing `next()` on a shared `NodeWalk`: no crash, no +//! `PyBorrowMutError`. +//! +//! The residual risk is source-level — a future Rust change — rather than +//! input-level, which is what invariant 3 says from the other direction. use big_code_analysis::tree_sitter::{Node as TsNode, TreeCursor}; use pyo3::prelude::*; @@ -105,46 +137,154 @@ fn push_children_for_preorder( stack[first_child..].reverse(); } -/// A lazy handle to one node of a parsed [`Ast`](big_code_analysis::Ast). +/// Ownership boundary for the erased-lifetime pair (#1057). /// -/// Reached from [`Ast.root_node`](PyAst) or [`Ast.find`](PyAst), then walked -/// with `children` / `parent` / `walk()` / `descendants_by_kind()`. Holds a -/// strong reference to its `Ast`, so it stays valid even after the caller -/// drops every other reference to the parse. -#[pyclass(name = "Node", module = "big_code_analysis._native", frozen)] -pub(crate) struct PyNode { - // Keep-alive: owns the `Ast` (Tree + source) the erased node borrows. - ast: Py, - node: TsNode<'static>, -} +/// Both pyclasses pair an erased `Node<'static>` / `TreeCursor<'static>` +/// with the `Py` that keeps the owning tree alive, and that +/// pairing *is* [`detach`]'s precondition: a handle paired with the +/// wrong `Ast` outlives its tree and dereferences freed memory. Rust +/// field privacy alone does not enforce it — `PyNode { ast, node }` +/// compiles anywhere in the defining module, which is how three +/// construction sites accumulated on one precondition. Fields private +/// to *this* module make the struct literal unavailable to the +/// `#[pymethods]` blocks outside it, so every handle is built by one of +/// the constructors below and the pairing is structural rather than +/// reviewed. The accessors are the price. +/// +/// Everything except [`PyNode::wrap`] is `pub(super)`, not +/// `pub(crate)`: `wrap` is the one entry point another module needs +/// (`ast.rs` builds the root handle), while `node` + `rewrap` together +/// *are* the mispairing primitive — `a.rewrap(py, b.node())` pairs one +/// tree's node with another tree's keep-alive. Keeping them file-local +/// is what makes the boundary above true beyond this file. +mod owned { + use pyo3::prelude::*; + + use super::{PyAst, TreeCursor, TsNode, detach, push_children_for_preorder}; + + /// A lazy handle to one node of a parsed [`Ast`](big_code_analysis::Ast). + /// + /// Reached from [`Ast.root_node`](PyAst) or [`Ast.find`](PyAst), then walked + /// with `children` / `parent` / `walk()` / `descendants_by_kind()`. Holds a + /// strong reference to its `Ast`, so it stays valid even after the caller + /// drops every other reference to the parse. + #[pyclass(name = "Node", module = "big_code_analysis._native", frozen)] + pub(crate) struct PyNode { + // Keep-alive: owns the `Ast` (Tree + source) the erased node borrows. + ast: Py, + node: TsNode<'static>, + } + + impl PyNode { + /// Wrap `node`, erasing its lifetime brand and keeping its owning + /// `Ast` alive through `ast`. + /// + /// The one place the pairing is a *caller* obligation: `node` must + /// have come from the tree held by `ast`. Both callers are + /// [`PyAst::root_node`] / [`PyAst::find`], which pass a node from + /// the very `Ast` they are wrapping. Every other handle in this + /// module is derived from an existing one by [`rewrap`](Self::rewrap) + /// or [`PyNodeWalk::next_node`], which carry the pairing over + /// instead of restating it. + pub(crate) fn wrap(ast: Py, node: TsNode<'_>) -> Self { + // SAFETY: `ast` is the very `Ast` whose `Tree` produced `node`, and + // it is stored here for the whole life of the returned `PyNode`, so + // the erased node never outlives its tree. + let node = unsafe { detach(node) }; + Self { ast, node } + } -impl PyNode { - /// Wrap `node`, erasing its lifetime brand and keeping its owning `Ast` - /// alive through `ast`. + /// Re-wrap a sibling/child node discovered during navigation, sharing + /// this node's keep-alive `Ast`. The input already carries the erased + /// `'static` brand (it came from `self.node`), so no further `detach` + /// is needed — only a refcount bump on the `Ast`. + /// + /// Callers pass a node reached from `self.node` by a `tree_sitter` + /// navigation call, so it belongs to the same tree by construction. + pub(super) fn rewrap(&self, py: Python<'_>, node: TsNode<'static>) -> Self { + Self { + ast: self.ast.clone_ref(py), + node, + } + } + + /// The keep-alive handle to the `Ast` this node borrows from. + pub(super) fn ast(&self) -> &Py { + &self.ast + } + + /// The erased node. `Node<'static>` is `Copy`, so this hands out a + /// value rather than a borrow — it is only valid while `self` (and + /// therefore [`ast`](Self::ast)) is alive. + pub(super) fn node(&self) -> TsNode<'static> { + self.node + } + } + + /// Lazy pre-order iterator over a node and its descendants, returned by + /// [`PyNode::walk`]. + /// + /// Holds a work stack of not-yet-visited erased nodes plus the keep-alive + /// `Py`. Each `__next__` pops the next node, pushes its children + /// (leftmost on top), and yields the popped node — pre-order, one node at a + /// time, so traversal never materialises the whole subtree at once. + /// + /// The `TreeCursor` is held for the whole walk rather than built per + /// step, for [`push_children_for_preorder`]'s reason. It is branded + /// `'static` by the same erasure as the nodes it enumerates, and stays + /// valid for the same reason: `ast` keeps the owning parse alive. /// - /// `node` must have come from the tree held by `ast` (the only callers - /// are [`PyAst::root_node`] / [`PyAst::find`] and this module's own - /// navigation, which always pass a node from `self.ast`'s tree). - pub(crate) fn wrap(ast: Py, node: TsNode<'_>) -> Self { - // SAFETY: `ast` is the very `Ast` whose `Tree` produced `node`, and - // it is stored here for the whole life of the returned `PyNode`, so - // the erased node never outlives its tree. - let node = unsafe { detach(node) }; - Self { ast, node } - } - - /// Re-wrap a sibling/child node discovered during navigation, sharing - /// this node's keep-alive `Ast`. The input already carries the erased - /// `'static` brand (it came from `self.node`), so no further `detach` - /// is needed — only a refcount bump on the `Ast`. - fn rewrap(&self, py: Python<'_>, node: TsNode<'static>) -> Self { - Self { - ast: self.ast.clone_ref(py), - node, + /// Field order is load-bearing. Rust drops fields in declaration order, + /// so `cursor` is declared *before* the `ast` that keeps its tree alive + /// — otherwise the keep-alive would be released first and the doc claim + /// above would be false by construction. Today's `Drop for TreeCursor` + /// only frees the cursor's own stack and never reads the tree, so the + /// current order is not unsound; this makes the stated invariant hold + /// regardless, and survives a future tree-sitter destructor that does + /// touch it. (`Vec` has no drop glue, so `cursor` is the only + /// field that raises the question.) + #[pyclass(name = "NodeWalk", module = "big_code_analysis._native")] + pub(crate) struct PyNodeWalk { + cursor: TreeCursor<'static>, + stack: Vec>, + ast: Py, + } + + impl PyNodeWalk { + /// Root a walk at `node`, sharing its keep-alive `Ast`. + /// + /// Takes the `PyNode` rather than a `(Ast, node)` pair, so the + /// cursor, the seed node, and the keep-alive all come from one + /// already-valid handle: there is no argument to get wrong. + pub(super) fn rooted_at(node: &PyNode, py: Python<'_>) -> Self { + Self { + cursor: node.node.walk(), + stack: vec![node.node], + ast: node.ast.clone_ref(py), + } + } + + /// Pop the next node in pre-order, pushing its children first. + /// + /// Lives here rather than in `__next__` because it is the walk's + /// only reader of `stack` / `cursor` / `ast`, and the node it + /// yields is paired with the `Ast` the walk was rooted at. + pub(super) fn next_node(&mut self, py: Python<'_>) -> Option { + let node = self.stack.pop()?; + push_children_for_preorder(&mut self.stack, &mut self.cursor, node); + // Each yielded node carries its own keep-alive `Py`: it may + // outlive this iterator, so the per-node refcount bump is required + // for soundness, not an optimisation to hoist out of the loop. + Some(PyNode { + ast: self.ast.clone_ref(py), + node, + }) } } } +pub(crate) use owned::{PyNode, PyNodeWalk}; + #[pymethods] impl PyNode { /// The raw grammar kind of this node (e.g. `"function_item"`). @@ -154,7 +294,7 @@ impl PyNode { /// altered nodes (string literals, etc.). #[getter] fn kind(&self) -> &'static str { - self.node.kind() + self.node().kind() } /// py-tree-sitter-compatible alias for [`kind`](PyNode::kind). @@ -164,71 +304,71 @@ impl PyNode { /// py-tree-sitter spelling so an existing matcher ports over unchanged. #[getter] fn r#type(&self) -> &'static str { - self.node.kind() + self.node().kind() } /// The numeric grammar id behind [`kind`](PyNode::kind). #[getter] fn kind_id(&self) -> u16 { - self.node.kind_id() + self.node().kind_id() } /// Whether this node is a *named* grammar production (as opposed to an /// anonymous token such as punctuation or a keyword literal). #[getter] fn is_named(&self) -> bool { - self.node.is_named() + self.node().is_named() } /// Whether this node is an `ERROR` node produced by the parser. #[getter] fn is_error(&self) -> bool { - self.node.is_error() + self.node().is_error() } /// Whether this node is a zero-width `MISSING` node the parser inserted /// to recover from a syntax error. #[getter] fn is_missing(&self) -> bool { - self.node.is_missing() + self.node().is_missing() } /// Whether this node is an `extra` (a node that may appear anywhere, /// such as a comment in most grammars). #[getter] fn is_extra(&self) -> bool { - self.node.is_extra() + self.node().is_extra() } /// Whether this node or any node beneath it is an error or missing node. #[getter] fn has_error(&self) -> bool { - self.node.has_error() + self.node().has_error() } /// Start byte offset (inclusive) into [`PyAst::source`]. #[getter] fn start_byte(&self) -> usize { - self.node.start_byte() + self.node().start_byte() } /// End byte offset (exclusive) into [`PyAst::source`]. #[getter] fn end_byte(&self) -> usize { - self.node.end_byte() + self.node().end_byte() } /// 0-based `(row, column)` of the node's start — py-tree-sitter parity. #[getter] fn start_point(&self) -> (usize, usize) { - let p = self.node.start_position(); + let p = self.node().start_position(); (p.row, p.column) } /// 0-based `(row, column)` of the node's end — py-tree-sitter parity. #[getter] fn end_point(&self) -> (usize, usize) { - let p = self.node.end_position(); + let p = self.node().end_position(); (p.row, p.column) } @@ -236,28 +376,28 @@ impl PyNode { /// vocabulary (`start_line == start_point[0] + 1`). #[getter] fn start_line(&self) -> usize { - self.node.start_position().row + 1 + self.node().start_position().row + 1 } /// 1-based end line (`end_line == end_point[0] + 1`). #[getter] fn end_line(&self) -> usize { - self.node.end_position().row + 1 + self.node().end_position().row + 1 } /// The node's span as the same 1-based dict `dump()` emits: /// `{start_line, start_col, end_line, end_col, start_byte, end_byte}`. #[getter] fn span<'py>(&self, py: Python<'py>) -> PyResult> { - let start = self.node.start_position(); - let end = self.node.end_position(); + let start = self.node().start_position(); + let end = self.node().end_position(); let dict = PyDict::new(py); dict.set_item("start_line", start.row + 1)?; dict.set_item("start_col", start.column + 1)?; dict.set_item("end_line", end.row + 1)?; dict.set_item("end_col", end.column + 1)?; - dict.set_item("start_byte", self.node.start_byte())?; - dict.set_item("end_byte", self.node.end_byte())?; + dict.set_item("start_byte", self.node().start_byte())?; + dict.set_item("end_byte", self.node().end_byte())?; Ok(dict) } @@ -266,8 +406,8 @@ impl PyNode { /// parent reaches without a field. #[getter] fn field_name(&self) -> Option<&'static str> { - let parent = self.node.parent()?; - let id = self.node.id(); + let parent = self.node().parent()?; + let id = self.node().id(); let mut cursor = parent.walk(); if !cursor.goto_first_child() { return None; @@ -285,20 +425,20 @@ impl PyNode { /// The number of direct children (named and anonymous). #[getter] fn child_count(&self) -> usize { - self.node.child_count() + self.node().child_count() } /// The number of direct *named* children. #[getter] fn named_child_count(&self) -> usize { - self.node.named_child_count() + self.node().named_child_count() } /// All direct children (named and anonymous), in document order. #[getter] fn children(&self, py: Python<'_>) -> Vec { - let mut cursor = self.node.walk(); - self.node + let mut cursor = self.node().walk(); + self.node() .children(&mut cursor) .map(|c| self.rewrap(py, c)) .collect() @@ -307,8 +447,8 @@ impl PyNode { /// The direct *named* children, in document order. #[getter] fn named_children(&self, py: Python<'_>) -> Vec { - let mut cursor = self.node.walk(); - self.node + let mut cursor = self.node().walk(); + self.node() .named_children(&mut cursor) .map(|c| self.rewrap(py, c)) .collect() @@ -317,31 +457,31 @@ impl PyNode { /// This node's parent, or `None` at the root. #[getter] fn parent(&self, py: Python<'_>) -> Option { - self.node.parent().map(|p| self.rewrap(py, p)) + self.node().parent().map(|p| self.rewrap(py, p)) } /// The next sibling (named or anonymous), or `None`. #[getter] fn next_sibling(&self, py: Python<'_>) -> Option { - self.node.next_sibling().map(|n| self.rewrap(py, n)) + self.node().next_sibling().map(|n| self.rewrap(py, n)) } /// The previous sibling (named or anonymous), or `None`. #[getter] fn prev_sibling(&self, py: Python<'_>) -> Option { - self.node.prev_sibling().map(|n| self.rewrap(py, n)) + self.node().prev_sibling().map(|n| self.rewrap(py, n)) } /// The next *named* sibling, or `None`. #[getter] fn next_named_sibling(&self, py: Python<'_>) -> Option { - self.node.next_named_sibling().map(|n| self.rewrap(py, n)) + self.node().next_named_sibling().map(|n| self.rewrap(py, n)) } /// The previous *named* sibling, or `None`. #[getter] fn prev_named_sibling(&self, py: Python<'_>) -> Option { - self.node.prev_named_sibling().map(|n| self.rewrap(py, n)) + self.node().prev_named_sibling().map(|n| self.rewrap(py, n)) } /// The child at `index` (named and anonymous children counted), or @@ -350,7 +490,7 @@ impl PyNode { fn child(&self, py: Python<'_>, index: usize) -> Option { u32::try_from(index) .ok() - .and_then(|i| self.node.child(i)) + .and_then(|i| self.node().child(i)) .map(|c| self.rewrap(py, c)) } @@ -359,14 +499,14 @@ impl PyNode { fn named_child(&self, py: Python<'_>, index: usize) -> Option { u32::try_from(index) .ok() - .and_then(|i| self.node.named_child(i)) + .and_then(|i| self.node().named_child(i)) .map(|c| self.rewrap(py, c)) } /// The first child reached through the grammar field `name`, or `None`. #[pyo3(signature = (name, /))] fn child_by_field_name(&self, py: Python<'_>, name: &str) -> Option { - self.node + self.node() .child_by_field_name(name) .map(|c| self.rewrap(py, c)) } @@ -374,8 +514,8 @@ impl PyNode { /// Every child reached through the grammar field `name`, in order. #[pyo3(signature = (name, /))] fn children_by_field_name(&self, py: Python<'_>, name: &str) -> Vec { - let mut cursor = self.node.walk(); - self.node + let mut cursor = self.node().walk(); + self.node() .children_by_field_name(name, &mut cursor) .map(|c| self.rewrap(py, c)) .collect() @@ -387,7 +527,7 @@ impl PyNode { fn field_name_for_child(&self, index: usize) -> Option<&'static str> { u32::try_from(index) .ok() - .and_then(|i| self.node.field_name_for_child(i)) + .and_then(|i| self.node().field_name_for_child(i)) } /// This node's source text, the `source[start_byte:end_byte]` slice of @@ -398,11 +538,11 @@ impl PyNode { /// is hot. #[getter] fn text<'py>(&self, py: Python<'py>) -> Bound<'py, PyBytes> { - let source = self.ast.bind(py).get().ast_ref().source(); + let source = self.ast().bind(py).get().ast_ref().source(); // Tree-sitter guarantees the span lies within the source, but slice // defensively rather than risk a panic on a malformed input. let slice = source - .get(self.node.start_byte()..self.node.end_byte()) + .get(self.node().start_byte()..self.node().end_byte()) .unwrap_or(&[]); PyBytes::new(py, slice) } @@ -414,11 +554,7 @@ impl PyNode { /// early without materialising the whole subtree — the memory payoff of /// the lazy surface. Mirrors the Rust `Node::preorder` (#728). fn walk(&self, py: Python<'_>) -> PyNodeWalk { - PyNodeWalk { - cursor: self.node.walk(), - stack: vec![self.node], - ast: self.ast.clone_ref(py), - } + PyNodeWalk::rooted_at(self, py) } /// Collect every node in this subtree (this node included) whose @@ -433,9 +569,9 @@ impl PyNode { #[allow(clippy::needless_pass_by_value)] fn descendants_by_kind(&self, py: Python<'_>, kinds: Vec) -> Vec { let mut out = Vec::new(); - let mut stack = vec![self.node]; + let mut stack = vec![self.node()]; // One cursor for the whole subtree, not one per visited node. - let mut cursor = self.node.walk(); + let mut cursor = self.node().walk(); while let Some(node) = stack.pop() { // Only matches pay the `rewrap` (a keep-alive refcount bump), so // a selective filter does not allocate a handle per visited node. @@ -455,9 +591,9 @@ impl PyNode { /// deallocated, so its address cannot be recycled for a *different* /// `Ast`. Pointer equality therefore implies the same `Ast` instance. fn __eq__(&self, other: &Bound<'_, PyAny>) -> bool { - other - .extract::>() - .is_ok_and(|o| self.ast.as_ptr() == o.ast.as_ptr() && self.node.id() == o.node.id()) + other.extract::>().is_ok_and(|o| { + self.ast().as_ptr() == o.ast().as_ptr() && self.node().id() == o.node().id() + }) } /// Hash consistent with [`__eq__`](PyNode::__eq__): the `(Ast object @@ -468,53 +604,24 @@ impl PyNode { fn __hash__(&self) -> isize { use std::hash::{Hash, Hasher}; let mut hasher = std::collections::hash_map::DefaultHasher::new(); - (self.ast.as_ptr() as usize).hash(&mut hasher); - self.node.id().hash(&mut hasher); + (self.ast().as_ptr() as usize).hash(&mut hasher); + self.node().id().hash(&mut hasher); hasher.finish() as isize } fn __repr__(&self) -> String { - let start = self.node.start_position(); + let start = self.node().start_position(); format!( "Node(kind='{}', start_point=({}, {}), byte_range={}..{})", - self.node.kind(), + self.node().kind(), start.row, start.column, - self.node.start_byte(), - self.node.end_byte(), + self.node().start_byte(), + self.node().end_byte(), ) } } -/// Lazy pre-order iterator over a node and its descendants, returned by -/// [`PyNode::walk`]. -/// -/// Holds a work stack of not-yet-visited erased nodes plus the keep-alive -/// `Py`. Each `__next__` pops the next node, pushes its children -/// (leftmost on top), and yields the popped node — pre-order, one node at a -/// time, so traversal never materialises the whole subtree at once. -/// -/// The `TreeCursor` is held for the whole walk rather than built per -/// step, for [`push_children_for_preorder`]'s reason. It is branded -/// `'static` by the same erasure as the nodes it enumerates, and stays -/// valid for the same reason: `ast` keeps the owning parse alive. -/// -/// Field order is load-bearing. Rust drops fields in declaration order, -/// so `cursor` is declared *before* the `ast` that keeps its tree alive -/// — otherwise the keep-alive would be released first and the doc claim -/// above would be false by construction. Today's `Drop for TreeCursor` -/// only frees the cursor's own stack and never reads the tree, so the -/// current order is not unsound; this makes the stated invariant hold -/// regardless, and survives a future tree-sitter destructor that does -/// touch it. (`Vec` has no drop glue, so `cursor` is the only -/// field that raises the question.) -#[pyclass(name = "NodeWalk", module = "big_code_analysis._native")] -pub(crate) struct PyNodeWalk { - cursor: TreeCursor<'static>, - stack: Vec>, - ast: Py, -} - #[pymethods] impl PyNodeWalk { fn __iter__(slf: PyRef<'_, Self>) -> PyRef<'_, Self> { @@ -522,14 +629,6 @@ impl PyNodeWalk { } fn __next__(&mut self, py: Python<'_>) -> Option { - let node = self.stack.pop()?; - push_children_for_preorder(&mut self.stack, &mut self.cursor, node); - // Each yielded node carries its own keep-alive `Py`: it may - // outlive this iterator, so the per-node refcount bump is required - // for soundness, not an optimisation to hoist out of the loop. - Some(PyNode { - ast: self.ast.clone_ref(py), - node, - }) + self.next_node(py) } } diff --git a/utils/check-safety-doc-pin-test.py b/utils/check-safety-doc-pin-test.py new file mode 100644 index 000000000..1d79533cd --- /dev/null +++ b/utils/check-safety-doc-pin-test.py @@ -0,0 +1,216 @@ +#!/usr/bin/env python3 +"""Tests for check-safety-doc-pin.py. + +Three kinds of test, matching the check-diagnostic-prefix-test.py +pattern: + +* Unit tests over the two extractors, weighted toward the false-clean + direction: a module doc the scanner fails to read reports no citations + and, if the "no citation" branch were ever softened, would report a + clean tree. The stop condition (where the ``//!`` block ends) is what + decides how much of the file the gate can see, so it is pinned in both + directions — a line that must not end the block, and one that must. +* ``main()`` tests over a synthetic repository root, covering the clean, + stale, and citation-dropped branches plus the unreadable-manifest exit. +* A smoke test running the real gate against the real repository, + asserting a clean tree reports OK. + +Run with: + python3 -m unittest -q utils/check-safety-doc-pin-test.py +""" + +from __future__ import annotations + +import contextlib +import importlib.util +import io +import pathlib +import subprocess +import sys +import tempfile +import unittest +from unittest import mock + +UTILS_DIR = pathlib.Path(__file__).resolve().parent +REPO_ROOT = UTILS_DIR.parent +SCRIPT_SRC = UTILS_DIR / "check-safety-doc-pin.py" + + +def _load_module(): # type: ignore[no-untyped-def] + spec = importlib.util.spec_from_file_location("check_safety_doc_pin", SCRIPT_SRC) + assert spec is not None and spec.loader is not None + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +GATE = _load_module() + +# A module doc shaped like node.rs's: prose, a blank `//!` line, and the +# sendability claim carrying the version literal. +DOC_SOURCE = """\ +//! Lazy `Node` handle over the tree retained by [`PyAst`]. +//! +//! `tree_sitter::Tree`, `Node`, and `TreeCursor` are `Send + Sync` under +//! the pinned `=0.26.12`, so the pyclasses are sendable. + +use pyo3::prelude::*; + +/// Item doc citing `=9.9.9`, deliberately out of scope. +fn f() {} +""" + + +class ModuleDocLines(unittest.TestCase): + """What the gate can and cannot see.""" + + def test_collects_the_leading_block_only(self) -> None: + lines = GATE.module_doc_lines(DOC_SOURCE) + self.assertEqual([n for n, _ in lines], [1, 2, 3, 4]) + self.assertTrue(lines[3][1].endswith("the pyclasses are sendable.")) + + def test_item_doc_below_is_out_of_scope(self) -> None: + # The `///` citation must not be collected: the gate's contract + # is the module-level soundness argument, and an item doc may + # legitimately name another crate's version. + bodies = [body for _, body in GATE.module_doc_lines(DOC_SOURCE)] + self.assertFalse(any("9.9.9" in body for body in bodies)) + + def test_inner_attribute_does_not_end_the_block(self) -> None: + # A `#![allow(...)]` between doc paragraphs must not truncate the + # scan: everything after it would silently stop being gated. + source = "//! first\n#![allow(clippy::pedantic)]\n//! `=1.2.3`\n\nfn f() {}\n" + self.assertEqual( + GATE.cited_versions(GATE.module_doc_lines(source)), [(3, "1.2.3")] + ) + + def test_code_ends_the_block(self) -> None: + source = "//! `=1.2.3`\nuse pyo3::prelude::*;\n//! `=4.5.6`\n" + self.assertEqual( + GATE.cited_versions(GATE.module_doc_lines(source)), [(1, "1.2.3")] + ) + + +class CitedVersions(unittest.TestCase): + def test_reads_the_backticked_literal(self) -> None: + self.assertEqual( + GATE.cited_versions(GATE.module_doc_lines(DOC_SOURCE)), [(4, "0.26.12")] + ) + + def test_unbackticked_version_is_ignored(self) -> None: + # Prose such as "since 0.26.0 the cursor …" is not a citation of + # the pin and must not fail the gate. + source = "//! behaviour changed in =0.26.0 upstream\n" + self.assertEqual(GATE.cited_versions(GATE.module_doc_lines(source)), []) + + def test_every_citation_is_reported(self) -> None: + source = "//! `=1.2.3` and later `=1.2.4`\n" + self.assertEqual( + GATE.cited_versions(GATE.module_doc_lines(source)), + [(1, "1.2.3"), (1, "1.2.4")], + ) + + +class PinnedVersion(unittest.TestCase): + def test_string_form(self) -> None: + manifest = '[workspace.dependencies]\ntree-sitter = "=0.26.12"\n' + self.assertEqual(GATE.pinned_version(manifest), "0.26.12") + + def test_table_form(self) -> None: + manifest = ( + "[workspace.dependencies]\n" + 'tree-sitter = { version = "=0.26.12", default-features = false }\n' + ) + self.assertEqual(GATE.pinned_version(manifest), "0.26.12") + + def test_caret_range_is_rejected(self) -> None: + manifest = '[workspace.dependencies]\ntree-sitter = "0.26"\n' + with self.assertRaises(GATE.ManifestError): + GATE.pinned_version(manifest) + + def test_missing_entry_is_rejected(self) -> None: + manifest = '[workspace.dependencies]\ntree-sitter-c = "=0.24.1"\n' + with self.assertRaises(GATE.ManifestError): + GATE.pinned_version(manifest) + + +@contextlib.contextmanager +def _synthetic_repo(manifest: str, doc_source: str): # type: ignore[no-untyped-def] + """Point the gate at a throwaway root and capture its stderr/stdout.""" + with tempfile.TemporaryDirectory() as tmp: + root = pathlib.Path(tmp) + (root / "Cargo.toml").write_text(manifest, encoding="utf-8") + doc = root / "big-code-analysis-py" / "src" / "node.rs" + doc.parent.mkdir(parents=True) + doc.write_text(doc_source, encoding="utf-8") + out, err = io.StringIO(), io.StringIO() + with ( + mock.patch.object(GATE, "REPO_ROOT", root), + mock.patch.object(GATE, "ROOT_MANIFEST", root / "Cargo.toml"), + mock.patch.object(GATE, "SAFETY_DOC", doc), + contextlib.redirect_stdout(out), + contextlib.redirect_stderr(err), + ): + yield lambda: (GATE.main(), out.getvalue(), err.getvalue()) + + +PINNED = '[workspace.dependencies]\ntree-sitter = "=0.26.12"\n' + + +class Main(unittest.TestCase): + def test_matching_citation_passes(self) -> None: + with _synthetic_repo(PINNED, DOC_SOURCE) as run: + code, out, err = run() + self.assertEqual(code, 0) + self.assertIn("safety-doc-pin: OK", out) + self.assertEqual(err, "") + + def test_stale_citation_fails_and_names_the_line(self) -> None: + stale = DOC_SOURCE.replace("=0.26.12", "=0.26.9") + with _synthetic_repo(PINNED, stale) as run: + code, _out, err = run() + self.assertEqual(code, 1) + self.assertIn("node.rs:4: cites `=0.26.9`, pin is `=0.26.12`", err) + # The remediation must send the reader back to the argument, not + # only to the literal — re-reading it is the point of the gate. + self.assertIn("Re-read that", err) + + def test_dropping_the_citation_fails(self) -> None: + symbolic = DOC_SOURCE.replace("the pinned `=0.26.12`", "the pinned version") + with _synthetic_repo(PINNED, symbolic) as run: + code, _out, err = run() + self.assertEqual(code, 1) + # Spelled out rather than matched loosely: the crate name is + # not itself a version requirement, so `=tree-sitter` would be + # a nonsense literal for the reader to go looking for. + self.assertIn("no longer cites a `tree-sitter` version", err) + self.assertIn("`=0.26.12`", err) + + def test_unreadable_pin_exits_two(self) -> None: + with _synthetic_repo("[workspace.dependencies]\n", DOC_SOURCE) as run: + code, _out, err = run() + self.assertEqual(code, 2) + self.assertIn("could not read the tree-sitter pin", err) + + def test_missing_safety_doc_exits_two(self) -> None: + with _synthetic_repo(PINNED, DOC_SOURCE) as run: + GATE.SAFETY_DOC.unlink() + code, _out, err = run() + self.assertEqual(code, 2) + self.assertIn("could not read", err) + + +class RealRepository(unittest.TestCase): + def test_gate_passes_on_the_checked_in_tree(self) -> None: + result = subprocess.run( + [sys.executable, str(SCRIPT_SRC)], + capture_output=True, + text=True, + check=False, + ) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn("safety-doc-pin: OK", result.stdout) + + +if __name__ == "__main__": + unittest.main() diff --git a/utils/check-safety-doc-pin.py b/utils/check-safety-doc-pin.py new file mode 100755 index 000000000..25fcec1ea --- /dev/null +++ b/utils/check-safety-doc-pin.py @@ -0,0 +1,182 @@ +#!/usr/bin/env python3 +"""check-safety-doc-pin + +Guard that the ``tree-sitter`` version cited by the soundness argument +in ``big-code-analysis-py/src/node.rs`` is the version the workspace +actually pins in the root ``Cargo.toml``. + +That module doc is the canonical justification for the workspace's only +sanctioned ``unsafe`` block (AGENTS.md, "Rust conventions"), and part of +its argument — that ``Tree``/``Node``/``TreeCursor`` are ``Send + Sync``, +that ``Node<'tree>`` is ``(ffi::TSNode, PhantomData<&'tree ()>)``, that +``Tree::edit`` takes ``&mut self`` — is a claim about a *specific* +release. The version literal is what makes the claim checkable: a reader +can open that version of the crate and confirm it. When the pin moves and +the literal does not, the argument keeps reading as verified while +silently describing a crate nobody is compiling against. That is #1057: +the doc said ``=0.26.9`` two patch releases after the pin had moved. + +The gate does not verify the *argument* — no gate can. It forces a diff +on that line at bump time, which is the prompt to re-read the argument. + +Two directions are enforced: + +* every backticked ``=X.Y.Z`` literal in the module doc equals the pin; +* at least one such literal is present. Dropping the literal in favour of + a version-free phrase ("under the pinned version") was considered and + declined in #1057: it removes the stale number by making the claim + unfalsifiable at read time, which moves the staleness from visible to + invisible rather than fixing it. + +Scope is deliberately one file. Two other places name this pin in +prose — the runtime-exemption rationale in ``AGENTS.md`` and the same +rationale echoed in ``utils/check-excluded-manifests.py`` — but both sit +in text that also cites *grammar* pins illustratively (``=0.23.5``, +``=0.26.10``), so a literal-scanning rule there would fail on versions +that are supposed to differ. In ``node.rs`` an ``=X.Y.Z`` literal can +only mean the tree-sitter pin, which is what makes the rule crisp here. + +See AGENTS.md "Validation gates" for the policy this enforces. +""" + +from __future__ import annotations + +import pathlib +import re +import sys +import tomllib + +# `parents[1]`, not `parent`: these gates live in `utils/` but every +# path they read is anchored at the repository root. +REPO_ROOT = pathlib.Path(__file__).resolve().parents[1] +ROOT_MANIFEST = REPO_ROOT / "Cargo.toml" +SAFETY_DOC = REPO_ROOT / "big-code-analysis-py" / "src" / "node.rs" + +# The dependency whose pin the safety argument is about. +PINNED_CRATE = "tree-sitter" + +# A backticked exact-version requirement, e.g. `=0.26.12`. Backticks are +# required so ordinary prose containing a version-shaped substring is not +# swept in. +CITED_VERSION = re.compile(r"`=(\d+\.\d+\.\d+)`") + + +class ManifestError(Exception): + """The root manifest did not carry a readable ``tree-sitter`` pin.""" + + +def module_doc_lines(text: str) -> list[tuple[int, str]]: + """Return the file's leading ``//!`` block as ``(line_no, body)``. + + Line numbers are 1-based and file-relative, so a failure can name the + offending line directly. Inner attributes (``#![…]``) and blank lines + are skipped rather than terminating the block, since either may be + interleaved with a module doc; the first other non-empty line ends + it, which keeps ``///`` item docs and code out of scope. + """ + out: list[tuple[int, str]] = [] + for line_no, raw in enumerate(text.splitlines(), start=1): + line = raw.strip() + if line.startswith("//!"): + out.append((line_no, line[len("//!") :].strip())) + elif not line or line.startswith("#!["): + continue + else: + break + return out + + +def cited_versions(doc: list[tuple[int, str]]) -> list[tuple[int, str]]: + """Return every ``(line_no, version)`` the module doc cites.""" + return [ + (line_no, match.group(1)) + for line_no, body in doc + for match in CITED_VERSION.finditer(body) + ] + + +def pinned_version(manifest_text: str) -> str: + """Return the exact version ``[workspace.dependencies]`` pins. + + Accepts both the string form (``tree-sitter = "=0.26.12"``) and the + table form (``tree-sitter = { version = "=0.26.12", … }``), so a + future manifest restructure does not read as a missing pin. + + Raises :class:`ManifestError` if the entry is absent or is not an + exact ``=X.Y.Z`` requirement — a caret range would make the cited + literal unverifiable, which is the same defect from the other end. + """ + data = tomllib.loads(manifest_text) + entry = data.get("workspace", {}).get("dependencies", {}).get(PINNED_CRATE) + if isinstance(entry, dict): + entry = entry.get("version") + if not isinstance(entry, str): + raise ManifestError( + f"[workspace.dependencies].{PINNED_CRATE} is missing or is not " + f"a version string (got {entry!r})" + ) + match = re.fullmatch(r"=\s*(\d+\.\d+\.\d+)", entry.strip()) + if match is None: + raise ManifestError( + f"[workspace.dependencies].{PINNED_CRATE} = {entry!r} is not an " + f"exact `=X.Y.Z` pin" + ) + return match.group(1) + + +def main() -> int: + try: + pin = pinned_version(ROOT_MANIFEST.read_text(encoding="utf-8")) + except (OSError, ManifestError) as exc: + sys.stderr.write( + f"error: could not read the {PINNED_CRATE} pin from " + f"{ROOT_MANIFEST}: {exc}\n" + ) + return 2 + + try: + doc = module_doc_lines(SAFETY_DOC.read_text(encoding="utf-8")) + except OSError as exc: + sys.stderr.write(f"error: could not read {SAFETY_DOC}: {exc}\n") + return 2 + + rel = SAFETY_DOC.relative_to(REPO_ROOT) + cited = cited_versions(doc) + + if not cited: + sys.stderr.write( + f"error: {rel} no longer cites a `{PINNED_CRATE}` version in " + f"its module doc\n" + f" The soundness argument for this workspace's only\n" + f" sanctioned `unsafe` block makes claims about a\n" + f" specific release; naming it is what lets a reader\n" + f" check them. Restore the literal as `={pin}` (see\n" + f" #1057 — the version-free phrasing was declined).\n" + ) + return 1 + + stale = [(line_no, version) for line_no, version in cited if version != pin] + if stale: + sys.stderr.write( + f"error: {rel} cites a {PINNED_CRATE} version the workspace " + f"does not pin\n" + ) + for line_no, version in stale: + sys.stderr.write(f" {rel}:{line_no}: cites `={version}`, pin is `={pin}`\n") + sys.stderr.write( + f"\nThe module doc is the canonical soundness argument for the\n" + f"only `unsafe` block in this workspace, and it reasons about\n" + f"the pinned {PINNED_CRATE} release specifically. Re-read that\n" + f"argument against {pin} — `Tree(NonNull)`,\n" + f"`Node<'tree>(ffi::TSNode, PhantomData<&'tree ()>)`,\n" + f"`Tree::edit(&mut self)`, `Send + Sync` — and then update the\n" + f"literal. See #1057.\n" + ) + return 1 + + print(f"safety-doc-pin: OK ({PINNED_CRATE} `={pin}`, {len(cited)} citation(s))") + return 0 + + +if __name__ == "__main__": + sys.exit(main())