diff --git a/common/changes/@typespec/compiler/feature-absolute-path_2023-11-29-18-24.json b/common/changes/@typespec/compiler/feature-absolute-path_2023-11-29-18-24.json new file mode 100644 index 0000000000..bc12592689 --- /dev/null +++ b/common/changes/@typespec/compiler/feature-absolute-path_2023-11-29-18-24.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@typespec/compiler", + "comment": "Emitter API: Added `absolute-path` as a known format for emitter options which will validate the value passed by the user resolve to an absolute path.", + "type": "none" + } + ], + "packageName": "@typespec/compiler" +} \ No newline at end of file diff --git a/docs/extending-typespec/emitters-basics.md b/docs/extending-typespec/emitters-basics.md index 38c494ed54..8056229d5e 100644 --- a/docs/extending-typespec/emitters-basics.md +++ b/docs/extending-typespec/emitters-basics.md @@ -72,6 +72,24 @@ export async function $onEmit(context: EmitContext) { } ``` +### Emitter options known format: + +### `absolute-path` + +Specify that the value for this option should resolve to an absolute path. e.g. `"{project-root}/dir"`. + +:::important +It is recommended that all options that involve path use this. Using relative path can be confusing for users on as it is not clear what the relative path is relative to. And more importantly relative path if not careful are resolved relative to the `cwd` in node file system which result in spec only compiling from the the project root. +::: + +Example: + +```js +{ + "asset-dir": { type: "string", format: "absolute-path", nullable: true }, +} +``` + ### Configuration options convention - Name options `kebab-case`. So it can be inline with the rest of the cli diff --git a/packages/compiler/src/config/config-to-options.ts b/packages/compiler/src/config/config-to-options.ts index 5ad6c58c54..ff261a863c 100644 --- a/packages/compiler/src/config/config-to-options.ts +++ b/packages/compiler/src/config/config-to-options.ts @@ -94,6 +94,7 @@ export function resolveOptionsFromConfig(config: TypeSpecConfig, options: Config const resolvedOptions: CompilerOptions = omitUndefined({ outputDir: expandedConfig.outputDir, config: config.filename, + configFile: config, additionalImports: expandedConfig["imports"], warningAsError: expandedConfig.warnAsError, trace: expandedConfig.trace, diff --git a/packages/compiler/src/core/options.ts b/packages/compiler/src/core/options.ts index 470475d82f..ddc2bdc5ff 100644 --- a/packages/compiler/src/core/options.ts +++ b/packages/compiler/src/core/options.ts @@ -1,4 +1,4 @@ -import { EmitterOptions } from "../config/types.js"; +import { EmitterOptions, TypeSpecConfig } from "../config/types.js"; import { LinterRuleSet, ParseOptions } from "./types.js"; export interface CompilerOptions { @@ -11,7 +11,7 @@ export interface CompilerOptions { outputDir?: string; /** - * Path to config YAML file or folder in which to search for default tspconfig.yaml file. + * Path to config YAML file used, this is also where the project root should be. */ config?: string; @@ -63,4 +63,7 @@ export interface CompilerOptions { /** Ruleset to enable for linting. */ linterRuleSet?: LinterRuleSet; + + /** @internal */ + readonly configFile?: TypeSpecConfig; } diff --git a/packages/compiler/src/core/program.ts b/packages/compiler/src/core/program.ts index 0a6e804267..44bc1229f9 100644 --- a/packages/compiler/src/core/program.ts +++ b/packages/compiler/src/core/program.ts @@ -753,7 +753,13 @@ export async function compile( if (libDefinition?.emitter?.options) { const diagnostics = libDefinition?.emitterOptionValidator?.validate( emitterOptions, - NoTarget + options.configFile?.file + ? { + kind: "path-target", + path: ["options", emitterNameOrPath], + script: options.configFile.file, + } + : NoTarget ); if (diagnostics && diagnostics.length > 0) { program.reportDiagnostics(diagnostics); diff --git a/packages/compiler/src/core/schema-validator.ts b/packages/compiler/src/core/schema-validator.ts index 52e9ba9b8a..155966b89e 100644 --- a/packages/compiler/src/core/schema-validator.ts +++ b/packages/compiler/src/core/schema-validator.ts @@ -1,8 +1,17 @@ -import Ajv, { DefinedError, Options } from "ajv"; +import Ajv, { ErrorObject, Options } from "ajv"; import { getLocationInYamlScript } from "../yaml/diagnostics.js"; -import { YamlScript } from "../yaml/types.js"; +import { YamlPathTarget, YamlScript } from "../yaml/types.js"; import { compilerAssert } from "./diagnostics.js"; -import { Diagnostic, JSONSchemaType, JSONSchemaValidator, NoTarget, SourceFile } from "./types.js"; +import { createDiagnostic } from "./messages.js"; +import { isPathAbsolute } from "./path-utils.js"; +import { + Diagnostic, + DiagnosticTarget, + JSONSchemaType, + JSONSchemaValidator, + NoTarget, + SourceFile, +} from "./types.js"; export interface JSONSchemaValidatorOptions { coerceTypes?: boolean; @@ -13,17 +22,23 @@ export function createJSONSchemaValidator( schema: JSONSchemaType, options: JSONSchemaValidatorOptions = { strict: true } ): JSONSchemaValidator { - const ajv = new (Ajv as any)({ + const ajv: import("ajv").default = new (Ajv as any)({ strict: options.strict, coerceTypes: options.coerceTypes, allErrors: true, } satisfies Options); + ajv.addFormat("absolute-path", { + type: "string", + validate: (path) => { + return !path.startsWith(".") && isPathAbsolute(path); + }, + }); return { validate }; function validate( config: unknown, - target: YamlScript | SourceFile | typeof NoTarget + target: YamlScript | YamlPathTarget | SourceFile | typeof NoTarget ): Diagnostic[] { const validate = ajv.compile(schema); const valid = validate(config); @@ -34,7 +49,7 @@ export function createJSONSchemaValidator( const diagnostics = []; for (const error of validate.errors ?? []) { - const diagnostic = ajvErrorToDiagnostic(error, target); + const diagnostic = ajvErrorToDiagnostic(config, error, target); diagnostics.push(diagnostic); } @@ -45,9 +60,19 @@ export function createJSONSchemaValidator( const IGNORED_AJV_PARAMS = new Set(["type", "errors"]); function ajvErrorToDiagnostic( - error: DefinedError, - target: YamlScript | SourceFile | typeof NoTarget + obj: unknown, + error: ErrorObject, unknown>, + target: YamlScript | YamlPathTarget | SourceFile | typeof NoTarget ): Diagnostic { + const tspTarget = resolveTarget(error, target); + if (error.params.format === "absolute-path") { + return createDiagnostic({ + code: "config-path-absolute", + format: { path: getErrorValue(obj, error) as any }, + target: tspTarget, + }); + } + const messageLines = [`Schema violation: ${error.message} (${error.instancePath || "/"})`]; for (const [name, value] of Object.entries(error.params).filter( ([name]) => !IGNORED_AJV_PARAMS.has(name) @@ -61,16 +86,33 @@ function ajvErrorToDiagnostic( code: "invalid-schema", message, severity: "error", - target: - target === NoTarget - ? target - : "kind" in target - ? getLocationInYamlScript(target, getErrorPath(error), "key") - : { file: target, pos: 0, end: 0 }, + target: tspTarget, }; } -function getErrorPath(error: DefinedError): string[] { +function resolveTarget( + error: ErrorObject, unknown>, + target: YamlScript | YamlPathTarget | SourceFile | typeof NoTarget +): DiagnosticTarget | typeof NoTarget { + if (target === NoTarget) { + return NoTarget; + } + if (!("kind" in target)) { + return { file: target, pos: 0, end: 0 }; + } + switch (target.kind) { + case "yaml-script": + return getLocationInYamlScript(target, getErrorPath(error), "key"); + case "path-target": + return getLocationInYamlScript( + target.script, + [...target.path, ...getErrorPath(error)], + "key" + ); + } +} + +function getErrorPath(error: ErrorObject, unknown>): string[] { const instancePath = parseJsonPointer(error.instancePath); switch (error.keyword) { case "additionalProperties": @@ -79,6 +121,17 @@ function getErrorPath(error: DefinedError): string[] { return instancePath; } } +function getErrorValue( + obj: any, + error: ErrorObject, unknown> +): unknown { + const path = getErrorPath(error); + let current = obj; + for (const segment of path) { + current = current[segment]; + } + return current; +} /** * Converts a json pointer into a array of reference tokens diff --git a/packages/compiler/src/core/types.ts b/packages/compiler/src/core/types.ts index 0fc115dd15..e86207de73 100644 --- a/packages/compiler/src/core/types.ts +++ b/packages/compiler/src/core/types.ts @@ -1,7 +1,7 @@ import type { JSONSchemaType as AjvJSONSchemaType } from "ajv"; import { TypeEmitter } from "../emitter-framework/type-emitter.js"; import { AssetEmitter } from "../emitter-framework/types.js"; -import { YamlScript } from "../yaml/types.js"; +import { YamlPathTarget, YamlScript } from "../yaml/types.js"; import { ModuleResolutionResult } from "./module-resolver.js"; import { Program } from "./program.js"; import type { TokenFlags } from "./scanner.js"; @@ -2037,6 +2037,9 @@ export type TypeOfDiagnostics> = T extends Diagnost export type JSONSchemaType = AjvJSONSchemaType; +/** + * @internal + */ export interface JSONSchemaValidator { /** * Validate the configuration against its JSON Schema. @@ -2045,7 +2048,10 @@ export interface JSONSchemaValidator { * @param target Source file target to use for diagnostics. * @returns Diagnostics produced by schema validation of the configuration. */ - validate(config: unknown, target: YamlScript | SourceFile | typeof NoTarget): Diagnostic[]; + validate( + config: unknown, + target: YamlScript | YamlPathTarget | SourceFile | typeof NoTarget + ): Diagnostic[]; } /** @deprecated Use TypeSpecLibraryDef */ @@ -2166,6 +2172,7 @@ export interface TypeSpecLibrary< > extends TypeSpecLibraryDef { /** * JSON Schema validator for emitter options + * @internal */ readonly emitterOptionValidator?: JSONSchemaValidator; diff --git a/packages/compiler/src/yaml/types.ts b/packages/compiler/src/yaml/types.ts index 6fe808c26a..a13f45f23e 100644 --- a/packages/compiler/src/yaml/types.ts +++ b/packages/compiler/src/yaml/types.ts @@ -11,4 +11,12 @@ export interface YamlScript { readonly doc: Document.Parsed; } +/** + * Represent the location of a value in a yaml script. + */ +export interface YamlPathTarget { + kind: "path-target"; + script: YamlScript; + path: string[]; +} export type YamlDiagnosticTargetType = "value" | "key"; diff --git a/packages/compiler/test/cli.test.ts b/packages/compiler/test/cli.test.ts index 69569fccdd..c734e62242 100644 --- a/packages/compiler/test/cli.test.ts +++ b/packages/compiler/test/cli.test.ts @@ -1,4 +1,4 @@ -import { deepStrictEqual, strictEqual } from "assert"; +import { deepStrictEqual, ok, strictEqual } from "assert"; import { stringify } from "yaml"; import { TypeSpecRawConfig } from "../src/config/types.js"; import { CompileCliArgs, getCompilerOptions } from "../src/core/cli/actions/compile/args.js"; @@ -31,7 +31,9 @@ describe("compiler: cli", () => { env ); expectDiagnosticEmpty(diagnostics); - return options; + ok(options, "Options should have been set."); + const { configFile: config, ...rest } = options; + return rest; } it("no args and config: return empty options with output-dir at {cwd}/tsp-output", async () => { @@ -188,7 +190,7 @@ describe("compiler: cli", () => { interface TestUnifiedOptions< K extends keyof CompileCliArgs & keyof TypeSpecRawConfig, - T extends keyof CompilerOptions, + T extends Exclude, > { default: CompileCliArgs[K]; set: { in: CompileCliArgs[K]; alt: CompileCliArgs[K]; expected: CompilerOptions[T] }[]; @@ -196,7 +198,7 @@ describe("compiler: cli", () => { function testUnifiedOptions< K extends keyof CompileCliArgs & keyof TypeSpecRawConfig, - T extends keyof CompilerOptions, + T extends Exclude, >(name: K, resolvedName: T, data: TestUnifiedOptions) { describe(name, () => { it("default", async () => { diff --git a/packages/compiler/test/config/resolve-compiler-option.test.ts b/packages/compiler/test/config/resolve-compiler-option.test.ts index c05c19e848..33fda9d1f9 100644 --- a/packages/compiler/test/config/resolve-compiler-option.test.ts +++ b/packages/compiler/test/config/resolve-compiler-option.test.ts @@ -16,11 +16,15 @@ describe("compiler: resolve compiler options", () => { describe("specifying explicit config file", () => { const resolveOptions = async (path: string) => { const fullPath = resolvePath(scenarioRoot, path); - return await resolveCompilerOptions(NodeHost, { - cwd: normalizePath(process.cwd()), - entrypoint: fullPath, // not really used here - configPath: fullPath, - }); + const [{ configFile: config, ...options }, diagnostics] = await resolveCompilerOptions( + NodeHost, + { + cwd: normalizePath(process.cwd()), + entrypoint: fullPath, // not really used here + configPath: fullPath, + } + ); + return [options, diagnostics] as const; }; it("loads config at the given path", async () => { diff --git a/packages/compiler/test/core/emitter-options.test.ts b/packages/compiler/test/core/emitter-options.test.ts new file mode 100644 index 0000000000..1cc4d1d614 --- /dev/null +++ b/packages/compiler/test/core/emitter-options.test.ts @@ -0,0 +1,120 @@ +import { ok, strictEqual } from "assert"; +import { Diagnostic, EmitContext, createTypeSpecLibrary } from "../../src/index.js"; +import { expectDiagnosticEmpty, expectDiagnostics } from "../../src/testing/expect.js"; +import { createTestHost } from "../../src/testing/test-host.js"; + +const fakeEmitter = createTypeSpecLibrary({ + name: "fake-emitter", + diagnostics: {}, + emitter: { + options: { + type: "object", + properties: { + "asset-dir": { type: "string", format: "absolute-path", nullable: true }, + "max-files": { type: "number", nullable: true }, + }, + additionalProperties: false, + }, + }, +}); + +describe("compiler: emitter options", () => { + async function runWithEmitterOptions( + options: Record + ): Promise<[EmitContext | undefined, readonly Diagnostic[]]> { + let emitContext: EmitContext | undefined; + const host = await createTestHost(); + host.addTypeSpecFile("main.tsp", ""); + host.addTypeSpecFile( + "node_modules/fake-emitter/package.json", + JSON.stringify({ + main: "index.js", + }) + ); + host.addJsFile("node_modules/fake-emitter/index.js", { + $lib: fakeEmitter, + $onEmit: (ctx: EmitContext) => { + emitContext = ctx; + }, + }); + + const diagnostics = await host.diagnose("main.tsp", { + emit: ["fake-emitter"], + options: { + "fake-emitter": options, + }, + }); + return [emitContext, diagnostics]; + } + + async function diagnoseEmitterOptions( + options: Record + ): Promise { + const [_, diagnostics] = await runWithEmitterOptions(options); + return diagnostics; + } + + async function getEmitContext(options: Record): Promise { + const [context, diagnostics] = await runWithEmitterOptions(options); + expectDiagnosticEmpty(diagnostics); + ok(context, "Emit context should have been set."); + return context; + } + + it("pass options", async () => { + const context = await getEmitContext({ + "emitter-output-dir": "/out", + "asset-dir": "/assets", + "max-files": 10, + }); + + strictEqual(context.emitterOutputDir, "/out"); + strictEqual(context.options["asset-dir"], "/assets"); + strictEqual(context.options["max-files"], 10); + }); + + it("emit diagnostic if passing unknown option", async () => { + const diagnostics = await diagnoseEmitterOptions({ + "invalid-option": "abc", + }); + expectDiagnostics(diagnostics, { + code: "invalid-schema", + message: [ + "Schema violation: must NOT have additional properties (/)", + " additionalProperty: invalid-option", + ].join("\n"), + }); + }); + + it("emit diagnostic if passing invalid option type", async () => { + const diagnostics = await diagnoseEmitterOptions({ + "max-files": "not a number", + }); + expectDiagnostics(diagnostics, { + code: "invalid-schema", + message: "Schema violation: must be number (/max-files)", + }); + }); + + describe("format: absolute-path", () => { + it("emit diagnostic if passing relative path starting with `./`", async () => { + const diagnostics = await diagnoseEmitterOptions({ + "asset-dir": "./assets", + }); + expectDiagnostics(diagnostics, { + code: "config-path-absolute", + message: `Path "./assets" cannot be relative. Use {cwd} or {project-root} to specify what the path should be relative to.`, + }); + }); + + it("emit diagnostic if passing relative path if starting with the file/dir name", async () => { + const diagnostics = await diagnoseEmitterOptions({ + "asset-dir": "assets", + }); + expectDiagnostics(diagnostics, { + code: "config-path-absolute", + message: `Path "assets" cannot be relative. Use {cwd} or {project-root} to specify what the path should be relative to.`, + }); + }); + }); +});