chore(compartment-mapper): review updates, more tests

- Adds tests for more situations - Fixes prevention of overriding builtin parsers - types for various options consolidated
endojs · May 30, 2024 · e5f88ee · e5f88ee
1 parent 2bc1e93
commit e5f88ee
Show file tree

Hide file tree

Showing 7 changed files with 337 additions and 63 deletions.
diff --git a/packages/compartment-mapper/src/archive.js b/packages/compartment-mapper/src/archive.js
@@ -1,7 +1,7 @@
 // @ts-check
 /* eslint no-shadow: 0 */
 
-/** @import {ArchiveOptions} from './types.js' */
+/** @import {ArchiveOptions, ParserForLanguage} from './types.js' */
 /** @import {ArchiveWriter} from './types.js' */
 /** @import {CompartmentDescriptor} from './types.js' */
 /** @import {CompartmentMapDescriptor} from './types.js' */
@@ -39,16 +39,20 @@ import { detectAttenuators } from './policy.js';
 
 const textEncoder = new TextEncoder();
 
-/** @type {Record<string, ParserImplementation>} */
-const parserForLanguage = {
-  mjs: parserArchiveMjs,
-  'pre-mjs-json': parserArchiveMjs,
-  cjs: parserArchiveCjs,
-  'pre-cjs-json': parserArchiveCjs,
-  json: parserJson,
-  text: parserText,
-  bytes: parserBytes,
-};
+const { freeze } = Object;
+
+/** @satisfies {Readonly<ParserForLanguage>} */
+const parserForLanguage = freeze(
+  /** @type {const} */ ({
+    mjs: parserArchiveMjs,
+    'pre-mjs-json': parserArchiveMjs,
+    cjs: parserArchiveCjs,
+    'pre-cjs-json': parserArchiveCjs,
+    json: parserJson,
+    text: parserText,
+    bytes: parserBytes,
+  }),
+);
 
 /**
  * @param {string} rel - a relative URL

diff --git a/packages/compartment-mapper/src/import-archive.js b/packages/compartment-mapper/src/import-archive.js
@@ -1,6 +1,23 @@
 // @ts-check
 /* eslint no-shadow: "off" */
 
+/** @import {ImportHook} from 'ses' */
+/** @import {ModuleExportsNamespace} from 'ses' */
+/** @import {StaticModuleType} from 'ses' */
+/** @import {Application} from './types.js' */
+/** @import {CompartmentDescriptor} from './types.js' */
+/** @import {ComputeSourceLocationHook} from './types.js' */
+/** @import {ComputeSourceMapLocationHook} from './types.js' */
+/** @import {ExecuteFn} from './types.js' */
+/** @import {ExitModuleImportHook} from './types.js' */
+/** @import {HashFn} from './types.js' */
+/** @import {ImportHookMaker} from './types.js' */
+/** @import {LoadArchiveOptions} from './types.js' */
+/** @import {ParserForLanguage} from './types.js' */
+/** @import {ReadFn} from './types.js' */
+/** @import {ReadPowers} from './types.js' */
+/** @import {SomeObject} from './types.js' */
+
 import { ZipReader } from '@endo/zip';
 import { link } from './link.js';
 import parserPreCjs from './parse-pre-cjs.js';
@@ -15,9 +32,6 @@ import { assertCompartmentMap } from './compartment-map.js';
 import { exitModuleImportHookMaker } from './import-hook.js';
 import { attenuateModuleHook, enforceModulePolicy } from './policy.js';
 
-/** @import {StaticModuleType} from 'ses' */
-/** @import {Application, CompartmentDescriptor, ComputeSourceLocationHook, ComputeSourceMapLocationHook, ExecuteFn, ExecuteOptions, ExitModuleImportHook, HashFn, ImportHookMaker, LoadArchiveOptions, ParserImplementation, ReadPowers} from './types.js' */
-
 const DefaultCompartment = Compartment;
 
 const { Fail, quote: q } = assert;
@@ -26,14 +40,14 @@ const textDecoder = new TextDecoder();
 
 const { freeze } = Object;
 
