From 71a950fae1c0ad7bc3c5fad0ecea0c4c8e5119cb Mon Sep 17 00:00:00 2001 From: Colby McHenry Date: Tue, 27 Oct 2026 20:41:39 -0500 Subject: [PATCH] fix(js,ts): a function exported as exports["name"] is a symbol named after its export MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `exports.getItems = async (req, res) => {…}` names its function after the export property, in both engines (since the CommonJS controller fix), but the same export written with the name in quotes, `exports["getItems"] = …` or `module.exports['deleteItem'] = function () {…}`, left the function anonymous: no node, so nothing could find it or link to it, and its calls counted as the file's. commonJsExportName (tree-sitter.ts) and its kernel twin common_js_export_name (tsjs/extractors.rs) accepted only a member_expression on the left of the assignment. Both now also accept a subscript_expression on `exports` or `module.exports` whose index is a string literal spelling a plain identifier. The test for that string, which `this['svc']` receivers already used in both engines, is now one helper per engine (quotedIdentifier / quoted_identifier) shared by the two readers. A key that no identifier can spell (`exports["not-a-name"]`), a computed key and a template string still name nothing. Validation: the new commonjs-exports cases fail on main and pass in both engines, and a parity case compares the kernel's and wasm's nodes and refs for the JS and TS forms. Reimplements the bug part of PR #2896 by @johnhldiver-beep. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + __tests__/commonjs-exports.test.ts | 98 ++++++++++++++++++++++++- codegraph-kernel/src/tsjs/extractors.rs | 59 +++++++++------ src/extraction/tree-sitter.ts | 29 ++++++-- 4 files changed, 156 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3eb8b971c0..dcf7f7b907 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -131,6 +131,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - Axum routes now link to their handler when rustfmt wraps the `.route(` call so the handler sits on a later line, and when the handler's name also appears elsewhere on the line, as in `delete(handlers::user::delete)` or `.route("/login", post(handlers::user::login))`; Actix's `web::resource(…)` and `.route(…)` registrations get the same fix. A call inside a closure handler is no longer mistaken for a route, and an Actix resource no longer takes the next route's handler as its own. Thanks @mg-mg-mg for the report and @danusha2345. (#2326) - In Play projects, a route or a call like `MediaType.parse(...)` now links to a method of the class or object it names, and a call to an overloaded one to the overload its arguments fit. Before, it went to the first method of that name anywhere in the class's file, often a nested or sibling class's. When a Scala and a Java type share a name, as Play's own `RequestAuthority` does, each language's calls now reach their own. Thanks @danusha2345. - In JavaScript and TypeScript, the functions written inside a named object literal are now symbols of their own even when the object isn't exported: a plain `const api = { load() {…} }`, an object declared inside an IIFE or a function, and a namespace hung on the page or on another object, like `window.App = { init() {…} }`, `App.utils = {…}` or `dw_page = {…}`. Calls such as `api.load()`, `window.App.init()`, `App.utils.pad()`, a sibling's `this.render()`, and `App.init()` from another script on the page now reach them, the calls made inside a member belong to that member instead of the object, and a bare `init()` is no longer taken for `App.init`, so script-tag apps no longer lose most of their code from `callers` and impact. A member's qualified name now carries its object, exported or not (`api::load`, `window.App::init`), so asking for `App.init` finds it. Re-index JavaScript and TypeScript projects after upgrading. Thanks @tkhoaaa for the report and @danusha2345. (#2300) +- In JavaScript and TypeScript, a function exported with its name in quotes, like `exports["getItems"] = async (req, res) => {…}` or `module.exports['deleteItem'] = function (req, res) {…}`, is now a symbol named after the export, as `exports.getItems = …` already was. Before, it had no symbol, so nothing could find it or link to it, and its calls counted as the file's own. Re-index JavaScript and TypeScript projects after upgrading. Thanks @johnhldiver-beep. - In JavaScript and TypeScript files with Windows (CRLF) line endings, as a Windows checkout usually has them, a store exported on a later line, like `const useStore = create(…)` followed by `export default useStore;`, now has its actions indexed, as it already did with Unix line endings. Before, those actions were missing from the graph on Windows, so their callers and impact came back empty. Re-index JavaScript and TypeScript projects after upgrading. - In VB.NET, a name that begins with a keyword, like `SharedCache`, `Dimension`, `PublicKey` or `FriendlyName`, is now indexed whole, and a method like `Sub NewItem()` is no longer taken for a constructor. Before, the keyword was cut off (`Cache`, `ension`, `Key`) or the declaration was lost, and a line in a method that starts with such a name, like `Constants.Reload()` or `ConstVBV.Value = False`, was misread as a declaration, so reads of and calls to these names didn't link. A `Private Protected` member now counts as private instead of public. Re-index VB.NET projects after upgrading. - In JavaScript and TypeScript, a store whose name contains a `$`, like `const items$ = create(…)` exported on a later line with `export default items$;`, now has its actions indexed. Before, they could be missing from the graph, for example in a file that also had a syntax error. Re-index JavaScript and TypeScript projects after upgrading. diff --git a/__tests__/commonjs-exports.test.ts b/__tests__/commonjs-exports.test.ts index eaadf2299a..039fe5db9c 100644 --- a/__tests__/commonjs-exports.test.ts +++ b/__tests__/commonjs-exports.test.ts @@ -8,15 +8,43 @@ * treatment `const getItems = () => {}` already has: a function node, exported, * with its calls attributed to it rather than to the file. */ -import { describe, it, expect, beforeAll } from 'vitest'; +import { describe, it, expect, beforeAll, beforeEach, afterEach } from 'vitest'; +import * as fs from 'fs'; +import * as path from 'path'; import { extractFromSource } from '../src/extraction'; import { initGrammars, loadAllGrammars } from '../src/extraction/grammars'; +import { tryKernelExtract, resetKernelForTests } from '../src/extraction/kernel'; +import type { ExtractionResult, Language } from '../src/types'; beforeAll(async () => { await initGrammars(); await loadAllGrammars(); }); +const KERNEL_PATH = path.join( + __dirname, + '..', + 'codegraph-kernel', + 'prebuilds', + `${process.platform}-${process.arch}`, + 'codegraph-kernel.node' +); +const kernelAvailable = fs.existsSync(KERNEL_PATH) || process.env.CODEGRAPH_KERNEL_EXPECT === '1'; +const backends = kernelAvailable ? (['kernel', 'wasm'] as const) : (['wasm'] as const); +const ENV_KEYS = ['CODEGRAPH_KERNEL', 'CODEGRAPH_KERNEL_LANGS'] as const; + +function extract(backend: 'kernel' | 'wasm', file: string, source: string, language: Language): ExtractionResult { + if (backend === 'wasm') { + process.env.CODEGRAPH_KERNEL = '0'; + return extractFromSource(file, source, language); + } + delete process.env.CODEGRAPH_KERNEL; + process.env.CODEGRAPH_KERNEL_LANGS = 'all'; + const result = tryKernelExtract(file, source, language); + expect(result, `kernel extraction of ${file}`).not.toBeNull(); + return result!; +} + const refsFrom = (result: ReturnType, id: string) => result.unresolvedReferences.filter((r) => r.fromNodeId === id).map((r) => r.referenceName); @@ -69,3 +97,71 @@ app.locals.format = function () { return 1; }; expect(result.nodes.filter((n) => n.kind === 'function')).toEqual([]); }); }); + +describe('CommonJS export assignments with the name in quotes', () => { + // `exports["getItems"] = …` is the same export as `exports.getItems = …`, + // but only the dotted form named its function: the bracket form left the + // handler without a node, so its calls belonged to the file and nothing + // could reach it. + let savedEnv: Record = {}; + beforeEach(() => { + savedEnv = Object.fromEntries(ENV_KEYS.map((k) => [k, process.env[k]])); + resetKernelForTests(); + }); + afterEach(() => { + for (const k of ENV_KEYS) { + if (savedEnv[k] === undefined) delete process.env[k]; + else process.env[k] = savedEnv[k]; + } + resetKernelForTests(); + }); + + const code = ` +const { findItems, removeItem } = require('./db'); + +exports["getItems"] = async (req, res) => { + res.json(await findItems()); +}; + +module.exports['deleteItem'] = function (req, res) { + removeItem(req.params.id); + res.end(); +}; + +exports["not-a-name"] = () => { skipped(); }; +exports[key] = () => { computed(); }; +exports[\`tpl\`] = () => { template(); }; +handlers["onSave"] = () => { persist(); }; +`; + + const summary = (result: ExtractionResult) => + result.nodes + .filter((n) => n.kind === 'function') + .map((n) => `${n.name}@${n.startLine} exported=${n.isExported} async=${n.isAsync}`) + .sort(); + + it.each(backends)('names the function an identifier in quotes exports: %s', (backend) => { + for (const [file, language] of [['src/controller.js', 'javascript'], ['src/controller.ts', 'typescript']] as const) { + const result = extract(backend, file, code, language); + expect(summary(result), file).toEqual(['deleteItem@8 exported=true async=false', 'getItems@4 exported=true async=true']); + + const fn = (name: string) => result.nodes.find((n) => n.kind === 'function' && n.name === name)!; + expect(refsFrom(result, fn('getItems').id)).toContain('findItems'); + expect(refsFrom(result, fn('deleteItem').id)).toContain('removeItem'); + const fileNode = result.nodes.find((n) => n.kind === 'file')!; + expect(refsFrom(result, fileNode.id)).not.toContain('findItems'); + expect(refsFrom(result, fileNode.id)).not.toContain('removeItem'); + } + }); + + it.runIf(kernelAvailable)('reads the same in both engines', () => { + for (const [file, language] of [['src/controller.js', 'javascript'], ['src/controller.ts', 'typescript']] as const) { + const kernel = extract('kernel', file, code, language); + const wasm = extract('wasm', file, code, language); + const nodes = (r: ExtractionResult) => r.nodes.map((n) => `${n.kind} ${n.qualifiedName} ${n.startLine}-${n.endLine} ${n.isExported}`).sort(); + const refs = (r: ExtractionResult) => r.unresolvedReferences.map((x) => `${x.referenceKind} ${x.referenceName} ${x.line}:${x.column} ${x.fromNodeId}`).sort(); + expect(nodes(kernel), file).toEqual(nodes(wasm)); + expect(refs(kernel), file).toEqual(refs(wasm)); + } + }); +}); diff --git a/codegraph-kernel/src/tsjs/extractors.rs b/codegraph-kernel/src/tsjs/extractors.rs index f10c5a5073..6ebdb8836c 100644 --- a/codegraph-kernel/src/tsjs/extractors.rs +++ b/codegraph-kernel/src/tsjs/extractors.rs @@ -138,27 +138,57 @@ impl<'t> Walker<'t> { } /// The property a CommonJS export assignment binds a function to — - /// `exports.NAME = ` / `module.exports.NAME = ` — or None for - /// any other assignment. The node must be the assignment's whole - /// right-hand side. Mirrors TreeSitterExtractor.commonJsExportName. + /// `exports.NAME = ` / `module.exports.NAME = `, or the same + /// with the name in quotes, `exports["NAME"] = ` — or None for any + /// other assignment. The node must be the assignment's whole right-hand + /// side. Mirrors TreeSitterExtractor.commonJsExportName. fn common_js_export_name(&self, assignment: Node<'t>, value: Node<'t>) -> Option { let right = assignment.child_by_field_name("right")?; if right.start_byte() != value.start_byte() || right.end_byte() != value.end_byte() { return None; } let left = assignment.child_by_field_name("left")?; - if left.kind() != "member_expression" { + if !matches!(left.kind(), "member_expression" | "subscript_expression") { return None; } let object = left.child_by_field_name("object")?; + if !matches!(self.text(object), "exports" | "module.exports") { + return None; + } + if left.kind() == "subscript_expression" { + return self.quoted_identifier(left.child_by_field_name("index")?).map(str::to_string); + } let property = left.child_by_field_name("property")?; if property.kind() != "property_identifier" { return None; } - if !matches!(self.text(object), "exports" | "module.exports") { + Some(self.text(property).to_string()) + } + + /// The name a string literal spells when it is a plain identifier: `'svc'` + /// or `"getItems"` → the text between the quotes. None for anything else, + /// including a string with an escape or a character no identifier can + /// hold. Mirrors quotedIdentifier in tree-sitter.ts. + fn quoted_identifier(&self, node: Node<'t>) -> Option<&'t str> { + if node.kind() != "string" { return None; } - Some(self.text(property).to_string()) + let text = self.text(node); + let quoted = text.len() >= 2 + && (text.starts_with('\'') || text.starts_with('"')) + && text.ends_with(&text[..1]); + if !quoted { + return None; + } + let key = &text[1..text.len() - 1]; + let mut chars = key.chars(); + let first = chars.next()?; + if !(first.is_ascii_alphabetic() || first == '_' || first == '$') + || !chars.all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '$') + { + return None; + } + Some(key) } // --- reactComponentHoc / extractReactComponentNode (#841) -------------------- @@ -1775,22 +1805,7 @@ impl<'t> Walker<'t> { } let object = peel_receiver(receiver.child_by_field_name("object")?); if object.kind() == "this" { - let index = receiver.child_by_field_name("index")?; - if index.kind() != "string" { - return None; - } - let text = self.text(index); - let quoted = text.len() >= 2 - && (text.starts_with('\'') || text.starts_with('"')) - && text.ends_with(&text[..1]); - let key = if quoted { &text[1..text.len() - 1] } else { return None }; - let mut chars = key.chars(); - let first = chars.next()?; - if !(first.is_ascii_alphabetic() || first == '_' || first == '$') - || !chars.all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '$') - { - return None; - } + let key = self.quoted_identifier(receiver.child_by_field_name("index")?)?; return Some(format!("this.{key}")); } self.this_field_of(object).map(|field| format!("this.{field}[]")) diff --git a/src/extraction/tree-sitter.ts b/src/extraction/tree-sitter.ts index 89b2cb6fac..22889804ed 100644 --- a/src/extraction/tree-sitter.ts +++ b/src/extraction/tree-sitter.ts @@ -704,6 +704,17 @@ function tsJsCalleeName(func: SyntaxNode, source: string): { name: string; at: S return /^[A-Za-z_$][\w$]*(?:\.[A-Za-z_$][\w$]*)+$/.test(chain) ? { name: chain, at: cur } : null; } +/** + * The name a JS/TS string literal spells when it is a plain identifier: `'svc'` + * or `"getItems"` → the text between the quotes. Null for anything else, + * including a string with an escape or a character no identifier can hold. + * Mirrored in the kernel's quoted_identifier. + */ +function quotedIdentifier(node: SyntaxNode | null, source: string): string | null { + if (node?.type !== 'string') return null; + return /^(['"])([A-Za-z_$][\w$]*)\1$/.exec(getNodeText(node, source))?.[2] ?? null; +} + /** * A TS/JS receiver that subscripts the enclosing class's object: an element * of a field, `this.[k]` → `this.[]`, or a field named by a @@ -716,8 +727,7 @@ function tsThisSubscriptReceiver(node: SyntaxNode, source: string): string | nul if (!objectNode) return null; const object = peelTsJsReceiver(objectNode); if (object.type === 'this') { - const index = getChildByField(node, 'index'); - const key = index?.type === 'string' ? /^(['"])([A-Za-z_$][\w$]*)\1$/.exec(getNodeText(index, source))?.[2] : undefined; + const key = quotedIdentifier(getChildByField(node, 'index'), source); return key ? `this.${key}` : null; } if (object.type !== 'member_expression' || getChildByField(object, 'object')?.type !== 'this') return null; @@ -7305,9 +7315,10 @@ export class TreeSitterExtractor { /** * The property a CommonJS export assignment binds a function to — - * `exports.NAME = ` or `module.exports.NAME = ` — or null for - * any other assignment. JS-family only; the node must be the assignment's - * whole right-hand side. + * `exports.NAME = ` or `module.exports.NAME = `, or the same + * with the name in quotes, `exports["NAME"] = ` — or null for any + * other assignment. JS-family only; the node must be the assignment's whole + * right-hand side. */ private commonJsExportName(assignment: SyntaxNode, value: SyntaxNode): string | null { if ( @@ -7321,12 +7332,14 @@ export class TreeSitterExtractor { const right = getChildByField(assignment, 'right'); if (!right || right.startIndex !== value.startIndex || right.endIndex !== value.endIndex) return null; const left = getChildByField(assignment, 'left'); - if (!left || left.type !== 'member_expression') return null; + if (!left || (left.type !== 'member_expression' && left.type !== 'subscript_expression')) return null; const object = getChildByField(left, 'object'); - const property = getChildByField(left, 'property'); - if (!object || !property || property.type !== 'property_identifier') return null; + if (!object) return null; const objectText = getNodeText(object, this.source); if (objectText !== 'exports' && objectText !== 'module.exports') return null; + if (left.type === 'subscript_expression') return quotedIdentifier(getChildByField(left, 'index'), this.source); + const property = getChildByField(left, 'property'); + if (!property || property.type !== 'property_identifier') return null; return getNodeText(property, this.source); }