From aa189c65b51bd95da5ed84d78c5a11b5ef6d660a Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Fri, 4 Oct 2024 20:37:12 -0700 Subject: [PATCH] Update decorator arg marshalling to new default (#4500) fix [#4138](https://github.com/microsoft/typespec/issues/4138) --- ...shalling-new-default-2024-8-23-14-22-52.md | 15 +++++ ...rshalling-new-default-2024-8-23-21-44-3.md | 8 +++ docs/extending-typespec/basics.md | 18 +---- packages/compiler/src/core/binder.ts | 12 +++- packages/compiler/src/core/checker.ts | 65 +------------------ packages/compiler/src/core/js-marshaller.ts | 19 +++++- packages/compiler/src/core/types.ts | 6 +- .../compiler/test/checker/decorators.test.ts | 22 ++----- .../compiler/test/checker/relation.test.ts | 4 +- .../compiler/test/checker/values/utils.ts | 2 +- packages/json-schema/src/lib.ts | 4 +- packages/json-schema/test/extension.test.ts | 2 +- packages/samples/package.json | 3 +- .../decorators-signatures.test.ts | 4 +- pnpm-lock.yaml | 7 +- 15 files changed, 77 insertions(+), 114 deletions(-) create mode 100644 .chronus/changes/decorator-arg-marshalling-new-default-2024-8-23-14-22-52.md create mode 100644 .chronus/changes/decorator-arg-marshalling-new-default-2024-8-23-21-44-3.md diff --git a/.chronus/changes/decorator-arg-marshalling-new-default-2024-8-23-14-22-52.md b/.chronus/changes/decorator-arg-marshalling-new-default-2024-8-23-14-22-52.md new file mode 100644 index 0000000000..92a38e36cb --- /dev/null +++ b/.chronus/changes/decorator-arg-marshalling-new-default-2024-8-23-14-22-52.md @@ -0,0 +1,15 @@ +--- +changeKind: breaking +packages: + - "@typespec/compiler" +--- + +API: Update default of `decoratorArgMarshalling` from `legacy` to `new` + +To revert to the old behavior export the following. **Highly discouraged, this will be removed in a few versions.** + +```ts +export const $flags = definePackageFlags({ + decoratorArgMarshalling: "legacy", +}); +``` diff --git a/.chronus/changes/decorator-arg-marshalling-new-default-2024-8-23-21-44-3.md b/.chronus/changes/decorator-arg-marshalling-new-default-2024-8-23-21-44-3.md new file mode 100644 index 0000000000..4b94b02c3f --- /dev/null +++ b/.chronus/changes/decorator-arg-marshalling-new-default-2024-8-23-21-44-3.md @@ -0,0 +1,8 @@ +--- +# Change versionKind to one of: internal, fix, dependencies, feature, deprecation, breaking +changeKind: internal +packages: + - "@typespec/json-schema" +--- + +Update decorator arg marshalling to new default diff --git a/docs/extending-typespec/basics.md b/docs/extending-typespec/basics.md index b6eb09ea71..4deefa490b 100644 --- a/docs/extending-typespec/basics.md +++ b/docs/extending-typespec/basics.md @@ -134,19 +134,7 @@ export const { reportDiagnostic, createDiagnostic } = $lib; Diagnostics are used for linters and decorators, which are covered in subsequent topics. -### f. Set package flags - -You can optionally set any package flags by exporting a `$flags` const that is initialized with the `definePackageFlags`. Like `$lib`, this value must be exported from your package. - -It is strongly recommended to set `valueMarshalling` to `"new"` as this will be the default behavior in future TypeSpec versions. - -```typescript -export const $flags = definePackageFlags({ - valueMarshalling: "new", -}); -``` - -### g. Create `index.ts` +### f. Create `index.ts` Open `./src/index.ts` and import your library definition: @@ -155,7 +143,7 @@ Open `./src/index.ts` and import your library definition: export { $lib } from "./lib.js"; ``` -### h. Build TypeScript +### g. Build TypeScript TypeSpec can only import JavaScript files, so any changes made to TypeScript sources need to be compiled before they are visible to TypeSpec. To do this, run `npx tsc -p .` in your library's root directory. If you want to re-run the TypeScript compiler whenever files are changed, you can run `npx tsc -p . --watch`. @@ -173,7 +161,7 @@ Alternatively, you can add these as scripts in your `package.json` to make them You can then run `npm run build` or `npm run watch` to build or watch your library. -### i. Add your main TypeSpec file +### h. Add your main TypeSpec file Open `./lib/main.tsp` and import your JS entrypoint. This ensures that when TypeSpec imports your library, the code to define the library is run. When we add decorators in later topics, this import will ensure those get exposed as well. diff --git a/packages/compiler/src/core/binder.ts b/packages/compiler/src/core/binder.ts index 06c72b8c32..c79c7f049c 100644 --- a/packages/compiler/src/core/binder.ts +++ b/packages/compiler/src/core/binder.ts @@ -1,5 +1,5 @@ import { mutate } from "../utils/misc.js"; -import { compilerAssert } from "./diagnostics.js"; +import { compilerAssert, reportDeprecated } from "./diagnostics.js"; import { getLocationContext } from "./helpers/location-context.js"; import { visitChildren } from "./parser.js"; import type { Program } from "./program.js"; @@ -137,6 +137,16 @@ export function createBinder(program: Program): Binder { const context = getLocationContext(program, sourceFile); if (context.type === "library" || context.type === "project") { mutate(context).flags = member as any; + if ((member as any).decoratorArgMarshalling === "legacy") { + reportDeprecated( + program, + [ + `decoratorArgMarshalling: "legacy" flag is deprecated and will be removed in a future release.`, + 'Change value to "new" or remove the flag to use the current default behavior.', + ].join("\n"), + sourceFile, + ); + } } } else if (key === "$decorators") { const value: DecoratorImplementations = member as any; diff --git a/packages/compiler/src/core/checker.ts b/packages/compiler/src/core/checker.ts index 07112b88b9..746bc76cbb 100644 --- a/packages/compiler/src/core/checker.ts +++ b/packages/compiler/src/core/checker.ts @@ -5,12 +5,7 @@ import { createChangeIdentifierCodeFix } from "./compiler-code-fixes/change-iden import { createModelToObjectValueCodeFix } from "./compiler-code-fixes/model-to-object-literal.codefix.js"; import { createTupleToArrayValueCodeFix } from "./compiler-code-fixes/tuple-to-array-value.codefix.js"; import { getDeprecationDetails, markDeprecated } from "./deprecation.js"; -import { - ProjectionError, - compilerAssert, - ignoreDiagnostics, - reportDeprecated, -} from "./diagnostics.js"; +import { ProjectionError, compilerAssert, ignoreDiagnostics } from "./diagnostics.js"; import { validateInheritanceDiscriminatedUnions } from "./helpers/discriminator-utils.js"; import { getLocationContext } from "./helpers/location-context.js"; import { explainStringTemplateNotSerializable } from "./helpers/string-template-utils.js"; @@ -20,11 +15,7 @@ import { getTypeName, type TypeNameOptions, } from "./helpers/type-name-utils.js"; -import { - canNumericConstraintBeJsNumber, - legacyMarshallTypeForJS, - marshallTypeForJS, -} from "./js-marshaller.js"; +import { legacyMarshallTypeForJS, marshallTypeForJS } from "./js-marshaller.js"; import { createDiagnostic } from "./messages.js"; import { Numeric } from "./numeric.js"; import { @@ -2054,59 +2045,9 @@ export function createChecker(program: Program): Checker { linkType(links, decoratorType, mapper); - checkDecoratorLegacyMarshalling(decoratorType); return decoratorType; } - function checkDecoratorLegacyMarshalling(decorator: Decorator) { - const marshalling = resolveDecoratorArgMarshalling(decorator); - function reportDeprecatedLegacyMarshalling(param: MixedFunctionParameter, message: string) { - reportDeprecated( - program, - [ - `Parameter ${param.name} of decorator ${decorator.name} is using legacy marshalling but is accepting ${message}.`, - `This will change in the future.`, - 'Add `export const $flags = {decoratorArgMarshalling: "new"}}` to your library to opt-in to the new marshalling behavior.', - ].join("\n"), - param.node, - ); - } - if (marshalling === "legacy") { - for (const param of decorator.parameters) { - if (param.type.valueType) { - if ( - ignoreDiagnostics( - relation.isTypeAssignableTo(nullType, param.type.valueType, param.type), - ) - ) { - reportDeprecatedLegacyMarshalling(param, "null as a type"); - } else if ( - param.type.valueType.kind === "Enum" || - param.type.valueType.kind === "EnumMember" || - (relation.isReflectionType(param.type.valueType) && - param.type.valueType.name === "EnumMember") - ) { - reportDeprecatedLegacyMarshalling(param, "enum members"); - } else if ( - ignoreDiagnostics( - relation.isTypeAssignableTo( - param.type.valueType, - getStdType("numeric"), - param.type.valueType, - ), - ) && - !canNumericConstraintBeJsNumber(param.type.valueType) - ) { - reportDeprecatedLegacyMarshalling( - param, - "a numeric type that is not representable as a JS Number", - ); - } - } - } - } - } - function checkFunctionDeclaration( node: FunctionDeclarationStatementNode, mapper: TypeMapper | undefined, @@ -5440,7 +5381,7 @@ export function createChecker(program: Program): Checker { ) { return location.flags.decoratorArgMarshalling; } else { - return "legacy"; + return "new"; } } return "new"; diff --git a/packages/compiler/src/core/js-marshaller.ts b/packages/compiler/src/core/js-marshaller.ts index bafc2b74f7..0b35fc1dbf 100644 --- a/packages/compiler/src/core/js-marshaller.ts +++ b/packages/compiler/src/core/js-marshaller.ts @@ -7,6 +7,7 @@ import type { MarshalledValue, NumericValue, ObjectValue, + Scalar, Type, Value, } from "./types.js"; @@ -64,11 +65,27 @@ export function marshallTypeForJS( } } +function isNumericScalar(scalar: Scalar) { + let current: Scalar | undefined = scalar; + + while (current) { + if (current.name === "numeric" && current.namespace?.name === "TypeSpec") { + return true; + } + current = current.baseScalar; + } + return false; +} + export function canNumericConstraintBeJsNumber(type: Type | undefined): boolean { if (type === undefined) return true; switch (type.kind) { case "Scalar": - return numericRanges[type.name as keyof typeof numericRanges]?.[2].isJsNumber; + if (isNumericScalar(type)) { + return numericRanges[type.name as keyof typeof numericRanges]?.[2].isJsNumber; + } else { + return true; + } case "Union": return [...type.variants.values()].every((x) => canNumericConstraintBeJsNumber(x.type)); default: diff --git a/packages/compiler/src/core/types.ts b/packages/compiler/src/core/types.ts index 23ca315dc6..7d1c0b96dc 100644 --- a/packages/compiler/src/core/types.ts +++ b/packages/compiler/src/core/types.ts @@ -2501,18 +2501,18 @@ export interface DecoratorImplementations { export interface PackageFlags { /** * Decorator arg marshalling algorithm. Specify how TypeSpec values are marshalled to decorator arguments. - * - `lossless` - New recommended behavior + * - `new` - New recommended behavior * - string value -> `string` * - numeric value -> `number` if the constraint can be represented as a JS number, Numeric otherwise(e.g. for types int64, decimal128, numeric, etc.) * - boolean value -> `boolean` * - null value -> `null` * - * - `legacy` Behavior before version 0.56.0. + * - `legacy` - DEPRECATED - Behavior before version 0.56.0. * - string value -> `string` * - numeric value -> `number` * - boolean value -> `boolean` * - null value -> `NullType` - * @default legacy + * @default new */ readonly decoratorArgMarshalling?: "legacy" | "new"; } diff --git a/packages/compiler/test/checker/decorators.test.ts b/packages/compiler/test/checker/decorators.test.ts index 5775528dd4..41e05c83f1 100644 --- a/packages/compiler/test/checker/decorators.test.ts +++ b/packages/compiler/test/checker/decorators.test.ts @@ -14,6 +14,7 @@ import { TestHost, createTestHost, createTestWrapper, + expectDiagnosticEmpty, expectDiagnostics, } from "../../src/testing/index.js"; import { mutate } from "../../src/utils/misc.js"; @@ -140,20 +141,6 @@ describe("compiler: checker: decorators", () => { message: "Extern declaration must have an implementation in JS file.", }); }); - - describe("emit deprecated warning if decorator is expecting valueof", () => { - it.each(["numeric", "int64", "uint64", "integer", "float", "decimal", "decimal128", "null"])( - "%s", - async (type) => { - const diagnostics = await runner.diagnose(` - extern dec testDec(target: unknown, value: valueof ${type}); - `); - expectDiagnostics(diagnostics, { - code: "deprecated", - }); - }, - ); - }); }); describe("usage", () => { @@ -366,7 +353,6 @@ describe("compiler: checker: decorators", () => { value: string, suppress?: boolean, ): Promise { - mutate($flags).decoratorArgMarshalling = "new"; await runner.compile(` extern dec testDec(target: unknown, arg1: ${type}); @@ -532,9 +518,8 @@ describe("compiler: checker: decorators", () => { suppress?: boolean, ): Promise { // Default so shouldn't be needed - // mutate($flags).decoratorArgMarshalling = "legacy"; - await runner.compile(` - #suppress "deprecated" "for testing" + mutate($flags).decoratorArgMarshalling = "legacy"; + const diagnostics = await runner.diagnose(` extern dec testDec(target: unknown, arg1: ${type}); ${suppress ? `#suppress "deprecated" "for testing"` : ""} @@ -542,6 +527,7 @@ describe("compiler: checker: decorators", () => { @test model Foo {} `); + expectDiagnosticEmpty(diagnostics.filter((x) => x.code !== "deprecated")); return calledArgs![2]; } diff --git a/packages/compiler/test/checker/relation.test.ts b/packages/compiler/test/checker/relation.test.ts index 286180b5c2..7f1ba4a3aa 100644 --- a/packages/compiler/test/checker/relation.test.ts +++ b/packages/compiler/test/checker/relation.test.ts @@ -39,9 +39,7 @@ describe("compiler: checker: type relations", () => { expectedDiagnosticPos: number; }> { host.addJsFile("mock.js", { - $flags: definePackageFlags({ - decoratorArgMarshalling: "new", - }), + $flags: definePackageFlags({}), $mock: () => null, }); const { source: code, pos } = extractCursor(` diff --git a/packages/compiler/test/checker/values/utils.ts b/packages/compiler/test/checker/values/utils.ts index 80a1ec2eba..151e3d1d72 100644 --- a/packages/compiler/test/checker/values/utils.ts +++ b/packages/compiler/test/checker/values/utils.ts @@ -73,7 +73,7 @@ export async function compileAndDiagnoseValueOrType( const host = await createTestHost(); host.addJsFile("collect.js", { $collect: () => {}, - $flags: definePackageFlags({ decoratorArgMarshalling: "new" }), + $flags: definePackageFlags({}), }); host.addTypeSpecFile( "main.tsp", diff --git a/packages/json-schema/src/lib.ts b/packages/json-schema/src/lib.ts index 5239c5e4fe..b810a538b8 100644 --- a/packages/json-schema/src/lib.ts +++ b/packages/json-schema/src/lib.ts @@ -112,9 +112,7 @@ export const $lib = createTypeSpecLibrary({ }, } as const); -export const $flags = definePackageFlags({ - decoratorArgMarshalling: "new", -}); +export const $flags = definePackageFlags({}); export const { reportDiagnostic, createStateSymbol } = $lib; diff --git a/packages/json-schema/test/extension.test.ts b/packages/json-schema/test/extension.test.ts index c8e6650f04..c650873784 100644 --- a/packages/json-schema/test/extension.test.ts +++ b/packages/json-schema/test/extension.test.ts @@ -197,7 +197,7 @@ describe("setExtension", () => { emitNamespace: true, decorators: { namespace: "test", - $flags: { decoratorArgMarshalling: "new" }, + $flags: {}, $setExtension(context: DecoratorContext, target: Type, key: string, value: unknown) { setExtension(context.program, target, key, value); }, diff --git a/packages/samples/package.json b/packages/samples/package.json index a64c79874f..d84d7d4314 100644 --- a/packages/samples/package.json +++ b/packages/samples/package.json @@ -52,7 +52,8 @@ "@typespec/openapi": "workspace:~", "@typespec/openapi3": "workspace:~", "@typespec/rest": "workspace:~", - "@typespec/versioning": "workspace:~" + "@typespec/versioning": "workspace:~", + "@typespec/protobuf": "workspace:~" }, "devDependencies": { "@types/node": "~22.7.1", diff --git a/packages/tspd/test/gen-extern-signature/decorators-signatures.test.ts b/packages/tspd/test/gen-extern-signature/decorators-signatures.test.ts index 4584201ef2..c87435bb24 100644 --- a/packages/tspd/test/gen-extern-signature/decorators-signatures.test.ts +++ b/packages/tspd/test/gen-extern-signature/decorators-signatures.test.ts @@ -13,9 +13,7 @@ async function generateDecoratorSignatures(code: string) { ${code}`, ); host.addJsFile("lib.js", { - $flags: definePackageFlags({ - decoratorArgMarshalling: "new", - }), + $flags: definePackageFlags({}), }); await host.diagnose("main.tsp", { parseOptions: { comments: true, docs: true }, diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index ce0568e200..dbeb9e5672 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -330,7 +330,7 @@ importers: version: link:../tmlanguage-generator ts-node: specifier: ~10.9.2 - version: 10.9.2(@swc/core@1.7.28(@swc/helpers@0.5.8))(@types/node@22.7.3)(typescript@5.6.2) + version: 10.9.2(@swc/core@1.7.28)(@types/node@22.7.3)(typescript@5.6.2) typescript: specifier: ~5.6.2 version: 5.6.2 @@ -1301,6 +1301,9 @@ importers: '@typespec/openapi3': specifier: workspace:~ version: link:../openapi3 + '@typespec/protobuf': + specifier: workspace:~ + version: link:../protobuf '@typespec/rest': specifier: workspace:~ version: link:../rest @@ -25514,7 +25517,7 @@ snapshots: ts-dedent@2.2.0: {} - ts-node@10.9.2(@swc/core@1.7.28(@swc/helpers@0.5.8))(@types/node@22.7.3)(typescript@5.6.2): + ts-node@10.9.2(@swc/core@1.7.28)(@types/node@22.7.3)(typescript@5.6.2): dependencies: '@cspotcode/source-map-support': 0.8.1 '@tsconfig/node10': 1.0.11