From 4a1e279fc67fbc2c1de9654de4dbdd713b0d52e6 Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Tue, 7 Jul 2026 06:40:42 -0600 Subject: [PATCH] fix: exclude PHP scalar type-hint/cast keywords from ast --kind string tree-sitter-php's primitive_type production (scalar type-hints like `string $x` and `: string` return types) and cast_type production (`(string) $x`) both lex the `string` keyword as an anonymous grammar token whose type string is identical to the named `string` literal node type -- the same collision as TypeScript's predefined_type (#1729). Without a named-node guard, `codegraph ast --kind string` misclassified these keyword occurrences as string literals. Fixes both engines: - Native: add `requires_named_node` to `LangAstConfig`, checked in `classify_ast_node` via `node.is_named()`; set true only for PHP. - WASM/TS: extend `astRequiresNamedNode()` (already scoped to JS/TS/TSX from #1729) to also cover `php`, reusing the existing `resolveAstKind()` guard threaded through all three production call sites. Verified non-tautological: reverting each side's guard and rebuilding reproduces the false positives (7 rows instead of 3) that the new tests catch, on both the WASM and native paths. Impact: 1 functions changed, 9 affected --- .../codegraph-core/src/extractors/helpers.rs | 52 +++++- src/ast-analysis/rules/index.ts | 43 +++-- .../visitors/ast-store-visitor.ts | 5 +- tests/engines/ast-parity.test.ts | 3 +- tests/parsers/ast-all-langs.test.ts | 9 + tests/parsers/ast-nodes.test.ts | 168 ++++++++++++++++++ 6 files changed, 263 insertions(+), 17 deletions(-) diff --git a/crates/codegraph-core/src/extractors/helpers.rs b/crates/codegraph-core/src/extractors/helpers.rs index 6e46ec44c..638cc2ad3 100644 --- a/crates/codegraph-core/src/extractors/helpers.rs +++ b/crates/codegraph-core/src/extractors/helpers.rs @@ -211,6 +211,13 @@ pub struct LangAstConfig { /// Single-char prefixes that can appear before string quotes (e.g. `r`, `b`, `f`, `u` for Python). /// Multi-char combos like `rb`, `fr` are handled by stripping each char in sequence. pub string_prefixes: &'static [char], + /// When true, a node is only classified if `node.is_named()` — guards against + /// anonymous grammar tokens whose `kind()` string collides with a named node + /// type mapped above (e.g. PHP's `primitive_type`/`cast_type` productions lex + /// the `string` scalar type-hint keyword as an unnamed token identical to the + /// named `string` literal node type). Mirrors the WASM-side + /// `astRequiresNamedNode()` guard in `ast-analysis/rules/index.ts`. + pub requires_named_node: bool, } // ── Per-language configs ───────────────────────────────────────────────────── @@ -223,6 +230,7 @@ pub const PYTHON_AST_CONFIG: LangAstConfig = LangAstConfig { regex_types: &[], quote_chars: &['\'', '"'], string_prefixes: &['r', 'b', 'f', 'u', 'R', 'B', 'F', 'U'], + requires_named_node: false, }; pub const GO_AST_CONFIG: LangAstConfig = LangAstConfig { @@ -233,6 +241,7 @@ pub const GO_AST_CONFIG: LangAstConfig = LangAstConfig { regex_types: &[], quote_chars: &['"', '`'], string_prefixes: &[], + requires_named_node: false, }; pub const RUST_AST_CONFIG: LangAstConfig = LangAstConfig { @@ -243,6 +252,7 @@ pub const RUST_AST_CONFIG: LangAstConfig = LangAstConfig { regex_types: &[], quote_chars: &['"'], string_prefixes: &[], + requires_named_node: false, }; pub const JAVA_AST_CONFIG: LangAstConfig = LangAstConfig { @@ -253,6 +263,7 @@ pub const JAVA_AST_CONFIG: LangAstConfig = LangAstConfig { regex_types: &[], quote_chars: &['"'], string_prefixes: &[], + requires_named_node: false, }; pub const CSHARP_AST_CONFIG: LangAstConfig = LangAstConfig { @@ -263,6 +274,7 @@ pub const CSHARP_AST_CONFIG: LangAstConfig = LangAstConfig { regex_types: &[], quote_chars: &['"'], string_prefixes: &[], + requires_named_node: false, }; pub const RUBY_AST_CONFIG: LangAstConfig = LangAstConfig { @@ -273,6 +285,7 @@ pub const RUBY_AST_CONFIG: LangAstConfig = LangAstConfig { regex_types: &["regex"], quote_chars: &['\'', '"'], string_prefixes: &[], + requires_named_node: false, }; pub const PHP_AST_CONFIG: LangAstConfig = LangAstConfig { @@ -283,6 +296,7 @@ pub const PHP_AST_CONFIG: LangAstConfig = LangAstConfig { regex_types: &[], quote_chars: &['\'', '"'], string_prefixes: &[], + requires_named_node: true, }; pub const C_AST_CONFIG: LangAstConfig = LangAstConfig { @@ -293,6 +307,7 @@ pub const C_AST_CONFIG: LangAstConfig = LangAstConfig { regex_types: &[], quote_chars: &['"'], string_prefixes: &[], + requires_named_node: false, }; pub const CPP_AST_CONFIG: LangAstConfig = LangAstConfig { @@ -303,6 +318,7 @@ pub const CPP_AST_CONFIG: LangAstConfig = LangAstConfig { regex_types: &[], quote_chars: &['"'], string_prefixes: &['L', 'u', 'U', 'R'], + requires_named_node: false, }; /// CUDA is a C++ superset; the tree-sitter-cuda grammar extends C++ with @@ -317,6 +333,7 @@ pub const CUDA_AST_CONFIG: LangAstConfig = LangAstConfig { regex_types: &[], quote_chars: &['"'], string_prefixes: &['L', 'u', 'U', 'R'], + requires_named_node: false, }; pub const KOTLIN_AST_CONFIG: LangAstConfig = LangAstConfig { @@ -327,6 +344,7 @@ pub const KOTLIN_AST_CONFIG: LangAstConfig = LangAstConfig { regex_types: &[], quote_chars: &['"'], string_prefixes: &[], + requires_named_node: false, }; pub const SWIFT_AST_CONFIG: LangAstConfig = LangAstConfig { @@ -337,6 +355,7 @@ pub const SWIFT_AST_CONFIG: LangAstConfig = LangAstConfig { regex_types: &[], quote_chars: &['"'], string_prefixes: &[], + requires_named_node: false, }; pub const SCALA_AST_CONFIG: LangAstConfig = LangAstConfig { @@ -347,6 +366,7 @@ pub const SCALA_AST_CONFIG: LangAstConfig = LangAstConfig { regex_types: &[], quote_chars: &['"'], string_prefixes: &[], + requires_named_node: false, }; pub const BASH_AST_CONFIG: LangAstConfig = LangAstConfig { @@ -357,6 +377,7 @@ pub const BASH_AST_CONFIG: LangAstConfig = LangAstConfig { regex_types: &[], quote_chars: &['"', '\''], string_prefixes: &[], + requires_named_node: false, }; pub const ELIXIR_AST_CONFIG: LangAstConfig = LangAstConfig { @@ -367,6 +388,7 @@ pub const ELIXIR_AST_CONFIG: LangAstConfig = LangAstConfig { regex_types: &["sigil"], quote_chars: &['"'], string_prefixes: &[], + requires_named_node: false, }; pub const LUA_AST_CONFIG: LangAstConfig = LangAstConfig { @@ -377,6 +399,7 @@ pub const LUA_AST_CONFIG: LangAstConfig = LangAstConfig { regex_types: &[], quote_chars: &['\'', '"'], string_prefixes: &[], + requires_named_node: false, }; pub const DART_AST_CONFIG: LangAstConfig = LangAstConfig { @@ -387,6 +410,7 @@ pub const DART_AST_CONFIG: LangAstConfig = LangAstConfig { regex_types: &[], quote_chars: &['\'', '"'], string_prefixes: &[], + requires_named_node: false, }; pub const ZIG_AST_CONFIG: LangAstConfig = LangAstConfig { @@ -397,6 +421,7 @@ pub const ZIG_AST_CONFIG: LangAstConfig = LangAstConfig { regex_types: &[], quote_chars: &['"'], string_prefixes: &[], + requires_named_node: false, }; pub const HASKELL_AST_CONFIG: LangAstConfig = LangAstConfig { @@ -407,6 +432,7 @@ pub const HASKELL_AST_CONFIG: LangAstConfig = LangAstConfig { regex_types: &[], quote_chars: &['"', '\''], string_prefixes: &[], + requires_named_node: false, }; pub const OCAML_AST_CONFIG: LangAstConfig = LangAstConfig { @@ -417,6 +443,7 @@ pub const OCAML_AST_CONFIG: LangAstConfig = LangAstConfig { regex_types: &[], quote_chars: &['"'], string_prefixes: &[], + requires_named_node: false, }; // F# string nodes in tree-sitter-fsharp surface under the `string` kind inside @@ -429,6 +456,7 @@ pub const FSHARP_AST_CONFIG: LangAstConfig = LangAstConfig { regex_types: &[], quote_chars: &['"'], string_prefixes: &[], + requires_named_node: false, }; /// Objective-C string literals use the `@"..."` prefix. The shared @@ -442,6 +470,7 @@ pub const OBJC_AST_CONFIG: LangAstConfig = LangAstConfig { regex_types: &[], quote_chars: &['"'], string_prefixes: &[], + requires_named_node: false, }; pub const GLEAM_AST_CONFIG: LangAstConfig = LangAstConfig { @@ -452,6 +481,7 @@ pub const GLEAM_AST_CONFIG: LangAstConfig = LangAstConfig { regex_types: &[], quote_chars: &['"'], string_prefixes: &[], + requires_named_node: false, }; pub const JULIA_AST_CONFIG: LangAstConfig = LangAstConfig { @@ -462,6 +492,7 @@ pub const JULIA_AST_CONFIG: LangAstConfig = LangAstConfig { regex_types: &[], quote_chars: &['"'], string_prefixes: &[], + requires_named_node: false, }; pub const CLOJURE_AST_CONFIG: LangAstConfig = LangAstConfig { @@ -472,6 +503,7 @@ pub const CLOJURE_AST_CONFIG: LangAstConfig = LangAstConfig { regex_types: &["regex_lit"], quote_chars: &['"'], string_prefixes: &[], + requires_named_node: false, }; pub const ERLANG_AST_CONFIG: LangAstConfig = LangAstConfig { @@ -482,6 +514,7 @@ pub const ERLANG_AST_CONFIG: LangAstConfig = LangAstConfig { regex_types: &[], quote_chars: &['"'], string_prefixes: &[], + requires_named_node: false, }; pub const GROOVY_AST_CONFIG: LangAstConfig = LangAstConfig { @@ -495,6 +528,7 @@ pub const GROOVY_AST_CONFIG: LangAstConfig = LangAstConfig { regex_types: &[], quote_chars: &['\'', '"'], string_prefixes: &[], + requires_named_node: false, }; pub const R_AST_CONFIG: LangAstConfig = LangAstConfig { @@ -506,6 +540,7 @@ pub const R_AST_CONFIG: LangAstConfig = LangAstConfig { regex_types: &[], quote_chars: &['\'', '"'], string_prefixes: &[], + requires_named_node: false, }; pub const SOLIDITY_AST_CONFIG: LangAstConfig = LangAstConfig { @@ -516,6 +551,7 @@ pub const SOLIDITY_AST_CONFIG: LangAstConfig = LangAstConfig { regex_types: &[], quote_chars: &['"', '\''], string_prefixes: &[], + requires_named_node: false, }; /// Verilog/SystemVerilog AST config. @@ -533,6 +569,7 @@ pub const VERILOG_AST_CONFIG: LangAstConfig = LangAstConfig { regex_types: &[], quote_chars: &['"'], string_prefixes: &[], + requires_named_node: false, }; // ── Generic AST node walker ────────────────────────────────────────────────── @@ -565,7 +602,18 @@ pub fn walk_ast_nodes_with_config( /// Classify a tree-sitter node against the language AST config. /// Returns the AST kind string if matched, or `None` to skip. -fn classify_ast_node<'a>(kind: &str, config: &'a LangAstConfig) -> Option<&'a str> { +/// +/// When `config.requires_named_node` is set, anonymous grammar tokens are +/// rejected even if their `kind()` string matches a mapped type — e.g. PHP's +/// `primitive_type`/`cast_type` productions lex the `string` scalar +/// type-hint keyword as an unnamed token whose `kind()` collides with the +/// named `string` literal node (#1821, mirrors TypeScript's `predefined_type` +/// guard from #1729). +fn classify_ast_node<'a>(node: &Node, config: &'a LangAstConfig) -> Option<&'a str> { + if config.requires_named_node && !node.is_named() { + return None; + } + let kind = node.kind(); if config.new_types.contains(&kind) { Some("new") } else if config.throw_types.contains(&kind) { @@ -673,7 +721,7 @@ fn walk_ast_nodes_with_config_depth( return; } - if let Some(ast_kind) = classify_ast_node(node.kind(), config) { + if let Some(ast_kind) = classify_ast_node(node, config) { match ast_kind { "new" => { ast_nodes.push(build_new_node(node, source)); diff --git a/src/ast-analysis/rules/index.ts b/src/ast-analysis/rules/index.ts index 392967427..e365a14c7 100644 --- a/src/ast-analysis/rules/index.ts +++ b/src/ast-analysis/rules/index.ts @@ -383,20 +383,37 @@ export function astStopRecurseKinds(langId: string): ReadonlySet { // ─── Per-language "named-node-required" guard ──────────────────────────── // -// tree-sitter-typescript's `predefined_type` production (the `string`, -// `number`, `boolean`, `void`, ... primitive type keywords) lexes its -// keyword as an anonymous grammar token whose `type` string is identical to -// the *named* `string` node type used for real string literals — the -// grammar's own node-types.json lists both a `"string", named: true` entry -// (the literal) and a `"string", named: false` entry (the keyword token). -// Matching by `node.type` alone therefore misclassifies `name: string` -// type annotations as string-literal ast_nodes (#1729). Mirrors the native -// `is_named()` guard in `extractors/javascript.rs::walk_ast_nodes_depth`. +// Some grammars reuse the same `type` string for two different node-types +// entries: a *named* node (a real literal) and an *anonymous* token (a +// keyword). Matching by `node.type` alone misclassifies the keyword as the +// literal. Two bundled grammars currently hit this: // -// Scoped to the JS family: no other bundled grammar currently maps an -// AST_TYPE_MAPS key that collides with an anonymous token of the same name. -const JS_REQUIRES_NAMED_NODE: ReadonlySet = new Set(['javascript', 'typescript', 'tsx']); +// - tree-sitter-typescript's `predefined_type` production (the `string`, +// `number`, `boolean`, `void`, ... primitive type keywords) lexes its +// keyword as an anonymous grammar token whose `type` string is identical +// to the *named* `string` node type used for real string literals — +// node-types.json lists both a `"string", named: true` entry (the +// literal) and a `"string", named: false` entry (the keyword token). +// `name: string` type annotations were misclassified as string-literal +// ast_nodes (#1729). +// - tree-sitter-php's `primitive_type`/`cast_type` productions (scalar +// type-hints like `string $x` and casts like `(string)`) lex the `string` +// keyword the same anonymous way, colliding with PHP's *named* `string` +// literal node type (#1821). +// +// Mirrors the native `is_named()`/`requires_named_node` guards in +// `extractors/javascript.rs::walk_ast_nodes_depth` (JS bespoke walker) and +// `extractors/helpers.rs::classify_ast_node` (generic walker, PHP config). +// +// A language is added here only when a `node-types.json` sweep confirms an +// AST_TYPE_MAPS key collides with an anonymous token of the same name. +const REQUIRES_NAMED_NODE: ReadonlySet = new Set([ + 'javascript', + 'typescript', + 'tsx', + 'php', +]); export function astRequiresNamedNode(langId: string): boolean { - return JS_REQUIRES_NAMED_NODE.has(langId); + return REQUIRES_NAMED_NODE.has(langId); } diff --git a/src/ast-analysis/visitors/ast-store-visitor.ts b/src/ast-analysis/visitors/ast-store-visitor.ts index 963840f9f..864c739da 100644 --- a/src/ast-analysis/visitors/ast-store-visitor.ts +++ b/src/ast-analysis/visitors/ast-store-visitor.ts @@ -277,7 +277,10 @@ function collectNode(ctx: CollectCtx, node: TreeSitterNode, kind: string): void * When `requireNamedNode` is set, anonymous grammar tokens are rejected even * if their `type` string matches a mapped key — e.g. TypeScript's * `predefined_type` keyword (`string`, `number`, ...) lexes as an unnamed - * token whose type collides with the named `string` literal node (#1729). + * token whose type collides with the named `string` literal node (#1729), + * and PHP's `primitive_type`/`cast_type` keyword (`string`) collides with + * the named `string` literal node the same way (#1821). See + * `astRequiresNamedNode()` in `../rules/index.ts` for the full language list. */ function resolveAstKind( node: TreeSitterNode, diff --git a/tests/engines/ast-parity.test.ts b/tests/engines/ast-parity.test.ts index 4e4e50192..f510b2582 100644 --- a/tests/engines/ast-parity.test.ts +++ b/tests/engines/ast-parity.test.ts @@ -260,10 +260,11 @@ end ext: '.php', code: ` ).toBe(true); }); + test('PHP: does not misclassify scalar type-hint keyword as kind:string (#1821)', () => { + // `string $name` (createUser's parameter, line 233) must never surface as a + // bare, unquoted "string" row — genuine literals are always quoted in `text`. + const strings = db + .prepare("SELECT * FROM ast_nodes WHERE kind = 'string' AND file LIKE '%fixture.php'") + .all(); + expect(strings.some((n) => n.text === 'string')).toBe(false); + }); + // ── Cross-language ── test('all nodes have valid kinds', () => { diff --git a/tests/parsers/ast-nodes.test.ts b/tests/parsers/ast-nodes.test.ts index b6d31a503..39caae9a4 100644 --- a/tests/parsers/ast-nodes.test.ts +++ b/tests/parsers/ast-nodes.test.ts @@ -537,3 +537,171 @@ describe.skipIf(!canTestNative)('buildAstNodes — native TypeScript extraction expect(nodes.length).toBe(4); }); }); + +// ─── PHP fixture (#1821: primitive_type/cast_type keyword false-positives) ─ +// +// tree-sitter-php's `primitive_type` production (scalar type-hints like +// `string $x` and `: string` return types) and `cast_type` production +// (`(string) $x`) both lex the `string` keyword as an anonymous token whose +// `type` string collides with the *named* `string` literal node type — +// mirroring TypeScript's `predefined_type` construct (#1729). + +const PHP_FIXTURE_CODE = ` { + let phpTmpDir: string, phpDb: any; + + function queryPhpAstNodes(kind: string) { + return phpDb.prepare('SELECT * FROM ast_nodes WHERE kind = ? ORDER BY line').all(kind); + } + + beforeAll(async () => { + phpTmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-ast-php-extract-')); + const srcDir = path.join(phpTmpDir, 'src'); + fs.mkdirSync(srcDir, { recursive: true }); + fs.mkdirSync(path.join(phpTmpDir, '.codegraph')); + + const fixturePath = path.join(srcDir, 'fixture.php'); + fs.writeFileSync(fixturePath, PHP_FIXTURE_CODE); + + const allSymbols = await parseFilesAuto([fixturePath], phpTmpDir, { engine: 'wasm' }); + const symbols = allSymbols.get('src/fixture.php'); + if (!symbols) throw new Error('Failed to parse PHP fixture file'); + + const dbPath = path.join(phpTmpDir, '.codegraph', 'graph.db'); + phpDb = new Database(dbPath); + phpDb.pragma('journal_mode = WAL'); + initSchema(phpDb); + + const insertNode = phpDb.prepare( + 'INSERT INTO nodes (name, kind, file, line, end_line) VALUES (?, ?, ?, ?, ?)', + ); + for (const def of symbols.definitions) { + insertNode.run(def.name, def.kind, 'src/fixture.php', def.line, def.endLine); + } + + await buildAstNodes(phpDb, allSymbols, phpTmpDir); + }); + + afterAll(() => { + if (phpDb) phpDb.close(); + fs.rmSync(phpTmpDir, { recursive: true, force: true }); + }); + + test('does not misclassify parameter scalar type-hint as kind:string', () => { + // `string $name` (line 4) must never surface as a bare, unquoted "string" + // row — genuine literals are always quoted in `text`. + const nodes = queryPhpAstNodes('string'); + expect(nodes.some((n) => n.text === 'string')).toBe(false); + }); + + test('does not misclassify return type-hint as kind:string', () => { + // `: string` return types on greet/label/normalize must not contribute + // bare "string" rows. + const nodes = queryPhpAstNodes('string'); + expect(nodes.filter((n) => n.text === 'string').length).toBe(0); + }); + + test('does not misclassify (string) cast as kind:string', () => { + // `(string) $value` (line 12) must not contribute a bare "string" row. + const nodes = queryPhpAstNodes('string'); + expect(nodes.some((n) => n.line === 12)).toBe(false); + }); + + test('still captures genuine string literals (interpolated and plain)', () => { + const nodes = queryPhpAstNodes('string'); + const names = nodes.map((n) => n.name); + expect(names.some((n) => n?.includes('Hello,'))).toBe(true); // interpolated + expect(names).toContain('static label'); + expect(names).toContain('hello world'); + }); + + test('captures exactly the 3 genuine literals — no keyword false-positives', () => { + const nodes = queryPhpAstNodes('string'); + expect(nodes.length).toBe(3); + }); +}); + +// ─── Native engine: PHP primitive_type/cast_type false-positives (#1821) ── + +describe.skipIf(!canTestNative)('buildAstNodes — native PHP extraction (#1821)', () => { + let nativePhpTmpDir: string, nativePhpDb: any; + + function queryNativePhpAstNodes(kind: string) { + return nativePhpDb.prepare('SELECT * FROM ast_nodes WHERE kind = ? ORDER BY line').all(kind); + } + + beforeAll(async () => { + nativePhpTmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-ast-php-native-')); + const srcDir = path.join(nativePhpTmpDir, 'src'); + fs.mkdirSync(srcDir, { recursive: true }); + fs.mkdirSync(path.join(nativePhpTmpDir, '.codegraph')); + + const fixturePath = path.join(srcDir, 'fixture.php'); + fs.writeFileSync(fixturePath, PHP_FIXTURE_CODE); + + const allSymbols = await parseFilesAuto([fixturePath], nativePhpTmpDir, { engine: 'native' }); + const symbols = allSymbols.get('src/fixture.php'); + if (!symbols) throw new Error('Failed to parse PHP fixture file with native engine'); + + const dbPath = path.join(nativePhpTmpDir, '.codegraph', 'graph.db'); + nativePhpDb = new Database(dbPath); + nativePhpDb.pragma('journal_mode = WAL'); + initSchema(nativePhpDb); + + const insertNode = nativePhpDb.prepare( + 'INSERT INTO nodes (name, kind, file, line, end_line) VALUES (?, ?, ?, ?, ?)', + ); + for (const def of symbols.definitions) { + insertNode.run(def.name, def.kind, 'src/fixture.php', def.line, def.endLine); + } + + await buildAstNodes(nativePhpDb, allSymbols, nativePhpTmpDir); + }); + + afterAll(() => { + if (nativePhpDb) nativePhpDb.close(); + if (nativePhpTmpDir) fs.rmSync(nativePhpTmpDir, { recursive: true, force: true }); + }); + + test('does not misclassify parameter scalar type-hint as kind:string', () => { + const nodes = queryNativePhpAstNodes('string'); + expect(nodes.some((n) => n.text === 'string')).toBe(false); + }); + + test('does not misclassify return type-hint or cast as kind:string', () => { + const nodes = queryNativePhpAstNodes('string'); + expect(nodes.filter((n) => n.text === 'string').length).toBe(0); + }); + + test('still captures genuine string literals (interpolated and plain)', () => { + const nodes = queryNativePhpAstNodes('string'); + const names = nodes.map((n) => n.name); + expect(names.some((n) => n?.includes('Hello,'))).toBe(true); + expect(names).toContain('static label'); + expect(names).toContain('hello world'); + }); + + test('captures exactly the 3 genuine literals — no keyword false-positives', () => { + const nodes = queryNativePhpAstNodes('string'); + expect(nodes.length).toBe(3); + }); +});