Facilitate merge of config file loading with semantic error recovery (#572)

It ended up being tricky to merge my pending semantic error recovery change
with the config file loading change. This change extracts out some changes
to config file loading from my error recovery branch as it will be easier to
review this independently from that.

* Don't bother making a diagnostic for incorrect file extension, as compiler
  should never attempt that. It's OK to throw a regular error in this case.
* Fix issue with returning or spreading default config where mutation of
  loaded config could mutate defaults.
* Avoid fileExists + readFile race condition by not using fileExists and
  catching readFile ENOENT error instead.
* Move extra json file for multi-file test to correct folder.
* Various small refactorings to make these changes easier.

packages/adl/compiler/messages.ts

@@ -65,12 +65,6 @@ export const Message = {
     text: `File {0} is not found.`,
     severity: "error",
   } as const,
-
-  InvalidConfigFormat: {
-    code: 1110,
-    text: `File {0} does not have a known configuration format.`,
-    severity: "error",
-  } as const,
 };
 
 // Static assert: this won't compile if one of the entries above is invalid.

packages/adl/compiler/util.ts

@@ -42,6 +42,34 @@ export function reportDuplicateSymbols(symbols: SymbolTable) {
   }
 }
 
+export function deepFreeze<T>(value: T): T {
+  if (Array.isArray(value)) {
+    value.map(deepFreeze);
+  } else if (typeof value === "object") {
+    for (const prop in value) {
+      deepFreeze(value[prop]);
+    }
+  }
+
+  return Object.freeze(value);
+}
+
+export function deepClone<T>(value: T): T {
+  if (Array.isArray(value)) {
+    return value.map(deepClone) as any;
+  }
+
+  if (typeof value === "object") {
+    const obj: any = {};
+    for (const prop in value) {
+      obj[prop] = deepClone(value[prop]);
+    }
+    return obj;
+  }
+
+  return value;
+}
+
 export const NodeHost: CompilerHost = {
   readFile: (path: string) => readFile(path, "utf-8"),
   readDir: (path: string) => readdir(path, { withFileTypes: true }),

packages/adl/config/config-loader.ts

@@ -1,19 +1,19 @@
-import { access, readFile } from "fs/promises";
+import { readFile } from "fs/promises";
 import { basename, extname, join } from "path";
-import { createSourceFile, Message, NoTarget, throwDiagnostic } from "../compiler/diagnostics.js";
+import { createSourceFile, throwDiagnostic } from "../compiler/diagnostics.js";
+import { deepClone, deepFreeze } from "../compiler/util.js";
 import { ConfigValidator } from "./config-validator.js";
-import { ADLConfig, ADLRawConfig, ConfigFile } from "./types.js";
+import { ADLConfig } from "./types.js";
 
 const configFilenames = [".adlrc.yaml", ".adlrc.yml", ".adlrc.json", "package.json"];
-
-const defaultConfig: ADLConfig = {
+const defaultConfig: ADLConfig = deepFreeze({
   plugins: [],
   emitters: {},
   lint: {
     extends: [],
     rules: {},
   },
-};
+});
 
 /**
  * Load ADL Configuration if present.
@@ -22,11 +22,16 @@ const defaultConfig: ADLConfig = {
 export async function loadADLConfigInDir(directoryPath: string): Promise<ADLConfig> {
   for (const filename of configFilenames) {
     const filePath = join(directoryPath, filename);
-    if (await fileExists(filePath)) {
-      return loadADLConfigFile(filePath);
+    try {
+      return await loadADLConfigFile(filePath);
+    } catch (e) {
+      if (e.code === "ENOENT") {
+        continue;
+      }
+      throw e;
     }
   }
-  return defaultConfig;
+  return deepClone(defaultConfig);
 }
 
 /**
@@ -45,99 +50,58 @@ export async function loadADLConfigFile(filePath: string): Promise<ADLConfig> {
       return loadYAMLConfigFile(filePath);
 
     default:
-      throwDiagnostic(Message.InvalidConfigFormat, NoTarget, [filePath]);
+      // This is not a diagnostic because the compiler only tries the
+      // well-known config file names.
+      throw new RangeError("Config file must have .yaml, .yml, or .json extension.");
   }
 }
 
 export async function loadPackageJSONConfigFile(filePath: string): Promise<ADLConfig> {
-  const rawConfig = await loadJSON<any>(filePath);
-  if (rawConfig.data.adl) {
-    return normalizeConfig({ file: rawConfig.file, data: rawConfig.data.adl });
-  } else {
-    return defaultConfig;
-  }
+  return await loadConfigFile(filePath, (content) => JSON.parse(content).adl ?? {});
 }
 
 export async function loadJSONConfigFile(filePath: string): Promise<ADLConfig> {
-  const rawConfig = await loadJSON<ADLRawConfig>(filePath);
-  return normalizeConfig(rawConfig);
+  return await loadConfigFile(filePath, JSON.parse);
 }
 
-/**
- * Loads a YAML configuration from a file.
- * @param filePath Path to the file.
- */
 export async function loadYAMLConfigFile(filePath: string): Promise<ADLConfig> {
-  const rawConfig = await loadYaml(filePath);
-  return normalizeConfig(rawConfig);
-}
-
-/**
- * Load YAML and throw @see DiagnosticError if there is an issue.
- * @param filePath Yaml file path.
- * @returns Parsed object.
- */
-async function loadYaml(filePath: string): Promise<ConfigFile<ADLRawConfig>> {
   // Lazy load.
   const jsyaml = await import("js-yaml");
-
-  const content = (await readFile(filePath)).toString();
-  const file = createSourceFile(content, filePath);
-  try {
-    return {
-      file,
-      data: jsyaml.load(content) as ADLRawConfig,
-    };
-  } catch (e) {
-    throwDiagnostic(e.message, { file, pos: 0, end: 0 });
-  }
-}
-
-/**
- * Load JSON and throw @see DiagnosticError if there is an issue.
- * @param filePath JSON file path.
- * @returns Parsed object.
- */
-async function loadJSON<T>(filePath: string): Promise<ConfigFile<T>> {
-  const content = (await readFile(filePath)).toString();
-  const file = createSourceFile(content, filePath);
-
-  try {
-    return {
-      file,
-      data: JSON.parse(content),
-    };
-  } catch (e) {
-    throwDiagnostic(e.message, { file, pos: 0, end: 0 });
-  }
+  return await loadConfigFile(filePath, jsyaml.load);
 }
 
 const configValidator = new ConfigValidator();
-export function normalizeConfig(config: ConfigFile<ADLRawConfig>): ADLConfig {
-  configValidator.validateConfig(config.data, config.file);
-  return {
-    filename: config.file.path,
-    ...defaultConfig,
-    ...config.data,
-    lint: {
-      ...defaultConfig.lint,
-      ...(config.data.lint ?? {}),
-    },
-  };
+
+async function loadConfigFile(
+  filePath: string,
+  loadData: (content: string) => any
+): Promise<ADLConfig> {
+  const content = await readFile(filePath, "utf-8");
+  const file = createSourceFile(content, filePath);
+
+  let config: any;
+  try {
+    config = loadData(content);
+  } catch (e) {
+    throwDiagnostic(e.message, { file, pos: 0, end: 0 });
+  }
+
+  configValidator.validateConfig(config, file);
+  mergeDefaults(config, defaultConfig);
+  config.filename = filePath;
+  return config;
 }
 
 /**
- * Validate the given config is valid.
+ * Recursively add properties from defaults that are not present in target.
  */
-export function validateConfig(config: ADLRawConfig) {
-  return {} as any;
-}
-
-async function fileExists(path: string) {
-  try {
-    await access(path);
-    return true;
-  } catch {
-    return false;
+function mergeDefaults(target: any, defaults: any) {
+  for (const prop in defaults) {
+    const value = target[prop];
+    if (value === undefined) {
+      target[prop] = deepClone(defaults[prop]);
+    } else if (typeof value === "object" && typeof defaults[prop] === "object") {
+      mergeDefaults(value, defaults[prop]);
+    }
   }
 }

packages/adl/config/config-validator.ts

@@ -16,10 +16,6 @@ export class ConfigValidator {
    * @returns
    */
   public validateConfig(config: ADLRawConfig, file?: SourceFile) {
-    this.validateSchema(config, file);
-  }
-
-  private validateSchema(config: ADLRawConfig, file?: SourceFile) {
     const validate = this.ajv.compile(ADLConfigJsonSchema);
     const valid = validate(config);
 

packages/adl/config/types.ts

@@ -1,5 +1,3 @@
-import { SourceFile } from "../compiler";
-
 /**
  * Represent the normalized user configuration.
  */
@@ -29,8 +27,3 @@ export interface ADLRawConfig {
   lint?: Partial<ADLLintConfig>;
   emitters?: Record<string, boolean>;
 }
-
-export interface ConfigFile<T> {
-  file: SourceFile;
-  data: T;
-}

packages/adl/test/config/config.ts

@@ -7,7 +7,7 @@ import { loadADLConfigInDir } from "../../config/index.js";
 
 const __dirname = dirname(fileURLToPath(import.meta.url));
 
-describe("adl: Config file loading", () => {
+describe("adl: config file loading", () => {
   describe("file discovery", async () => {
     const scenarioRoot = resolve(__dirname, "../../../test/config/scenarios");
     const loadTestConfig = async (folderName: string) => {
@@ -25,24 +25,26 @@ describe("adl: Config file loading", () => {
         },
         lint: {
           extends: [],
-          rules: {},
+          rules: {
+            "some-rule": "on",
+          },
         },
       });
     };
 
-    it("Loads yaml config file", async () => {
+    it("loads yaml config file", async () => {
       await assertLoadFromFolder("yaml");
     });
 
-    it("Loads json config file", async () => {
+    it("loads json config file", async () => {
       await assertLoadFromFolder("json");
     });
 
-    it("Loads from adl section in package.json config file", async () => {
+    it("loads from adl section in package.json config file", async () => {
       await assertLoadFromFolder("package-json");
     });
 
-    it("Loads empty config if it can't find any config files", async () => {
+    it("loads empty config if it can't find any config files", async () => {
       const config = await loadTestConfig("empty");
       deepStrictEqual(config, {
         plugins: [],
@@ -58,6 +60,50 @@ describe("adl: Config file loading", () => {
       // Should load .adlrc.yaml and MOT load .adlrc.json here
       await assertLoadFromFolder("yaml-json");
     });
+
+    it("deep clones defaults when not found", async () => {
+      // load and mutate
+      let config = await loadTestConfig("empty");
+      config.plugins.push("x");
+      config.emitters["x"] = true;
+      config.lint.extends.push("x");
+      config.lint.rules["x"] = "off";
+
+      // reload and make sure mutation is not observed
+      config = await loadTestConfig("empty");
+      deepStrictEqual(config, {
+        plugins: [],
+        emitters: {},
+        lint: {
+          extends: [],
+          rules: {},
+        },
+      });
+    });
+
+    it("deep clones defaults when found", async () => {
+      // load and mutate
+      let config = await loadTestConfig("yaml");
+      config.plugins.push("x");
+      config.emitters["x"] = true;
+      config.lint.extends.push("x");
+      config.lint.rules["x"] = "off";
+
+      // reload and make sure mutation is not observed
+      config = await loadTestConfig("yaml");
+      deepStrictEqual(config, {
+        plugins: ["foo"],
+        emitters: {
+          "foo:openapi": true,
+        },
+        lint: {
+          extends: [],
+          rules: {
+            "some-rule": "on",
+          },
+        },
+      });
+    });
   });
 
   describe("validation", () => {

packages/adl/test/config/scenarios/json/.adlrc.json

@@ -2,5 +2,10 @@
   "plugins": ["foo"],
   "emitters": {
     "foo:openapi": true
+  },
+  "lint": {
+    "rules": {
+      "some-rule": "on"
+    }
   }
 }

packages/adl/test/config/scenarios/package-json/package.json

@@ -5,6 +5,11 @@
     ],
     "emitters": {
       "foo:openapi": true
+    },
+    "lint": {
+      "rules": {
+        "some-rule": "on"
+      }
     }
   }
 }

packages/adl/test/config/scenarios/yaml-json/.adlrc.yaml

@@ -4,3 +4,7 @@ plugins:
 # This has comments
 emitters:
   foo:openapi: true
+
+lint:
+  rules:
+    "some-rule": "on"

packages/adl/test/config/scenarios/yaml/.adlrc.yaml

@@ -4,3 +4,7 @@ plugins:
 # This has comments
 emitters:
   foo:openapi: true
+
+lint:
+  rules:
+    some-rule: on