fix imported feature detection (#411)

src/javascript/features.ts

@@ -2,27 +2,88 @@ import type {CallExpression, Identifier, Literal, Node, TemplateLiteral} from "a
 import {simple} from "acorn-walk";
 import {getLocalPath} from "../files.js";
 import type {Feature} from "../javascript.js";
+import {defaultGlobals} from "./globals.js";
+import {findReferences} from "./references.js";
 import {syntaxError} from "./syntaxError.js";
 
 export function findFeatures(node: Node, path: string, references: Identifier[], input: string): Feature[] {
+  const featureMap = getFeatureReferenceMap(node);
   const features: Feature[] = [];
 
   simple(node, {
     CallExpression(node) {
       const {callee} = node;
-      // Ignore function calls that are not references to the feature. For
-      // example, if there’s a local variable called Secret, that will mask the
-      // built-in Secret and won’t be considered a feature.
-      if (callee.type !== "Identifier" || !references.includes(callee)) return;
-      const {name: type} = callee;
-      if (type !== "Secret" && type !== "FileAttachment" && type !== "DatabaseClient") return;
+      if (callee.type !== "Identifier") return;
+      let type = featureMap.get(callee);
+      // If this feature wasn’t explicitly imported into this cell, then ignore
+      // function calls that are not references to the feature. For example, if
+      // there’s a local variable called Secret, that will mask the built-in
+      // Secret and won’t be considered a feature.
+      if (!type) {
+        if (!references.includes(callee)) return;
+        const name = callee.name;
+        if (name !== "Secret" && name !== "FileAttachment" && name !== "DatabaseClient") return;
+        type = name;
+      }
       features.push(getFeature(type, node, path, input));
     }
   });
 
   return features;
 }
 
+/**
+ * Returns a map from Identifier to the feature type, such as FileAttachment.
+ * Note that this may be different than the identifier.name because of aliasing.
+ */
+export function getFeatureReferenceMap(node: Node): Map<Identifier, Feature["type"]> {
+  const declarations = new Set<{name: string}>();
+  const alias = new Map<string, Feature["type"]>();
+  let globals: Set<string> | undefined;
+
+  // Find the declared local names of the imported symbol. Only named imports
+  // are supported. TODO Support namespace imports?
+  simple(node, {
+    ImportDeclaration(node) {
+      if (node.source.value === "npm:@observablehq/stdlib") {
+        for (const specifier of node.specifiers) {
+          if (
+            specifier.type === "ImportSpecifier" &&
+            specifier.imported.type === "Identifier" &&
+            (specifier.imported.name === "FileAttachment" ||
+              specifier.imported.name === "Secret" ||
+              specifier.imported.name === "DatabaseClient")
+          ) {
+            declarations.add(specifier.local);
+            alias.set(specifier.local.name, specifier.imported.name);
+          }
+        }
+      }
+    }
+  });
+
+  // If the import is masking a global, don’t treat it as a global (since we’ll
+  // ignore the import declaration below).
+  for (const name of alias.keys()) {
+    if (defaultGlobals.has(name)) {
+      if (globals === undefined) globals = new Set(defaultGlobals);
+      globals.delete(name);
+    }
+  }
+
+  function filterDeclaration(node: {name: string}): boolean {
+    return !declarations.has(node); // treat the imported declaration as unbound
+  }
+
+  const references = findReferences(node, {globals, filterDeclaration});
+  const map = new Map<Identifier, Feature["type"]>();
+  for (const r of references) {
+    const type = alias.get(r.name);
+    if (type) map.set(r, type);
+  }
+  return map;
+}
+
 export function getFeature(type: Feature["type"], node: CallExpression, path: string, input: string): Feature {
   const {
     arguments: args,

src/javascript/imports.ts

@@ -10,9 +10,7 @@ import {type Feature, type ImportReference, type JavaScriptNode} from "../javasc
 import {parseOptions} from "../javascript.js";
 import {Sourcemap} from "../sourcemap.js";
 import {relativeUrl, resolvePath} from "../url.js";
-import {getFeature, getStringLiteralValue, isStringLiteral} from "./features.js";
-import {defaultGlobals} from "./globals.js";
-import {findReferences} from "./references.js";
+import {getFeature, getFeatureReferenceMap, getStringLiteralValue, isStringLiteral} from "./features.js";
 
 type ImportNode = ImportDeclaration | ImportExpression;
 type ExportNode = ExportAllDeclaration | ExportNamedDeclaration;
@@ -132,58 +130,6 @@ export function parseLocalImports(root: string, paths: string[]): ImportsAndFeat
   return {imports, features};
 }
 
-/**
- * Returns a map from Identifier to the feature type, such as FileAttachment.
- * Note that this may be different than the identifier.name because of aliasing.
- */
-export function getFeatureReferenceMap(node: Node): Map<Identifier, Feature["type"]> {
-  const declarations = new Set<{name: string}>();
-  const alias = new Map<string, Feature["type"]>();
-  let globals: Set<string> | undefined;
-
-  // Find the declared local names of the imported symbol. Only named imports
-  // are supported. TODO Support namespace imports?
-  simple(node, {
-    ImportDeclaration(node) {
-      if (node.source.value === "npm:@observablehq/stdlib") {
-        for (const specifier of node.specifiers) {
-          if (
-            specifier.type === "ImportSpecifier" &&
-            specifier.imported.type === "Identifier" &&
-            (specifier.imported.name === "FileAttachment" ||
-              specifier.imported.name === "Secret" ||
-              specifier.imported.name === "DatabaseClient")
-          ) {
-            declarations.add(specifier.local);
-            alias.set(specifier.local.name, specifier.imported.name);
-          }
-        }
-      }
-    }
-  });
-
-  // If the import is masking a global, don’t treat it as a global (since we’ll
-  // ignore the import declaration below).
-  for (const name of alias.keys()) {
-    if (defaultGlobals.has(name)) {
-      if (globals === undefined) globals = new Set(defaultGlobals);
-      globals.delete(name);
-    }
-  }
-
-  function filterDeclaration(node: {name: string}): boolean {
-    return !declarations.has(node); // treat the imported declaration as unbound
-  }
-
-  const references = findReferences(node, {globals, filterDeclaration});
-  const map = new Map<Identifier, Feature["type"]>();
-  for (const r of references) {
-    const type = alias.get(r.name);
-    if (type) map.set(r, type);
-  }
-  return map;
-}
-
 export function findImportFeatures(node: Node, path: string, input: string): Feature[] {
   const featureMap = getFeatureReferenceMap(node);
   const features: Feature[] = [];

test/javascript/features-test.ts

@@ -0,0 +1,34 @@
+import assert from "node:assert";
+import type {Program} from "acorn";
+import {Parser} from "acorn";
+import {findFeatures} from "../../src/javascript/features.js";
+import {findReferences} from "../../src/javascript/references.js";
+
+describe("findFeatures(node, path, references, input)", () => {
+  it("finds FileAttachment", () => {
+    assert.deepStrictEqual(features('FileAttachment("foo.json")'), [{type: "FileAttachment", name: "foo.json"}]);
+  });
+  it("finds Secret", () => {
+    assert.deepStrictEqual(features('Secret("foo")'), [{type: "Secret", name: "foo"}]);
+  });
+  it("finds DatabaseClient", () => {
+    assert.deepStrictEqual(features('DatabaseClient("foo")'), [{type: "DatabaseClient", name: "foo"}]);
+  });
+  it("finds imported FileAttachment", () => {
+    assert.deepStrictEqual(features('import {FileAttachment} from "npm:@observablehq/stdlib";\nFileAttachment("foo.json")'), [{type: "FileAttachment", name: "foo.json"}]); // prettier-ignore
+    assert.deepStrictEqual(features('import {FileAttachment as F} from "npm:@observablehq/stdlib";\nF("foo.json")'), [{type: "FileAttachment", name: "foo.json"}]); // prettier-ignore
+  });
+  it("finds imported DatabaseClient", () => {
+    assert.deepStrictEqual(features('import {DatabaseClient} from "npm:@observablehq/stdlib";\nDatabaseClient("foo")'), [{type: "DatabaseClient", name: "foo"}]); // prettier-ignore
+    assert.deepStrictEqual(features('import {DatabaseClient as D} from "npm:@observablehq/stdlib";\nD("foo")'), [{type: "DatabaseClient", name: "foo"}]); // prettier-ignore
+  });
+});
+
+function features(input) {
+  const node = parse(input);
+  return findFeatures(node, "index.js", findReferences(node), input);
+}
+
+function parse(input: string): Program {
+  return Parser.parse(input, {ecmaVersion: 13, sourceType: "module"});
+}

test/javascript/imports-test.ts

@@ -2,7 +2,8 @@ import assert from "node:assert";
 import type {Node, Program} from "acorn";
 import {Parser} from "acorn";
 import {ascending} from "d3-array";
-import {getFeatureReferenceMap, parseLocalImports, rewriteModule} from "../../src/javascript/imports.js";
+import {getFeatureReferenceMap} from "../../src/javascript/features.js";
+import {parseLocalImports, rewriteModule} from "../../src/javascript/imports.js";
 import type {Feature, ImportReference} from "../../src/javascript.js";
 
 describe("parseLocalImports(root, paths)", () => {