Feature: Built-in linter engine (microsoft#2066)

Fix microsoft#1996

Implementation for the linter engine

Author: timotheeguerin

common/changes/@typespec/compiler/feature-linter-engine_2023-06-14-21-59.json

@@ -0,0 +1,10 @@
+{
+  "changes": [
+    {
+      "packageName": "@typespec/compiler",
+      "comment": "**Feature** New built-in linter system. Typespec libraries are able to define linting rules which can be configured in `tspconfig.yaml`. See documentation for configuring a [linter](https://microsoft.github.io/typespec/introduction/configuration#linter---configuring-linters) and [writing a linter](https://microsoft.github.io/typespec/extending-typespec/linters)",
+      "type": "none"
+    }
+  ],
+  "packageName": "@typespec/compiler"
+}

common/changes/@typespec/lint/feature-linter-engine_2023-06-14-21-59.json

@@ -0,0 +1,10 @@
+{
+  "changes": [
+    {
+      "packageName": "@typespec/lint",
+      "comment": "**Deprecation** Package is deprecated in favor of built-in linter system",
+      "type": "none"
+    }
+  ],
+  "packageName": "@typespec/lint"
+}

common/config/rush/pnpm-lock.yaml

@@ -9,33 +9,54 @@ title: Linters
 
 TypeSpec library can probide a `$onValidate` hook which can be used to validate the typespec program is valid in the eye of your library.
 
-A linter on the other hand might be a validation that is more optional, the program is correct but there could be some improvements. For example requiring documentation on every type. This is not something that is needed to represent the typespec program but without it the end user experience might suffer.
+A linter on the other hand might be a validation that is optional, the program is correct but there could be some improvements. For example requiring documentation on every type. This is not something that is needed to represent the typespec program but without it the end user experience might suffer.
+Linters need to be explicitly enabled. `$onValidate` will be run automatically if that library is imported.
 
 ## Writing a linter
 
-There is no built-in concept of linter into TypeSpec, there is however a library `@typespec/lint` that lets a library define its linting rules and hooks on to the `onValidate`.
+See examples in `packages/best-practices`.
 
-### 1. Install the `@typespec/lint` package
-
-```bash
-npm install @typespec/lint
-```
-
-### 2. Define the rules
+### 1. Define a rules
 
 ```ts
-import { createRule } from "@typespec/lint";
+import {  createLinterRule } from "@typespec/compiler";
 import { reportDiagnostic } from "../lib.js";
 
-export const modelDocRule = createRule({
+export const requiredDocRule = createLinterRule({
   name: "no-model-doc",
-  create({ program }) {
+  severity: "warning",
+  // Short description of what this linter rule does. To be used for generated summary of a linter.
+  description: "Enforce documentation on models.",
+  messages: {
+    default: `Must be documented.`,
+    // Different messages can be provided
+    models: `Models must be documented.`,
+
+    // Message can be parameterized
+    enums: paramMessage`Enum ${"enumName"} must be documented.`,
+  },
+  create(context) {
     return {
+      operation: (op) => {
+        if (!getDoc(context.program, op)) {
+          context.reportDiagnostic({
+            target: model,
+          });
+        }
+      },
       model: (model) => {
-        if (!getDoc(program, model)) {
-          reportDiagnostic(program, {
-            // Note 1
-            code: "no-model-doc",
+        if (!getDoc(context.program, model)) {
+          context.reportDiagnostic({
+            messageId: "models",
+            target: model,
+          });
+        }
+      },
+      enums: (type) => {
+        if (!getDoc(context.program, type)) {
+          context.reportDiagnostic({
+            messageId: "enums",
+            format: {enumName: type.name}
             target: model,
           });
         }
@@ -45,98 +66,94 @@ export const modelDocRule = createRule({
 });
 ```
 
-Note 1: `code` refers to they key of a diagnostic defined when you `createTypeSpecLibrary` [See](./basics.md#4-create-libts)
+#### Don'ts
 
-### Register the rules
-
-<!-- cspell:disable-next-line -->
-
-`$lib` refer to the value of `createTypeSpecLibrary` [See](./basics.md#4-create-libts)
+- ❌ Do not call `program.reportDiagnostic` or your library `reportDiagnostic` helper directly in a linter rule
 
 ```ts
-import { $lib } from "../lib.js";
-import { modelDocRule } from "./rules/model-doc.js";
-
-// Get the instance of the linter for your library
-const linter = getLinter($lib);
-
-linter.registerRule(modelDocRule);
-```
-
-Or multiple rules at once
-
-```ts
-linter.registerRules([modelDocRule, interfaceDocRule]);
-```
-
-When registering a rule, its name will be prefixed by the library named defined in `$lib`.
-
-### Enable the rules
-
-Rules are by default just registered but not enabled. This allows a library to provide a set of linting rules for other libraries to use or a user to enable.
+// ❌ Bad
+program.reportDiagnostic({
+  code: "other-code",
+  target,
+});
+// ❌ Bad
+reportDiagnostic(program, {
+  code: "other-code",
+  target,
+});
 
-```ts
-// Note: The rule ID here needs to be the fully qualified rule name prefixed with the
-//       `<libraryname>/` prefix and it cannot be a rule provided by another library.
-linter.enableRule("my-library/no-model-doc");
+// ✅ Good
+context.reportDiagnostic({
+  target,
+});
 ```
 
-Alternatively rules can be automatically enabled when registered.
-
-```ts
-// With single registration
-linter.registerRule(modelDocRule, { autoEnable: true });
-
-// With multi registration
-linter.registerRules([modelDocRule, interfaceDocRule], { autoEnable: true });
-```
+### Register the rules
 
-### Register the linter hook
+<!-- cspell:disable-next-line -->
 
-The lint library still depends on `$onValidate` to run. For that each library providing a linter should call `linter.lintOnValidate(program);` to ensure that the linter will be run.
+When defining your `$lib` with `createTypeSpecLibrary`([See](./basics.md#4-create-libts)) an additional entry for `linter` can be provided
 
 ```ts
-export function $onValidate(program: Program) {
-  // Optional if you want to automatically enable your rules.
-  linter.autoEnableRules();
-
-  // Alternatively enable rules explicitly.
-  // Note: The rule IDs here needs to be the fully qualified rule names with the
-  //       `<libraryname>/` prefix and they cannot be rules provided by other libraries.
-  linter.enableRules(["<library-name>/<rule-name-1>", "<library-name>/<rule-name-2>]);
-
-  linter.lintOnValidate(program);
-}
+// Import the rule defined previously
+import { requiredDocRule } from "./rules/required-doc.rule.js";
+
+export const $lib = createTypeSpecLibrary({
+  name: "@typespec/my-linter",
+  diagnostics: {},
+  linter: {
+    // Include all the rules your linter is defining here.
+    rules: [requiredDocRule],
+
+    // Optionally a linter can provide a set of rulesets
+    ruleSets: {
+      recommended: {
+        // (optional) A ruleset takes a map of rules to explicitly enable
+        enable: { [`@typespec/my-linter:${requiredDocRule.name}`]: true },
+
+        // (optional) A rule set can extend another rule set
+        extends: ["@typespec/best-practices:recommended"],
+
+        // (optional) A rule set can disable a rule enabled in a ruleset it extended.
+        disable: {
+          "`@typespec/best-practices:no-a": "This doesn't apply in this ruleset.",
+        },
+      },
+    },
+  },
+});
 ```
 
-This will not run the linter right here, it will just add a new callback to the onValidate list giving time for all linters to register their rules.
+When referencing a rule or ruleset(in `enable`, `extends`, `disable`) the rule or rule set id must be used which in this format: `<libraryName>:<ruleName>`
 
 ## Testing a linter
 
 To test linter rule an rule tester is provided letting you test a specific rule without enabling the others.
 
-First you'll want to create an instance of the rule tester using `createRuleTester` passing it the rule that is being tested.
+First you'll want to create an instance of the rule tester using `createLinterRuleTester` passing it the rule that is being tested.
 You can then provide different test checking the rule pass or fails.
 
 ```ts
