Add no-string-discriminator linter rule (#693)

fix [#97](#97) add linter
rule to recommend the discriminator property to be explicitly defined as
an extensible union

.chronus/changes/linter-no-string-discriminator-2024-3-18-15-54-43.md

@@ -0,0 +1,8 @@
+---
+# Change versionKind to one of: internal, fix, dependencies, feature, deprecation, breaking
+changeKind: fix
+packages:
+  - "@azure-tools/typespec-azure-core"
+---
+
+Add new `no-string-discriminator` linter rule suggesting using an explicit extensible union as the discriminator kind.

docs/libraries/azure-core/reference/linter.md

@@ -60,3 +60,4 @@ Available ruleSets:
 | `@azure-tools/typespec-azure-core/spread-discriminated-model`                                                        | Check a model with a discriminator has not been used in composition.                                                                                 |
 | `@azure-tools/typespec-azure-core/use-standard-names`                                                                | Use recommended names for operations.                                                                                                                |
 | `@azure-tools/typespec-azure-core/use-standard-operations`                                                           | Operations should be defined using a signature from the Azure.Core namespace.                                                                        |
+| [`@azure-tools/typespec-azure-core/no-string-discriminator`](/libraries/azure-core/rules/no-string-discriminator.md) | Azure services discriminated models should define the discriminated property as an extensible union.                                                 |

docs/libraries/azure-core/rules/no-string-discriminator.md

@@ -0,0 +1,61 @@
+---
+title: "no-string-discriminator"
+---
+
+```text title="Full name"
+@azure-tools/typespec-azure-core/no-string-discriminator
+```
+
+Azure services favor using an extensible union to define the discriminator property. This allow the service to add new discriminated models without being breaking.
+
+#### ❌ Incorrect
+
+- Missing explicit property
+
+```tsp
+@discriminator("kind")
+model Pet {
+  name: string;
+}
+```
+
+- Property is a string
+
+```tsp
+@discriminator("kind")
+model Pet {
+  kind: string;
+  name: string;
+}
+```
+
+- Property is a closed enum
+
+```tsp
+@discriminator("kind")
+model Pet {
+  kind: PetKind;
+  name: string;
+}
+enum PetKind {
+  cat,
+  dog,
+}
+```
+
+#### ✅ Correct
+
+- Property is a closed enum
+
+```tsp
+@discriminator("kind")
+model Pet {
+  kind: PetKind;
+  name: string;
+}
+union PetKind {
+  cat: "cat",
+  dog: "dog",
+  string,
+}
+```

packages/typespec-azure-core/README.md

@@ -64,6 +64,7 @@ Available ruleSets:
 | `@azure-tools/typespec-azure-core/spread-discriminated-model`                                                                                                | Check a model with a discriminator has not been used in composition.                                                                                 |
 | `@azure-tools/typespec-azure-core/use-standard-names`                                                                                                        | Use recommended names for operations.                                                                                                                |
 | `@azure-tools/typespec-azure-core/use-standard-operations`                                                                                                   | Operations should be defined using a signature from the Azure.Core namespace.                                                                        |
+| [`@azure-tools/typespec-azure-core/no-string-discriminator`](https://azure.github.io/typespec-azure/docs/libraries/azure-core/rules/no-string-discriminator) | Azure services discriminated models should define the discriminated property as an extensible union.                                                 |
 
 ## Decorators
 

packages/typespec-azure-core/src/linter.ts

@@ -17,6 +17,7 @@ import { noOffsetDateTimeRule } from "./rules/no-offsetdatetime.js";
 import { operationIdRule } from "./rules/no-operation-id.js";
 import { noResponseBodyRule } from "./rules/no-response-body.js";
 import { noRpcPathParamsRule } from "./rules/no-rpc-path-params.js";
+import { noStringDiscriminatorRule } from "./rules/no-string-discriminator.js";
 import { nonBreakingVersioningRule } from "./rules/non-breaking-versioning.js";
 import { preferCsvCollectionFormatRule } from "./rules/prefer-csv-collection-format.js";
 import { preventFormatUse } from "./rules/prevent-format.js";
@@ -68,6 +69,7 @@ const rules = [
   spreadDiscriminatedModelRule,
   useStandardNames,
   useStandardOperations,
+  noStringDiscriminatorRule,
 ];
 
 export const $linter = defineLinter({
@@ -108,6 +110,7 @@ export const $linter = defineLinter({
         [`@azure-tools/typespec-azure-core/${friendlyNameRule.name}`]: true,
         [`@azure-tools/typespec-azure-core/${noEnumRule.name}`]: true,
         [`@azure-tools/typespec-azure-core/${noClosedLiteralUnionRule.name}`]: true,
+        [`@azure-tools/typespec-azure-core/${noStringDiscriminatorRule.name}`]: true,
       },
       extends: ["@typespec/http/all"],
     },

packages/typespec-azure-core/src/rules/no-string-discriminator.ts

@@ -0,0 +1,38 @@
+import { Model, createRule, getDiscriminator, paramMessage } from "@typespec/compiler";
+
+export const noStringDiscriminatorRule = createRule({
+  name: "no-string-discriminator",
+  description:
+    "Azure services discriminated models should define the discriminated property as an extensible union.",
+  severity: "warning",
+  url: "https://azure.github.io/typespec-azure/docs/libraries/azure-core/rules/no-string-discriminator",
+  messages: {
+    default: `Use an extensible union instead of a plain string (ex: \`union PetKind { cat: "cat", dog: "dog", string };\`)`,
+    noProp: paramMessage`Discriminated model should define an the discriminator property ${"propName"} with an extensible union as type.`,
+  },
+  create(context) {
+    return {
+      model: (model: Model) => {
+        const discriminator = getDiscriminator(context.program, model);
+        if (discriminator === undefined) {
+          return;
+        }
+
+        const prop = model.properties.get(discriminator.propertyName);
+        if (prop === undefined) {
+          context.reportDiagnostic({
+            format: { propName: discriminator.propertyName },
+            target: model,
+          });
+        } else {
+          if (prop.type.kind !== "Union") {
+            context.reportDiagnostic({
+              format: { propName: discriminator.propertyName },
+              target: prop,
+            });
+          }
+        }
+      },
+    };
+  },
+});

packages/typespec-azure-core/test/rules/no-string-discriminator.test.ts

@@ -0,0 +1,75 @@
+import {
+  BasicTestRunner,
+  LinterRuleTester,
+  createLinterRuleTester,
+} from "@typespec/compiler/testing";
+import { beforeEach, it } from "vitest";
+import { noStringDiscriminatorRule } from "../../src/rules/no-string-discriminator.js";
+import { createAzureCoreTestRunner } from "../test-host.js";
+
+let runner: BasicTestRunner;
+let tester: LinterRuleTester;
+
+beforeEach(async () => {
+  runner = await createAzureCoreTestRunner({ omitServiceNamespace: true });
+  tester = createLinterRuleTester(
+    runner,
+    noStringDiscriminatorRule,
+    "@azure-tools/typespec-azure-core"
+  );
+});
+
+it("emits a warning @discriminator is used without the explicit property", async () => {
+  await tester
+    .expect(
+      `        
+        @discriminator("kind")
+        model Pet {
+        }
+        `
+    )
+    .toEmitDiagnostics([
+      {
+        code: "@azure-tools/typespec-azure-core/no-string-discriminator",
+        message:
+          'Use an extensible union instead of a plain string (ex: `union PetKind { cat: "cat", dog: "dog", string };`)',
+      },
+    ]);
+});
+
+it("emits a warning @discriminator points to a property that is not a union", async () => {
+  await tester
+    .expect(
+      `        
+        @discriminator("kind")
+        model Pet {
+          kind: string;
+        }
+        `
+    )
+    .toEmitDiagnostics([
+      {
+        code: "@azure-tools/typespec-azure-core/no-string-discriminator",
+        message:
+          'Use an extensible union instead of a plain string (ex: `union PetKind { cat: "cat", dog: "dog", string };`)',
+      },
+    ]);
+});
+
+it("doesn't warn when using an extensible union as the type", async () => {
+  await tester
+    .expect(
+      `       
+        @discriminator("kind")
+        model Pet { kind: PetKind; }
+
+        model Cat extends Pet { kind: "cat" }
+
+        union PetKind {
+          dog: "dog",
+          cat: "cat",
+        }
+        `
+    )
+    .toBeValid();
+});