mirror of
https://github.com/colbymchenry/codegraph.git
synced 2026-10-18 13:53:01 +08:00
fix(js,ts): a catch-all or node_modules path alias no longer makes package imports look local (#2437)
The out-of-repo import guard (`isOutOfRepoImport`) never fired in two setups: - A catch-all `paths` key. The guard started with `isExternalImport`, which treats an import as local when it starts with a root alias's prefix, and a `"*"` key's prefix is empty. On cord-field (`"*": ["./typings/*"]`), 169 imports of `Typography` from `@mui/material` bound to the project's own `Typography`. - An alias that lands on disk but outside the index. `fileExists` falls back to `fs.existsSync`, which also answers for a directory or a file under `node_modules`. home-assistant's `"lit/decorators": ["./node_modules/lit/decorators.js"]` bound 4,916 `@property()` decorators to an unrelated `property` field, and topcoder's `config` package landed on its `config/` folder. The guard now skips the alias-prefix test (it already asks `resolveImportPath` whether an alias maps the import to a file) and counts that resolution only when it lands on an indexed file. Other callers of `isExternalImport` are unchanged: resolving an import still needs the prefix test so `"*": ["src/*"]` aliases resolve at all. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -96,6 +96,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
|
||||
- In JavaScript and TypeScript, a module loaded with `require('./x').default` or `const { default: X } = require('./x')` now links to its default export when it is written as an ES module: bitwarden's desktop app loads `export default class OsBiometricsServiceMac` this way before calling `new OsBiometricsServiceMac(…)`, and React Native's own libraries load many of their modules like this. Before, `.default` was looked up only as an export named `default`, which `export default` is not, so the `new` and the calls made through it linked to nothing, to the local variable holding the module, or to a method of the same name somewhere else. A Svelte component loaded this way and created with `new App({ target })` now links to the component. A CommonJS module that sets `exports.default` itself, or both `module.exports` and `module.exports.default` as fastify does, still links to what it sets, and one that sets no `default` still links to nothing. Re-index JavaScript and TypeScript projects after upgrading.
|
||||
- In JavaScript and TypeScript, an import written `import { default as AppRoot } from './routes/app/root'`, as in bulletproof-react's router, now counts as the module's default import, the same as `import AppRoot from './routes/app/root'`. Before, it was read as an import of an export named `default`, which no module has, so a call, a route or a JSX attribute that used `AppRoot` was matched by its name alone: it could link to another file's `AppRoot`, or to nothing when the default export has a name of its own. Svelte, Vue and Astro script blocks are read the same way.
|
||||
- In React, a component's JSX no longer links it to a class or component that only shares a name with one of its type arguments or with a variable of its own. A type in angle brackets, like `Document` in `<PaginatedList<Document> items={…} />`, `User` in `useState<User>()` or `Entry` in a generic `<Entry extends BaseEntity>(…) =>` component, is no longer read as a tag. A tag naming a variable or parameter the component sets itself, like `<Content>` after `const Content = isDropdown ? DropdownMenu.SubContent : ContextMenu.SubContent` or `<Widget>` in `widgets.map((Widget) => <Widget />)`, now links to nothing, unless the component declares a component of that name inside itself. Before, these linked to an unrelated class or component elsewhere in the repository: outline's document lists showed up among the callers of its `Document` model class, and its menus among the callers of the command bar's `Content`, so `codegraph_explore`, callers and impact followed renders that never happen. Re-index React projects after upgrading.
|
||||
- In JavaScript and TypeScript, a name imported from a package your `package.json` lists no longer links to a project symbol that only shares its name when `tsconfig.json` or `jsconfig.json` has a catch-all path alias, like `"*": ["./typings/*"]` or `"*": ["src/*", "node_modules/*"]`, or an alias that points the package at a file in `node_modules`, like `"lit/decorators": ["./node_modules/lit/decorators.js"]`. Such an alias made every package look like part of the project, so `import { Typography } from '@mui/material'` was linked to the project's own `Typography` and every lit `@property()` decorator to an unrelated class's `property` field, and `codegraph callers`, impact and `codegraph affected` listed code that never used them. An import the alias does map to a file of your project, like a `.d.ts` you keep for an untyped package or `components/Button` through `"*": ["src/*"]`, links as before. Re-index affected projects after upgrading.
|
||||
|
||||
## [1.6.2] - 2026-10-03
|
||||
|
||||
|
||||
@@ -0,0 +1,184 @@
|
||||
/**
|
||||
* A tsconfig `paths` entry keyed `"*"` matches every specifier, so treating
|
||||
* "an alias pattern matches" as "the import is the project's" made every
|
||||
* package import in such a project look local. The out-of-repo guard never
|
||||
* fired there: on cord-field (`"baseUrl": "src"`, `"*": ["./typings/*"]`),
|
||||
* 169 imports of `Typography` from `@mui/material` landed on a project
|
||||
* `Typography`, and `Form` from `react-final-form` on the project's form.
|
||||
*
|
||||
* An alias counts only when it maps the specifier to a project file. A
|
||||
* package the importing file's package.json declares, which no alias maps to
|
||||
* a file, is outside the repository, catch-all or not. A catch-all that does
|
||||
* find a file (a local `.d.ts` for an untyped package, or `"*": ["src/*"]`
|
||||
* for the project's own folders) still makes that import the project's.
|
||||
*/
|
||||
import { describe, it, expect, afterAll } from 'vitest';
|
||||
import * as fs from 'fs';
|
||||
import * as os from 'os';
|
||||
import * as path from 'path';
|
||||
import { CodeGraph } from '../src';
|
||||
|
||||
const roots: string[] = [];
|
||||
afterAll(() => {
|
||||
for (const r of roots.splice(0)) fs.rmSync(r, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
async function indexProject(files: Record<string, string>): Promise<CodeGraph> {
|
||||
const root = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-catch-all-alias-'));
|
||||
roots.push(root);
|
||||
for (const [rel, content] of Object.entries(files)) {
|
||||
fs.mkdirSync(path.dirname(path.join(root, rel)), { recursive: true });
|
||||
fs.writeFileSync(path.join(root, rel), content);
|
||||
}
|
||||
return CodeGraph.init(root, { index: true });
|
||||
}
|
||||
|
||||
/**
|
||||
* `<kind> <target file>:<target qualified name>` for every edge leaving
|
||||
* `file`'s nodes. JSX tags are left out: a separate pass links `<Button>` by
|
||||
* name without reading the file's imports, alias or not.
|
||||
*/
|
||||
function edgesFrom(cg: CodeGraph, file: string): string[] {
|
||||
const ids = cg.getNodesInFile(file).map((n) => n.id);
|
||||
return cg
|
||||
.getOutgoingEdgesFrom(ids)
|
||||
.filter((e) => e.metadata?.synthesizedBy !== 'jsx-render')
|
||||
.map((e) => {
|
||||
const target = cg.getNode(e.target);
|
||||
return `${e.kind} ${target?.filePath}:${target?.qualifiedName}`;
|
||||
})
|
||||
.sort();
|
||||
}
|
||||
|
||||
describe('a catch-all tsconfig path alias', () => {
|
||||
it('leaves a declared package outside the repository when it names no project file', async () => {
|
||||
const cg = await indexProject({
|
||||
'package.json': JSON.stringify({
|
||||
name: 'portal',
|
||||
dependencies: {
|
||||
'@mui/material': '^5.15.0',
|
||||
'react-final-form': '^6.5.9',
|
||||
luxon: '^3.4.3',
|
||||
'legacy-widgets': '^1.0.0',
|
||||
react: '^18.2.0',
|
||||
},
|
||||
}),
|
||||
// cord-field's shape: `~/…` for the project's own files, `*` for local
|
||||
// type declarations of untyped packages.
|
||||
'tsconfig.json': JSON.stringify({
|
||||
compilerOptions: { baseUrl: 'src', jsx: 'react-jsx', paths: { '~/*': ['./*'], '*': ['./typings/*'] } },
|
||||
}),
|
||||
'src/components/Button.tsx': `export interface ButtonProps { label: string }
|
||||
export function Button(props: ButtonProps) { return <button>{props.label}</button>; }
|
||||
`,
|
||||
'src/components/Card.tsx': `export function Card(props: { children?: unknown }) { return <div>{String(props.children)}</div>; }
|
||||
`,
|
||||
'src/components/form/Form.tsx': `export function Form() { return <form />; }
|
||||
export function useForm() { return { valid: true }; }
|
||||
`,
|
||||
'src/common/DateTime.ts': `export class DateTime {
|
||||
static now(): DateTime { return new DateTime(); }
|
||||
}
|
||||
`,
|
||||
// The catch-all does answer this package: its types live in the project.
|
||||
'src/typings/legacy-widgets.d.ts': `export interface WidgetOptions { name: string }
|
||||
export declare class Widget {
|
||||
constructor(options: WidgetOptions);
|
||||
render(): void;
|
||||
}
|
||||
`,
|
||||
'src/scenes/Page.tsx': `import { Button, type ButtonProps } from '@mui/material';
|
||||
import { Form, useForm } from 'react-final-form';
|
||||
import { DateTime } from 'luxon';
|
||||
import { Widget, type WidgetOptions } from 'legacy-widgets';
|
||||
import { Card } from '~/components/Card';
|
||||
|
||||
const options: WidgetOptions = { name: 'page' };
|
||||
|
||||
export function Page(props: ButtonProps) {
|
||||
const form = useForm();
|
||||
const when = DateTime.now();
|
||||
new Widget(options).render();
|
||||
return <Card><Button>{props.label}{String(form.valid)}{String(when)}</Button><Form /></Card>;
|
||||
}
|
||||
`,
|
||||
});
|
||||
try {
|
||||
const fromPage = edgesFrom(cg, 'src/scenes/Page.tsx');
|
||||
// No name the page imports from a package lands on a project namesake.
|
||||
expect(fromPage.filter((e) => /src\/components\/(?:Button|form\/Form)\.tsx|src\/common\/DateTime\.ts/.test(e))).toEqual([]);
|
||||
// `~/…` still reaches the project, and so does a package whose types the
|
||||
// catch-all finds in the project.
|
||||
expect(fromPage).toContain('imports src/components/Card.tsx:Card');
|
||||
expect(fromPage).toContain('imports src/typings/legacy-widgets.d.ts:Widget');
|
||||
expect(fromPage).toContain('imports src/typings/legacy-widgets.d.ts:WidgetOptions');
|
||||
} finally {
|
||||
cg.close();
|
||||
}
|
||||
});
|
||||
|
||||
it('still makes an import the project’s when it maps the import to a project file', async () => {
|
||||
const cg = await indexProject({
|
||||
'package.json': JSON.stringify({ name: 'shop', dependencies: { '@mui/material': '^5.15.0', react: '^18.2.0' } }),
|
||||
'tsconfig.json': JSON.stringify({ compilerOptions: { baseUrl: '.', jsx: 'react-jsx', paths: { '*': ['src/*'] } } }),
|
||||
'src/components/Button.tsx': `export function Button() { return <button />; }
|
||||
`,
|
||||
'src/pages/Home.tsx': `import { Button } from 'components/Button';
|
||||
export function Home() { return <Button />; }
|
||||
`,
|
||||
'src/pages/Checkout.tsx': `import { Button } from '@mui/material';
|
||||
export function Checkout() { return <Button />; }
|
||||
`,
|
||||
});
|
||||
try {
|
||||
expect(edgesFrom(cg, 'src/pages/Home.tsx')).toContain('imports src/components/Button.tsx:Button');
|
||||
expect(edgesFrom(cg, 'src/pages/Checkout.tsx').filter((e) => e.includes('src/components/Button.tsx'))).toEqual([]);
|
||||
} finally {
|
||||
cg.close();
|
||||
}
|
||||
});
|
||||
|
||||
it('leaves a declared package outside the repository when its alias lands outside the index', async () => {
|
||||
const cg = await indexProject({
|
||||
'package.json': JSON.stringify({ name: 'panel', dependencies: { lit: '^3.0.0', 'date-fns': '^3.0.0', config: '^3.3.0' } }),
|
||||
// home-assistant pins lit's entry points to files in node_modules, and
|
||||
// topcoder's catch-all ends in `node_modules/*`.
|
||||
'tsconfig.json': JSON.stringify({
|
||||
compilerOptions: {
|
||||
baseUrl: '.',
|
||||
experimentalDecorators: true,
|
||||
paths: { 'lit/decorators': ['./node_modules/lit/decorators.js'], '*': ['src/*', 'node_modules/*'] },
|
||||
},
|
||||
}),
|
||||
// Installed packages: on disk, never indexed.
|
||||
'node_modules/lit/decorators.js': `export function property() { return () => {}; }
|
||||
`,
|
||||
'node_modules/date-fns/index.js': `export function format(value) { return String(value); }
|
||||
`,
|
||||
'src/data/zwave.ts': `export interface ConfigParam {
|
||||
property: string;
|
||||
}
|
||||
`,
|
||||
'src/format.ts': `export function format(value: number) { return value.toFixed(2); }
|
||||
`,
|
||||
// The `config` package reads this folder at run time.
|
||||
'config/default.js': `const config = { port: 3000 };
|
||||
module.exports = config;
|
||||
`,
|
||||
'src/panel.ts': `import { property } from 'lit/decorators';
|
||||
import { format } from 'date-fns';
|
||||
import config from 'config';
|
||||
|
||||
export class Panel {
|
||||
@property() label = '';
|
||||
render() { return format(config.port); }
|
||||
}
|
||||
`,
|
||||
});
|
||||
try {
|
||||
expect(edgesFrom(cg, 'src/panel.ts').filter((e) => /src\/data\/zwave\.ts|src\/format\.ts|config\/default\.js/.test(e))).toEqual([]);
|
||||
} finally {
|
||||
cg.close();
|
||||
}
|
||||
});
|
||||
});
|
||||
@@ -435,11 +435,17 @@ const RUST_STDLIB_ROOTS = new Set(['std', 'core', 'alloc', 'proc_macro']);
|
||||
* (tsconfig/jsconfig `paths`). Without that check, custom prefixes
|
||||
* like `@components/*` would fail the bare-specifier heuristic and
|
||||
* be classified as external before alias resolution can run.
|
||||
*
|
||||
* `aliasPrefixes: false` skips that check, for a caller that has already
|
||||
* asked `resolveImportPath` whether an alias maps the specifier to a file.
|
||||
* Matching a prefix proves nothing by itself: a catch-all `"*"` pattern
|
||||
* (`"*": ["./typings/*"]`) has an empty prefix and matches every package.
|
||||
*/
|
||||
export function isExternalImport(
|
||||
importPath: string,
|
||||
language: Language,
|
||||
context?: ResolutionContext
|
||||
context?: ResolutionContext,
|
||||
options: { aliasPrefixes?: boolean } = {}
|
||||
): boolean {
|
||||
// Relative imports are not external
|
||||
if (importPath.startsWith('.')) {
|
||||
@@ -462,7 +468,7 @@ export function isExternalImport(
|
||||
return true;
|
||||
}
|
||||
// Project-defined alias prefix? Treat as local.
|
||||
const aliases = context?.getProjectAliases?.();
|
||||
const aliases = options.aliasPrefixes === false ? null : context?.getProjectAliases?.();
|
||||
if (aliases) {
|
||||
for (const pat of aliases.patterns) {
|
||||
if (importPath.startsWith(pat.prefix)) return false;
|
||||
|
||||
+19
-4
@@ -643,10 +643,19 @@ export class ReferenceResolver {
|
||||
private createContext(): ResolutionContext {
|
||||
return {
|
||||
resolveImport: (ref) => resolveViaImport(ref, this.context),
|
||||
isOutOfRepoImport: (source, fromFile, language) =>
|
||||
isExternalImport(source, language, this.context) &&
|
||||
resolveImportPath(source, fromFile, language, this.context) === null &&
|
||||
this.isDeclaredOutsidePackage(source, fromFile),
|
||||
// A path alias makes an import the project's only when it maps the
|
||||
// import to a file the index holds. Matching its prefix is not enough:
|
||||
// cord-field's `"*": ["./typings/*"]` matches every package, and
|
||||
// counting the match bound 169 imports of `@mui/material`'s Typography
|
||||
// to its own. Nor is a path that exists on disk: home-assistant's
|
||||
// `"lit/decorators": ["./node_modules/lit/decorators.js"]` lands in
|
||||
// `node_modules`, topcoder's `config` package on its `config/` folder.
|
||||
isOutOfRepoImport: (source, fromFile, language) => {
|
||||
if (!isExternalImport(source, language, this.context, { aliasPrefixes: false })) return false;
|
||||
const resolved = resolveImportPath(source, fromFile, language, this.context);
|
||||
if (resolved !== null && this.isIndexedFile(resolved)) return false;
|
||||
return this.isDeclaredOutsidePackage(source, fromFile);
|
||||
},
|
||||
getNodesInFile: (filePath: string) => {
|
||||
if (!this.nodeCache.has(filePath)) {
|
||||
this.nodeCache.set(filePath, this.queries.getNodesByFile(filePath));
|
||||
@@ -3301,6 +3310,12 @@ export class ReferenceResolver {
|
||||
return target && !isRustNameInScope(target, ref, this.context) ? null : result;
|
||||
}
|
||||
|
||||
/** Is `filePath` (project-relative) one of the files the index holds? */
|
||||
private isIndexedFile(filePath: string): boolean {
|
||||
const normalized = filePath.replace(/\\/g, '/');
|
||||
return this.knownFiles ? this.knownFiles.has(normalized) : this.queries.getFileByPath(normalized) !== null;
|
||||
}
|
||||
|
||||
/** The repository's own package name, from its root package.json; null without one. */
|
||||
/** Per directory: the package names its package.json and every enclosing one own and depend on. */
|
||||
private manifestScopes = new Map<string, { own: Set<string>; deps: Set<string> }>();
|
||||
|
||||
Reference in New Issue
Block a user