-/** @type {Record<string, ParserImplementation>} */
-const parserForLanguage = {
+/** @satisfies {ParserForLanguage} */
+const parserForLanguage = /** @type {const} */ ({
   'pre-cjs-json': parserPreCjs,
   'pre-mjs-json': parserPreMjs,
   json: parserJson,
   text: parserText,
   bytes: parserBytes,
-};
+});
 
 /**
  * @param {string} errorMessage - error to throw on execute
@@ -87,7 +101,7 @@ const makeArchiveImportHookMaker = (
     // per-compartment:
     const compartmentDescriptor = compartments[packageLocation];
     const { modules } = compartmentDescriptor;
-    /** @type {import('ses').ImportHook} */
+    /** @type {ImportHook} */
     const importHook = async moduleSpecifier => {
       // per-module:
       const module = modules[moduleSpecifier];
@@ -216,7 +230,7 @@ const makeArchiveImportHookMaker = (
  * Creates a fake module namespace object that passes a brand check.
  *
  * @param {typeof Compartment} Compartment
- * @returns {import('ses').ModuleExportsNamespace}
+ * @returns {ModuleExportsNamespace}
  */
 const makeFauxModuleExportsNamespace = Compartment => {
   const compartment = new Compartment(
@@ -404,7 +418,7 @@ export const parseArchive = async (
 };
 
 /**
- * @param {import('@endo/zip').ReadFn | ReadPowers} readPowers
+ * @param {ReadFn | ReadPowers} readPowers
  * @param {string} archiveLocation
  * @param {LoadArchiveOptions} [options]
  * @returns {Promise<Application>}
@@ -432,10 +446,10 @@ export const loadArchive = async (
 };
 
 /**
- * @param {import('@endo/zip').ReadFn | ReadPowers} readPowers
+ * @param {ReadFn | ReadPowers} readPowers
  * @param {string} archiveLocation
- * @param {ExecuteOptions & LoadArchiveOptions} options
- * @returns {Promise<object>}
+ * @param {LoadArchiveOptions} options
+ * @returns {Promise<SomeObject>}
  */
 export const importArchive = async (readPowers, archiveLocation, options) => {
   const archive = await loadArchive(readPowers, archiveLocation, options);

diff --git a/packages/compartment-mapper/src/import.js b/packages/compartment-mapper/src/import.js
@@ -1,13 +1,14 @@
 // @ts-check
 /* eslint no-shadow: "off" */
 
-/** @import {Application, ExtraImportOptions} from './types.js' */
-/** @import {ArchiveOptions} from './types.js' */
+/** @import {Application} from './types.js' */
+/** @import {ImportLocationOptions} from './types.js' */
+/** @import {LoadLocationOptions} from './types.js' */
+/** @import {ParserForLanguage} from './types.js' */
 /** @import {ExecuteFn} from './types.js' */
-/** @import {ExecuteOptions} from './types.js' */
-/** @import {ParserImplementation} from './types.js' */
 /** @import {ReadFn} from './types.js' */
 /** @import {ReadPowers} from './types.js' */
+/** @import {SomeObject} from './types.js' */
 
 import { compartmentMapForNodeModules } from './node-modules.js';
 import { search } from './search.js';
@@ -24,19 +25,23 @@ import parserMjs from './parse-mjs.js';
 import { parseLocatedJson } from './json.js';
 import { unpackReadPowers } from './powers.js';
 
-/** @type {Record<string, ParserImplementation>} */
-export const parserForLanguage = {
-  mjs: parserMjs,
-  cjs: parserCjs,
-  json: parserJson,
-  text: parserText,
-  bytes: parserBytes,
-};
+const { freeze } = Object;
+
+/** @satisfies {Readonly<ParserForLanguage>} */
+export const parserForLanguage = freeze(
+  /** @type {const} */ ({
+    mjs: parserMjs,
+    cjs: parserCjs,
+    json: parserJson,
+    text: parserText,
+    bytes: parserBytes,
+  }),
+);
 
 /**
  * @param {ReadFn | ReadPowers} readPowers
  * @param {string} moduleLocation
- * @param {ArchiveOptions & ExtraImportOptions} [options]
+ * @param {LoadLocationOptions} [options]
  * @returns {Promise<Application>}
  */
 export const loadLocation = async (readPowers, moduleLocation, options) => {
@@ -116,8 +121,8 @@ export const loadLocation = async (readPowers, moduleLocation, options) => {
 /**
  * @param {ReadFn | ReadPowers} readPowers
  * @param {string} moduleLocation
- * @param {ExecuteOptions & ArchiveOptions & ExtraImportOptions} [options]
- * @returns {Promise<import('./types.js').SomeObject>} the object of the imported modules exported
+ * @param {ImportLocationOptions} [options]
+ * @returns {Promise<SomeObject>} the object of the imported modules exported
  * names.
  */
 export const importLocation = async (

diff --git a/packages/compartment-mapper/src/link.js b/packages/compartment-mapper/src/link.js
@@ -1,8 +1,7 @@
 // @ts-check
 
 /** @import {ModuleMapHook} from 'ses' */
-/** @import {ResolveHook} from 'ses' */
-/** @import {ExtraImportOptions, ParseFn} from './types.js' */
+/** @import {ParseFn, ParserForLanguage} from './types.js' */
 /** @import {ParserImplementation} from './types.js' */
 /** @import {ShouldDeferError} from './types.js' */
 /** @import {ModuleTransforms} from './types.js' */
@@ -66,7 +65,7 @@ const extensionImpliesLanguage = extension => extension !== 'js';
  * @param {Record<string, string>} languageForModuleSpecifier - In a rare case,
  * the type of a module is implied by package.json and should not be inferred
  * from its extension.
- * @param {Record<string, ParserImplementation>} parserForLanguage
+ * @param {ParserForLanguage} parserForLanguage
  * @param {ModuleTransforms} moduleTransforms
  * @returns {ParseFn}
  */
@@ -124,7 +123,9 @@ const makeExtensionParser = (
         `Cannot parse module ${specifier} at ${location}, no parser configured for the language ${language}`,
       );
     }
-    const { parse } = parserForLanguage[language];
+    const { parse } = /** @type {ParserImplementation} */ (
+      parserForLanguage[language]
+    );
     return parse(bytes, specifier, location, packageLocation, {
       sourceMap,
       ...options,
@@ -136,7 +137,7 @@ const makeExtensionParser = (
  * @param {Record<string, Language>} languageForExtension
  * @param {Record<string, string>} languageForModuleSpecifier - In a rare case, the type of a module
  * is implied by package.json and should not be inferred from its extension.
- * @param {Record<string, ParserImplementation>} parserForLanguage
+ * @param {ParserForLanguage} parserForLanguage
  * @param {ModuleTransforms} moduleTransforms
  * @returns {ParseFn}
  */
@@ -325,7 +326,7 @@ const makeModuleMapHook = (
  * only.
  *
  * @param {CompartmentMapDescriptor} compartmentMap
- * @param {LinkOptions & ExtraImportOptions} options
+ * @param {LinkOptions} options
  */
 export const link = (
   { entry, compartments: compartmentDescriptors },
@@ -359,25 +360,38 @@ export const link = (
 
   /** @type {Record<string, Language>} */
   const customLanguageForExtension = Object.create(null);
+  /** @type {ParserForLanguage} */
+  const customParserForLanguage = Object.create(null);
+
   for (const { parser, extensions, language } of parsers) {
     if (
-      language in parserForLanguage &&
-      parserForLanguage[language] !== parser
+      language in customParserForLanguage &&
+      customParserForLanguage[language] !== parser
     ) {
-      throw new Error(`Parser for language ${q(language)} already defined`);
+      throw new Error(
+        `Custom parser for language ${q(language)} already defined`,
+      );
     }
-    parserForLanguage[language] = parser;
+
+    if (language in parserForLanguage) {
+      throw new Error(
+        `Parser for language ${q(language)} already defined by builtin`,
+      );
+    }
+
     for (const extension of extensions) {
       if (
         extension in customLanguageForExtension &&
         customLanguageForExtension[extension] !== language
       ) {
         throw new Error(
-          `Extension ${q(extension)} already assigned language ${q(customLanguageForExtension[extension])}`,
+          `Extension ${q(extension)} already assigned custom language ${q(customLanguageForExtension[extension])}`,
         );
       }
       customLanguageForExtension[extension] = language;
     }
+
+    customParserForLanguage[language] = parser;
   }
 
   for (const [compartmentName, compartmentDescriptor] of entries(
@@ -397,20 +411,42 @@ export const link = (
     // The `moduleMapHook` writes back to the compartment map.
     compartmentDescriptor.modules = modules;
 
+    /** @type {Record<string, Language>} */
+    const finalLanguageForExtension = Object.create(null);
     for (const [extension, language] of entries(customLanguageForExtension)) {
-      languageForExtension[extension] = language;
+      if (extension in languageForExtension) {
+        throw new Error(
+          `Extension ${q(extension)} already assigned language ${q(languageForExtension[extension])}`,
+        );
+      }
+      finalLanguageForExtension[extension] = language;
+    }
+    for (const [extension, language] of entries(languageForExtension)) {
+      finalLanguageForExtension[extension] = language;
+    }
+
+    /** @type {ParserForLanguage} */
+    const finalParserForLanguage = Object.create(null);
+
+    for (const [language, parser] of [
+      ...entries(customParserForLanguage),
+      ...entries(parserForLanguage),
+    ]) {
+      finalParserForLanguage[language] = parser;
     }
 
     const parse = mapParsers(
-      languageForExtension,
+      finalLanguageForExtension,
       languageForModuleSpecifier,
-      parserForLanguage,
+      finalParserForLanguage,
       moduleTransforms,
     );
     /** @type {ShouldDeferError} */
     const shouldDeferError = language => {
-      if (language && has(parserForLanguage, language)) {
-        return parserForLanguage[language].heuristicImports;
+      if (language && has(finalParserForLanguage, language)) {
+        return /** @type {ParserImplementation} */ (
+          finalParserForLanguage[language]
+        ).heuristicImports;
       } else {
         // If language is undefined or there's no parser, the error we could consider deferring is surely related to
         // that. Nothing to throw here.