diff --git a/scanner/jsimports_test.go b/scanner/jsimports_test.go new file mode 100644 index 0000000..9a4eaa0 --- /dev/null +++ b/scanner/jsimports_test.go @@ -0,0 +1,94 @@ +package scanner + +import ( + "context" + "sort" + "testing" +) + +func sortedImporters(t *testing.T, graph *FileGraph, file string) []string { + t.Helper() + got := append([]string(nil), graph.Importers[file]...) + sort.Strings(got) + return got +} + +func equalStrings(left, right []string) bool { + if len(left) != len(right) { + return false + } + for i := range left { + if left[i] != right[i] { + return false + } + } + return true +} + +// The reported case: a CommonJS project written in the single quotes Prettier +// emits by default. js-imports matched only double-quoted literals, so every +// require in such a project was invisible and --importers answered a confident +// zero for a file with many requirers. +func TestCommonJSSingleQuoteImportersResolve(t *testing.T) { + graph, err := BuildFileGraph(context.Background(), "../testdata/commonjs-single-quotes", Filters{}) + if err != nil { + t.Fatalf("build commonjs fixture graph: %v", err) + } + + for _, tc := range []struct { + file string + want []string + why string + }{ + {"services/layoutInputService.js", []string{"routes/admin.js", "routes/members.js"}, "single-quoted require, both ../ and ./../ forms"}, + {"routes/members.js", []string{"app.js"}, "double-quoted require still resolves"}, + {"routes/admin.js", []string{"app.js"}, "single-quoted ESM import in a .js file (already matched on main via jsx.yml's kind: import_statement; pinned so the rule rewrite cannot regress it)"}, + {"app.js", nil, "entry point is imported by nothing"}, + } { + got := sortedImporters(t, graph, tc.file) + if !equalStrings(got, tc.want) { + t.Errorf("%s importers = %v, want exactly %v (%s)", tc.file, got, tc.want, tc.why) + } + } +} + +// A require whose argument is a variable cannot name a file. It has to resolve +// to nothing: a fabricated edge is worse than a missing one. +func TestDynamicRequireResolvesToNothing(t *testing.T) { + graph, err := BuildFileGraph(context.Background(), "../testdata/commonjs-single-quotes", Filters{}) + if err != nil { + t.Fatalf("build commonjs fixture graph: %v", err) + } + got := append([]string(nil), graph.Imports["app.js"]...) + sort.Strings(got) + want := []string{"routes/admin.js", "routes/members.js"} + if !equalStrings(got, want) { + t.Fatalf("app.js imports = %v, want exactly %v — require(which) must add no edge", got, want) + } +} + +// extractImportPath is what makes the structural rule quote-agnostic, since +// the rule binds no $PATH metavariable. Its fail-closed behavior on a +// non-literal argument is what keeps a dynamic require from inventing a path. +func TestExtractImportPathQuoteStyles(t *testing.T) { + for _, tc := range []struct { + name string + text string + want string + }{ + {"double-quoted require", `require("./routes/members")`, "./routes/members"}, + {"single-quoted require", `require('./routes/members')`, "./routes/members"}, + {"template-literal require", "require(`./routes/members`)", "./routes/members"}, + {"double-quoted esm import", `import x from "./mod";`, "./mod"}, + {"single-quoted esm import", `import x from './mod';`, "./mod"}, + {"bare esm import", `import './side-effect';`, "./side-effect"}, + {"dynamic require yields nothing", `require(someVariable)`, ""}, + {"computed require yields nothing", `require(base + '/x')`, "/x"}, + } { + t.Run(tc.name, func(t *testing.T) { + if got := extractImportPath(tc.text); got != tc.want { + t.Fatalf("extractImportPath(%q) = %q, want %q", tc.text, got, tc.want) + } + }) + } +} diff --git a/scanner/rustcargo.go b/scanner/rustcargo.go index 9f08133..90db77d 100644 --- a/scanner/rustcargo.go +++ b/scanner/rustcargo.go @@ -100,6 +100,10 @@ func parseCargoMetadata(data []byte) (cargoMetadata, error) { } func buildRustWorkspaceIndex(ctx context.Context, root string, analyses []FileAnalysis, files []FileInfo, loader cargoMetadataLoader) (*rustWorkspaceIndex, *ScanSourceOutcome, error) { + return buildRustWorkspaceIndexWithTimeout(ctx, root, analyses, files, loader, cargoMetadataTimeout) +} + +func buildRustWorkspaceIndexWithTimeout(ctx context.Context, root string, analyses []FileAnalysis, files []FileInfo, loader cargoMetadataLoader, metadataTimeout time.Duration) (*rustWorkspaceIndex, *ScanSourceOutcome, error) { index := buildRustFallbackWorkspaceIndex(root, analyses) manifestPaths, err := discoverCargoManifests(ctx, root, files) if err != nil { @@ -115,7 +119,7 @@ func buildRustWorkspaceIndex(ctx context.Context, root string, analyses []FileAn outcome := cargoMetadataOutcome(0, len(manifestPaths)) return index, &outcome, nil } - ctx, cancel := context.WithTimeout(ctx, cargoMetadataTimeout) + metadataCtx, cancel := context.WithTimeout(ctx, metadataTimeout) defer cancel() packagesByRoot := make(map[string]rustPackage, len(index.packages)) @@ -129,11 +133,14 @@ func buildRustWorkspaceIndex(ctx context.Context, root string, analyses []FileAn if err := ctx.Err(); err != nil { return nil, nil, err } + if metadataCtx.Err() != nil { + break + } manifestPath = filepath.Clean(manifestPath) if handledManifests[manifestPath] { continue } - data, err := loader(ctx, manifestPath) + data, err := loader(metadataCtx, manifestPath) if err != nil { continue } diff --git a/scanner/sg-rules/javascript.yml b/scanner/sg-rules/javascript.yml index 892ec11..a9d5c32 100644 --- a/scanner/sg-rules/javascript.yml +++ b/scanner/sg-rules/javascript.yml @@ -2,9 +2,15 @@ id: js-imports language: javascript rule: any: - - pattern: import $$$_ from "$PATH" - - pattern: import "$PATH" - - pattern: require("$PATH") + # Structural, so quote style stops mattering: the literal-text patterns + # this replaces were hardcoded to double quotes, and single quotes are the + # Prettier default, so single-quoted imports and requires were invisible. + - kind: import_statement + # require() is a call, not an import_statement, so a kind rule alone would + # lose CommonJS entirely. $$$ binds no $PATH metavariable, which keeps + # path extraction on the quote-agnostic text path and lets a dynamic + # require(someVar) resolve to nothing rather than to a fabricated edge. + - pattern: require($$$) --- id: js-functions language: javascript diff --git a/scanner/sg-rules/jsx.yml b/scanner/sg-rules/jsx.yml index b09fc2d..3d67699 100644 --- a/scanner/sg-rules/jsx.yml +++ b/scanner/sg-rules/jsx.yml @@ -1,7 +1,11 @@ id: jsx-imports language: jsx rule: - kind: import_statement + any: + - kind: import_statement + # CommonJS require() is a call expression, so kind: import_statement alone + # never saw it, in either quote style. + - pattern: require($$$) --- id: jsx-functions language: jsx diff --git a/scanner/sg-rules/tsx.yml b/scanner/sg-rules/tsx.yml index 3d13096..3dea6b6 100644 --- a/scanner/sg-rules/tsx.yml +++ b/scanner/sg-rules/tsx.yml @@ -1,7 +1,11 @@ id: tsx-imports language: tsx rule: - kind: import_statement + any: + - kind: import_statement + # CommonJS require() is a call expression, so kind: import_statement alone + # never saw it, in either quote style. + - pattern: require($$$) --- id: tsx-functions language: tsx diff --git a/scanner/sg-rules/typescript.yml b/scanner/sg-rules/typescript.yml index 597cd54..0ab7039 100644 --- a/scanner/sg-rules/typescript.yml +++ b/scanner/sg-rules/typescript.yml @@ -1,7 +1,11 @@ id: ts-imports language: typescript rule: - kind: import_statement + any: + - kind: import_statement + # CommonJS require() is a call expression, so kind: import_statement alone + # never saw it, in either quote style. + - pattern: require($$$) --- id: ts-functions language: typescript diff --git a/testdata/commonjs-single-quotes/app.js b/testdata/commonjs-single-quotes/app.js new file mode 100644 index 0000000..cf6d07b --- /dev/null +++ b/testdata/commonjs-single-quotes/app.js @@ -0,0 +1,10 @@ +// Double quotes still work, and an ESM-style single-quoted import is picked +// up too — both were broken for plain .js before. +const members = require("./routes/members"); +import admin from './routes/admin'; + +// A dynamic require must resolve to nothing rather than a fabricated edge. +const which = process.env.ROUTE; +const dynamic = require(which); + +module.exports = { members, admin, dynamic }; diff --git a/testdata/commonjs-single-quotes/routes/admin.js b/testdata/commonjs-single-quotes/routes/admin.js new file mode 100644 index 0000000..fc01324 --- /dev/null +++ b/testdata/commonjs-single-quotes/routes/admin.js @@ -0,0 +1,5 @@ +const layout = require('../services/layoutInputService'); + +module.exports = function admin() { + return layout.build(); +}; diff --git a/testdata/commonjs-single-quotes/routes/members.js b/testdata/commonjs-single-quotes/routes/members.js new file mode 100644 index 0000000..41d9f92 --- /dev/null +++ b/testdata/commonjs-single-quotes/routes/members.js @@ -0,0 +1,5 @@ +const layout = require('./../services/layoutInputService'); + +module.exports = function members() { + return layout.build(); +}; diff --git a/testdata/commonjs-single-quotes/services/layoutInputService.js b/testdata/commonjs-single-quotes/services/layoutInputService.js new file mode 100644 index 0000000..1987f16 --- /dev/null +++ b/testdata/commonjs-single-quotes/services/layoutInputService.js @@ -0,0 +1,5 @@ +// The shared module from the report: many files require it, all with the +// single quotes that Prettier writes by default. +module.exports.build = function build() { + return {}; +};