From 451274c2695d8d0e52d1872e071137e2c93994a3 Mon Sep 17 00:00:00 2001 From: Nick Guerrera Date: Wed, 2 Jun 2021 15:20:45 -0700 Subject: [PATCH] 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 | 6 - packages/adl/compiler/util.ts | 28 ++++ packages/adl/config/config-loader.ts | 132 +++++++----------- packages/adl/config/config-validator.ts | 4 - packages/adl/config/types.ts | 7 - packages/adl/test/config/config.ts | 58 +++++++- .../test/config/scenarios/json/.adlrc.json | 5 + .../scenarios/package-json/package.json | 5 + .../scenarios/{yaml => yaml-json}/.adlrc.json | 0 .../config/scenarios/yaml-json/.adlrc.yaml | 4 + .../test/config/scenarios/yaml/.adlrc.yaml | 4 + 11 files changed, 146 insertions(+), 107 deletions(-) rename packages/adl/test/config/scenarios/{yaml => yaml-json}/.adlrc.json (100%) diff --git a/packages/adl/compiler/messages.ts b/packages/adl/compiler/messages.ts index 133bd4cb3..5b0f23b4f 100644 --- a/packages/adl/compiler/messages.ts +++ b/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. diff --git a/packages/adl/compiler/util.ts b/packages/adl/compiler/util.ts index b620078a4..5fee3e894 100644 --- a/packages/adl/compiler/util.ts +++ b/packages/adl/compiler/util.ts @@ -42,6 +42,34 @@ export function reportDuplicateSymbols(symbols: SymbolTable) { } } +export function deepFreeze(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(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 }), diff --git a/packages/adl/config/config-loader.ts b/packages/adl/config/config-loader.ts index ff86f67c6..85965ce1d 100644 --- a/packages/adl/config/config-loader.ts +++ b/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 { 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 { 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 { - const rawConfig = await loadJSON(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 { - const rawConfig = await loadJSON(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 { - 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> { // 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(filePath: string): Promise> { - 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): 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 { + 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]); + } } } diff --git a/packages/adl/config/config-validator.ts b/packages/adl/config/config-validator.ts index faaaed81c..42bf81656 100644 --- a/packages/adl/config/config-validator.ts +++ b/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); diff --git a/packages/adl/config/types.ts b/packages/adl/config/types.ts index e078130fb..de738b41c 100644 --- a/packages/adl/config/types.ts +++ b/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; emitters?: Record; } - -export interface ConfigFile { - file: SourceFile; - data: T; -} diff --git a/packages/adl/test/config/config.ts b/packages/adl/test/config/config.ts index 07b1f4a55..d51a7f632 100644 --- a/packages/adl/test/config/config.ts +++ b/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", () => { diff --git a/packages/adl/test/config/scenarios/json/.adlrc.json b/packages/adl/test/config/scenarios/json/.adlrc.json index 9a302c2d8..1dc207a32 100644 --- a/packages/adl/test/config/scenarios/json/.adlrc.json +++ b/packages/adl/test/config/scenarios/json/.adlrc.json @@ -2,5 +2,10 @@ "plugins": ["foo"], "emitters": { "foo:openapi": true + }, + "lint": { + "rules": { + "some-rule": "on" + } } } diff --git a/packages/adl/test/config/scenarios/package-json/package.json b/packages/adl/test/config/scenarios/package-json/package.json index cc2f4f645..a569bcab5 100644 --- a/packages/adl/test/config/scenarios/package-json/package.json +++ b/packages/adl/test/config/scenarios/package-json/package.json @@ -5,6 +5,11 @@ ], "emitters": { "foo:openapi": true + }, + "lint": { + "rules": { + "some-rule": "on" + } } } } diff --git a/packages/adl/test/config/scenarios/yaml/.adlrc.json b/packages/adl/test/config/scenarios/yaml-json/.adlrc.json similarity index 100% rename from packages/adl/test/config/scenarios/yaml/.adlrc.json rename to packages/adl/test/config/scenarios/yaml-json/.adlrc.json diff --git a/packages/adl/test/config/scenarios/yaml-json/.adlrc.yaml b/packages/adl/test/config/scenarios/yaml-json/.adlrc.yaml index 90baaf1ea..6a0d62b9a 100644 --- a/packages/adl/test/config/scenarios/yaml-json/.adlrc.yaml +++ b/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" diff --git a/packages/adl/test/config/scenarios/yaml/.adlrc.yaml b/packages/adl/test/config/scenarios/yaml/.adlrc.yaml index 90baaf1ea..cf462ad4d 100644 --- a/packages/adl/test/config/scenarios/yaml/.adlrc.yaml +++ b/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