Update decorator arg marshalling to new default (#4500)

fix [#4138](#4138)

.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",
+});
+```

.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

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.
 

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;

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";

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<T extends Value>(
   }
 }
 
+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:

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";
 }

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<any> {
-        mutate($flags).decoratorArgMarshalling = "new";
         await runner.compile(`
           extern dec testDec(target: unknown, arg1: ${type});
           
@@ -532,16 +518,16 @@ describe("compiler: checker: decorators", () => {
         suppress?: boolean,
       ): Promise<any> {
         // 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"` : ""}
           @testDec(${value})
           @test
           model Foo {}
         `);
+        expectDiagnosticEmpty(diagnostics.filter((x) => x.code !== "deprecated"));
         return calledArgs![2];
       }
 

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(`

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",

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;
 

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);
           },

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",

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 },