-import { createRuleTester } from "@typespec/lint/testing";
-import { noFooModelRule } from "./no-foo-model.js";
+import { createLinterRuleTester, RuleTester, createTestRunner } from "@typespec/compiler/testing";
+import { requiredDocRule } from "./rules/required-doc.rule.js";
 
-let ruleTester: RuleTester;
+describe("required-doc rule", () => {
+  let ruleTester: RuleTester;
 
-beforeEach(() => {
-  const runner = createTestRunner();
-  ruleTester = createRuleTester(runner, noFooModelRule);
-});
+  beforeEach(() => {
+    const runner = createTestRunner();
+    ruleTester = createLinterRuleTester(runner, requiredDocRule, "@typespec/my-linter");
+  });
 
-it("emit diagnostics when using model named foo", () => {
-  ruleTester.expect(`model Foo {}`).toEmitDiagnostic({
-    code: "my-library/no-foo-model",
-    message: "Cannot name a model with 'Foo'",
+  it("emit diagnostics when using model named foo", async () => {
+    await ruleTester.expect(`model Foo {}`).toEmitDiagnostics({
+      code: "@typespec/my-linter:no-foo-model",
+      message: "Cannot name a model with 'Foo'",
+    });
   });
-});
 
-it("should be valid to use other names", () => {
-  ruleTester.expect(`model Bar {}`).toBeValid();
+  it("should be valid to use other names", async () => {
+    await ruleTester.expect(`model Bar {}`).toBeValid();
+  });
 });
 ```

docs/extending-typespec/linters.md

@@ -36,6 +36,13 @@ model TypeSpecProjectSchema {
   imports?: string;
   emit?: string[];
   options?: Record<unknown>;
+  linter?: LinterConfig;
+}
+
+model LinterConfig {
+  extends?: RuleRef[];
+  enable?: Record<RuleRef, boolean>;
+  disable?: Record<RuleRef, string>;
 }
 ```
 
@@ -182,6 +189,7 @@ options:
 | `imports`       | `--import`                | Additional imports to include                            |
 | `emit`          | `--emit`                  | Emitter configuration                                    |
 | `options`       | `--option` or `--options` | Emitter configuration                                    |
+| `linter`        |                           | Linter configuration                                     |
 
 ### `output-dir` - Configure the default output dir
 
@@ -301,6 +309,22 @@ Default: `{output-dir}/{emitter-name}`
 
 See [output directory configuration for mode details](#output-directory-configuration)
 
+### `linter` - Configuring linters
+
+Configure which linter rules should be enabled in this repository. Referencing to a rule or ruleset must be using their id which is in this format `<libraryName>:<ruleName>`
+
+```yaml
+linter:
+  extends: # Extend `recommended` ruleset from @typespec/best-practices library
+    - "@typespec/best-practices:recommended"
+
+  enable: # Explicitly enable some rules
+    "@typespec/best-practices:no-x": true
+
+  disable: # Disable some rules defined in one of the ruleset extended.
+    "@typespec/best-practices:no-y": "This rule cannot be applied in this project because X"
+```
+
 ## Emitter control cli flags
 
 ### `--no-emit`

docs/introduction/configuration/configuration.md

@@ -58,12 +58,17 @@ Using:
 
 This is a list of the trace area used in the compiler
 
-| Area                        | Description                                                          |
-| --------------------------- | -------------------------------------------------------------------- |
-| `compiler.options`          | Log the resolved compiler options                                    |
-| `import-resolution.library` | Information related to the resolution of import libraries            |
-| `projection.log`            | Debug information logged by the `log()` function used in projections |
-| `bind.js`                   | Information when binding JS files                                    |
+| Area                           | Description                                                          |
+| ------------------------------ | -------------------------------------------------------------------- |
+| `compiler.options`             | Log the resolved compiler options                                    |
+| `import-resolution.library`    | Information related to the resolution of import libraries            |
+| `projection.log`               | Debug information logged by the `log()` function used in projections |
+| `bind.js`                      | Information when binding JS files                                    |
+| `linter.register-library`      | Information that a library rules will be loaded                      |
+| `linter.register-library.rule` | Information about a rule that is being registered                    |
+| `linter.extend-rule-set.start` | Information about a ruleset it is about to extend                    |
+| `linter.extend-rule-set.end`   | Information about rules enabled after extending a ruleset            |
+| `linter.lint`                  | Start the lint process and show information of all the rules enabled |
 
 ## Tracing in TypeSpec library
 

docs/introduction/configuration/tracing.md

@@ -0,0 +1,3 @@
+{
+  "reporter": ["cobertura", "json", "text"]
+}

packages/best-practices/.c8rc.json

@@ -0,0 +1,7 @@
+require("@typespec/eslint-config-typespec/patch/modern-module-resolution");
+
+module.exports = {
+  plugins: ["@typespec/eslint-plugin"],
+  extends: ["@typespec/eslint-config-typespec", "plugin:@typespec/eslint-plugin/recommended"],
+  parserOptions: { tsconfigRootDir: __dirname },
+};

packages/best-practices/.eslintrc.cjs

@@ -0,0 +1,7 @@
+timeout: 5000
+require: source-map-support/register
+spec: "dist/test/**/*.test.js"
+ignore: "dist/test/manual/**/*.js"
+
+# Config for https://www.npmjs.com/package/mocha-multi-reporters
+reporterOptions: "configFile=mocha.reporter.config.json"

packages/best-practices/.mocharc.yaml

@@ -0,0 +1,5 @@
+# TypeSpec Best Practices
+
+**WORK IN PROGRESS This is not ready for consumption**
+
+Set of linter rules to enforce TypeSpec best practices

packages/best-practices/README.md

@@ -0,0 +1,3 @@
+{
+  "reporterEnabled": "spec, mocha-junit-reporter"
+}