diff --git a/.github/skills/create-linter-rule/SKILL.md b/.github/skills/create-linter-rule/SKILL.md new file mode 100644 index 0000000000..35d2ce22b2 --- /dev/null +++ b/.github/skills/create-linter-rule/SKILL.md @@ -0,0 +1,215 @@ +--- +name: create-linter-rule +description: > + Create a new TypeSpec linter rule, lint diagnostic, or design guideline checker with a + TDD approach, including implementation, tests, documentation, ruleset registration, + and changeset. Use this skill when asked to create, add, implement, or write a new + linter rule, lint warning, validation rule, or design guideline enforcement for + TypeSpec Azure libraries. +allowed-tools: shell +--- + +# Create a TypeSpec Linter Rule + +Follow these steps in order to create a complete, high-quality linter rule. + +## Step 1: IDENTIFY + +Determine the rule metadata before writing any code: + +- **Package**: `typespec-azure-core` (data-plane rules), `typespec-azure-resource-manager` (ARM rules), or `typespec-client-generator-core` (client SDK generation rules) +- **Rule name**: Must follow these naming conventions: + - Use kebab-case (lowercase letters, numbers, hyphens) + - DO NOT include the package name in the rule ID (the package is already part of the fully-qualified diagnostic code) + - Use `no-` when the rule bans a construct or usage (e.g., `no-nullable`, `no-enum`, `no-format`) + - Use `use-` when the rule points users to a standard/preferred TypeSpec pattern (e.g., `use-standard-operations`, `use-extensible-enum`) + - Keep names short and concise — prefer `no-enum` over `no-enum-type-usage` +- **Severity**: All linter rules are warnings; no severity choice is needed +- **Target ruleset(s)**: `data-plane`, `resource-manager`, or both +- **Description**: One-line explanation of what the rule enforces + +Choose the package by scope: + +- Decorators from `typespec-client-generator-core`, or rules only about client SDK generation → `typespec-client-generator-core` +- Rules specific to ARM APIs → `typespec-azure-resource-manager` +- Rules that apply to both data-plane and ARM, or only data-plane → `typespec-azure-core` + +## Step 2: SCAFFOLD + +Generate all required files using the repo's scaffolding tool: + +```bash +pnpm create:linter-rule < rule-name > --package < azure-core | azure-resource-manager | client-generator-core > --description "" +``` + +This creates: + +- `packages//src/rules/.ts` — rule skeleton +- `packages//test/rules/.test.ts` — test skeleton +- `website/src/content/docs/docs/libraries//rules/.md` — docs skeleton +- Updates `packages//src/linter.ts` — registers the rule + +## Step 3: WRITE FAILING TESTS FIRST (TDD) + +Edit `packages//test/rules/.test.ts`: + +1. Write tests for **valid code** that should produce no diagnostics (`.toBeValid()`) +2. Write tests for **invalid code** that should produce specific diagnostics (`.toEmitDiagnostics()`) +3. Create **equivalence classes** for the input and write tests covering at least one instance of each class: + - Group inputs by how the rule handles them (e.g., for a rule targeting `ModelProperty`): + - Simply defined properties + - Properties defined using `spread` or `is` + - Properties inherited from a base class + - Add boundary conditions specific to the rule logic (e.g., for a name-prefix rule): + - Properties with the forbidden prefix + - Properties with the prefix text in the middle or end of the name + - Properties with names shorter than the prefix +4. Always include at least one test verifying that **library types in `Azure.Core` and `Azure.ResourceManager` are not subject to the rule** + +Test API reference: + +```typescript +// No diagnostics expected +await tester.expect(`model Foo {}`).toBeValid(); + +// Specific diagnostic expected +await tester.expect(`model foo {}`).toEmitDiagnostics([ + { + code: "@azure-tools/typespec-/", + severity: "warning", + message: "Expected message text", + }, +]); + +// Test code fix (if rule provides one) +await tester + .expect(`enum Color { red }`) + .applyCodeFix("fix-id") + .toEqual(`union Color { string, red: "red" }`); +``` + +Verify tests fail before implementing: + +```bash +pnpm --filter "@azure-tools/typespec-..." build +pnpm --filter "@azure-tools/typespec-..." test +``` + +## Step 4: IMPLEMENT THE RULE + +Edit `packages//src/rules/.ts`: + +- Implement visitor logic in the `create(context)` function +- Return a `SemanticNodeListener` with the appropriate hooks: + - `model` — for model-level checks + - `modelProperty` — for property-level checks + - `operation` — for operation-level checks + - `enum` — for enum checks + - `namespace` — for namespace-level checks + - `interface` — for interface checks + - `union` / `unionVariant` — for union checks +- Use `context.reportDiagnostic({ target })` to report violations +- Use `paramMessage` from `@typespec/compiler` for interpolated messages +- Add `codefixes` array to `reportDiagnostic()` if a fix is possible + +## Step 5: VERIFY TESTS PASS + +```bash +pnpm --filter "@azure-tools/typespec-..." build +pnpm --filter "@azure-tools/typespec-..." test +``` + +All tests should pass. If not, fix the implementation until they do. + +## Step 6: REGISTER IN RULESETS + +Add the rule to the appropriate ruleset(s): + +- **Data-plane rules**: Edit `packages/typespec-azure-rulesets/src/rulesets/data-plane.ts` +- **ARM rules**: Edit `packages/typespec-azure-rulesets/src/rulesets/resource-manager.ts` + +TCGC rules generally go in **both** rulesets. Rules in `typespec-azure-core` that apply to both ARM and data-plane specs also go in **both** rulesets. Only rules that are truly ARM-specific go exclusively in `resource-manager.ts`; otherwise, add the rule to both `resource-manager.ts` and `data-plane.ts` as appropriate. + +Add an entry: `"@azure-tools/typespec-/": true,` + +Every rule MUST be explicitly listed (enabled or disabled). The `validate-rules-defined.test.ts` test will fail otherwise. + +Verify: + +```bash +pnpm --filter "@azure-tools/typespec-azure-rulesets..." build +pnpm --filter "@azure-tools/typespec-azure-rulesets..." test +``` + +## Step 7: WRITE DOCUMENTATION AND REGENERATE DOCS + +Edit `website/src/content/docs/docs/libraries//rules/.md`: + +- Replace all placeholder text +- Write a clear description of what the rule checks and why +- Provide realistic ❌ Incorrect and ✅ Correct examples using actual TypeSpec patterns +- Ensure the `Full name` code block shows the correct fully-qualified rule name + +Then regenerate the library's reference docs (updates the rule listing): + +```bash +pnpm --filter "@azure-tools/typespec-..." build +pnpm --filter "@azure-tools/typespec-" regen-docs +``` + +## Step 8: CREATE CHANGESET + +```bash +pnpm change add +``` + +When prompted: + +- Select change kind: `feature` (new rule) or `fix` (bugfix to existing rule) +- Select affected package: `@azure-tools/typespec-` +- Write a concise description: "Add `` linter rule that " + +## Step 9: FINAL VALIDATION + +Run the general pre-PR validation to ensure everything is ready: + +```bash +pnpm validate:pr +``` + +This checks: branch is up to date, build passes, tests pass, lint passes, format is clean, spelling is clean, regen-docs is clean, changeset exists, and diff only contains expected files. + +If any check fails, fix the issue and re-run. Use `pnpm validate:pr --fix` to auto-fix formatting and lint issues. + +## Step 10: EXTERNAL INTEGRATION CHECK + +New linter rules MUST NOT break existing Azure service specs. After pushing your PR: + +1. Apply the `int:azure-specs` label to the PR to trigger the External Integration check: `gh pr edit --add-label "int:azure-specs"` +2. If the agent cannot apply the label automatically, tell the user to apply the `int:azure-specs` label manually in the GitHub UI +3. After applying the label, the External Integration workflow will start. Monitor it via `gh run list --workflow=external-integration.yml` +4. This workflow packages your changes and runs TypeSpec validation against all specs in `Azure/azure-rest-api-specs` +5. Wait for the check to pass before requesting review + +If the check fails, your rule produces diagnostics on existing specs. To resolve: + +1. **Apply an API-neutral fix to the spec** (preferred): If the fix doesn't change API behavior, submit a PR to `Azure/azure-rest-api-specs` on the **main** branch. +2. **Suppress the rule**: If the spec cannot be fixed without changing API behavior, add a suppress comment. Suppressions always go to the **main** branch. +3. **Fix on typespec-next**: If the fix requires unreleased TypeSpec APIs or behavior, submit to the **typespec-next** branch (uses nightly builds). +4. **Link your spec fix PR**: Always link the spec fix PR in your linter rule PR description. + +The External Integration workflow: + +- Builds and packs all typespec-azure packages from your PR +- Checks out `Azure/azure-rest-api-specs` (main branch) +- Patches in your packaged changes +- Runs `tsp-integration azure-specs --stage validate` +- Checks for unexpected git changes + +## Important Notes + +- **Import extensions**: Always use `.js` extensions in imports (e.g., `from "./rules/my-rule.js"`) +- **Rule URL**: Must match `https://azure.github.io/typespec-azure/docs/libraries//rules/` +- **Test coverage**: Aim for ≥ 95% branch coverage of the rule file +- **Existing patterns**: Look at similar existing rules in `packages//src/rules/` for reference +- **ARM rules can import from azure-core**: `import { ... } from "@azure-tools/typespec-azure-core"` diff --git a/cspell.yaml b/cspell.yaml index 8df61de653..0a289b1c74 100644 --- a/cspell.yaml +++ b/cspell.yaml @@ -51,6 +51,7 @@ words: - healthdataaiservices - horiz - Howtos + - incentivizes - infile - kvset - kwargs diff --git a/eng/scripts/create-linter-rule.ts b/eng/scripts/create-linter-rule.ts new file mode 100644 index 0000000000..2ce5f9280b --- /dev/null +++ b/eng/scripts/create-linter-rule.ts @@ -0,0 +1,343 @@ +import { existsSync, mkdirSync, readFileSync, writeFileSync } from "fs"; +import { dirname, resolve } from "path"; +import process from "process"; + +type PackageOption = "azure-core" | "azure-resource-manager" | "client-generator-core"; + +interface Options { + ruleName: string; + packageName: PackageOption; + description: string; + dryRun: boolean; +} + +interface PackageConfig { + packageDir: string; + docsLibraryDir: string; + packageNpmName: string; + testHostComment?: string; + testHostImport: string; + testHostFactory: string; +} + +const usage = + "Usage: pnpm create:linter-rule [--package ] [--description ] [--dry-run]"; + +const packageConfigs: Record = { + "azure-core": { + packageDir: "typespec-azure-core", + docsLibraryDir: "azure-core", + packageNpmName: "@azure-tools/typespec-azure-core", + testHostImport: 'import { TesterWithService } from "#test/test-host.js";', + testHostFactory: "TesterWithService.createInstance()", + }, + "azure-resource-manager": { + packageDir: "typespec-azure-resource-manager", + docsLibraryDir: "azure-resource-manager", + packageNpmName: "@azure-tools/typespec-azure-resource-manager", + testHostImport: 'import { Tester } from "#test/tester.js";', + testHostFactory: "Tester.createInstance()", + }, + "client-generator-core": { + packageDir: "typespec-client-generator-core", + docsLibraryDir: "typespec-client-generator-core", + packageNpmName: "@azure-tools/typespec-client-generator-core", + testHostComment: + "// TODO: Verify this helper matches the current client-generator-core linter test host pattern.", + testHostImport: + 'import { createSdkTestRunner } from "@azure-tools/typespec-client-generator-core/testing";', + testHostFactory: "createSdkTestRunner()", + }, +}; + +const scriptDir = import.meta.dirname; +const repoRoot = resolve(scriptDir, "..", ".."); + +function fail(message: string): never { + console.error(`Error: ${message}`); + console.error(usage); + process.exit(1); +} + +function isKebabCase(value: string): boolean { + return /^[a-z0-9]+(?:-[a-z0-9]+)*$/.test(value); +} + +function toCamelCase(value: string): string { + return value.replace(/-([a-z0-9])/g, (_, letter: string) => letter.toUpperCase()); +} + +function toDoubleQuotedString(value: string): string { + return JSON.stringify(value); +} + +function normalizeNewlines(content: string): string { + return content.replace(/\r\n/g, "\n"); +} + +function readText(path: string): string { + return normalizeNewlines(readFileSync(path, "utf8")); +} + +function writeText(path: string, content: string): void { + mkdirSync(dirname(path), { recursive: true }); + writeFileSync(path, content, "utf8"); +} + +function parseArgs(argv: string[]): Options { + const positionals: string[] = []; + let packageName: PackageOption = "azure-core"; + let description = "TODO: Add rule description."; + let dryRun = false; + + for (let index = 0; index < argv.length; index++) { + const arg = argv[index]; + + if (!arg.startsWith("--")) { + positionals.push(arg); + continue; + } + + switch (arg) { + case "--package": { + const value = argv[++index]; + if ( + value !== "azure-core" && + value !== "azure-resource-manager" && + value !== "client-generator-core" + ) { + fail( + "--package must be one of azure-core, azure-resource-manager, or client-generator-core.", + ); + } + packageName = value; + break; + } + case "--description": { + const value = argv[++index]; + if (!value) { + fail("--description requires a value."); + } + description = value; + break; + } + case "--dry-run": + dryRun = true; + break; + default: + fail(`Unknown argument: ${arg}`); + } + } + + if (positionals.length !== 1) { + fail("You must provide exactly one rule name."); + } + + const [ruleName] = positionals; + if (!isKebabCase(ruleName)) { + fail("rule-name must be kebab-case using lowercase letters, numbers, and hyphens only."); + } + + return { ruleName, packageName, description, dryRun }; +} + +function updateLinterFile(linterPath: string, importLine: string, ruleIdentifier: string): string { + const current = readText(linterPath); + + if (current.includes(importLine) || current.includes(` ${ruleIdentifier},`)) { + fail(`Linter is already wired for rule '${ruleIdentifier}'.`); + } + + const importBlockMatch = current.match(/^(import .*;\n)+/m); + if (!importBlockMatch || importBlockMatch.index !== 0) { + fail(`Could not find import block in ${linterPath}.`); + } + + const importBlock = importBlockMatch[0]; + const importLines = importBlock.trimEnd().split("\n"); + importLines.push(importLine); + importLines.sort((left, right) => left.localeCompare(right)); + const updatedImports = `${importLines.join("\n")}\n\n`; + const afterImports = current.slice(importBlock.length); + const withUpdatedImports = `${updatedImports}${afterImports}`; + + const rulesArrayMatch = withUpdatedImports.match(/const rules = \[(?[\s\S]*?)\];/); + if ( + !rulesArrayMatch || + rulesArrayMatch.index === undefined || + rulesArrayMatch.groups === undefined + ) { + fail(`Could not find rules array in ${linterPath}.`); + } + + const start = rulesArrayMatch.index; + const end = start + rulesArrayMatch[0].length; + const body = rulesArrayMatch.groups.body; + const ruleEntries = body + .split(",") + .map((entry) => entry.trim()) + .filter((entry) => entry.length > 0); + ruleEntries.push(ruleIdentifier); + const updatedRules = `const rules = [\n${ruleEntries.map((entry) => ` ${entry},`).join("\n")}\n];`; + + return `${withUpdatedImports.slice(0, start)}${updatedRules}${withUpdatedImports.slice(end)}`; +} + +function main(): void { + const options = parseArgs(process.argv.slice(2)); + const config = packageConfigs[options.packageName]; + const camelCaseName = toCamelCase(options.ruleName); + const ruleIdentifier = `${camelCaseName}Rule`; + + const packageRoot = resolve(repoRoot, "packages", config.packageDir); + const rulePath = resolve(packageRoot, "src", "rules", `${options.ruleName}.ts`); + const testPath = resolve(packageRoot, "test", "rules", `${options.ruleName}.test.ts`); + const docsPath = resolve( + repoRoot, + "website", + "src", + "content", + "docs", + "docs", + "libraries", + config.docsLibraryDir, + "rules", + `${options.ruleName}.md`, + ); + const linterPath = resolve(packageRoot, "src", "linter.ts"); + + const fileTargets = [rulePath, testPath, docsPath]; + const existingFiles = fileTargets.filter((path) => existsSync(path)); + if (existingFiles.length > 0) { + fail( + `The following file(s) already exist:\n${existingFiles.map((path) => `- ${path}`).join("\n")}`, + ); + } + + if (!existsSync(linterPath)) { + fail(`Could not find linter file at ${linterPath}.`); + } + + const ruleFile = [ + 'import { createRule } from "@typespec/compiler";', + "", + `export const ${ruleIdentifier} = createRule({`, + ` name: ${toDoubleQuotedString(options.ruleName)},`, + ` description: ${toDoubleQuotedString(options.description)},`, + ' severity: "warning",', + ` url: ${toDoubleQuotedString(`https://azure.github.io/typespec-azure/docs/libraries/${config.docsLibraryDir}/rules/${options.ruleName}`)},`, + " messages: {", + ' default: "TODO: Add default diagnostic message.",', + " },", + " create(context) {", + " return {", + " // TODO: Implement visitor hooks (model, modelProperty, operation, enum, namespace, interface, union)", + " };", + " },", + "});", + "", + ].join("\n"); + + const testFile = [ + ...(config.testHostComment ? [config.testHostComment] : []), + config.testHostImport, + 'import { LinterRuleTester, createLinterRuleTester } from "@typespec/compiler/testing";', + 'import { beforeEach, describe, it } from "vitest";', + `import { ${ruleIdentifier} } from "../../src/rules/${options.ruleName}.js";`, + "", + "let tester: LinterRuleTester;", + "", + "beforeEach(async () => {", + ` const runner = await ${config.testHostFactory};`, + " tester = createLinterRuleTester(", + " runner,", + ` ${ruleIdentifier},`, + ` ${toDoubleQuotedString(config.packageNpmName)},`, + " );", + "});", + "", + `describe(${toDoubleQuotedString(options.ruleName)}, () => {`, + ' it("is valid when TODO: describe valid case", async () => {', + " await tester", + " .expect(", + " `", + " model Example {}", + " `,", + " )", + " .toBeValid();", + " });", + "", + ' it("emits diagnostic when TODO: describe invalid case", async () => {', + " await tester", + " .expect(", + " `", + " model Example {}", + " `,", + " )", + " .toEmitDiagnostics([", + " {", + ` code: ${toDoubleQuotedString(`${config.packageNpmName}/${options.ruleName}`)},`, + ' severity: "warning",', + ' message: "TODO: Expected diagnostic message",', + " },", + " ]);", + " });", + "});", + "", + ].join("\n"); + + const docsFile = [ + "---", + `title: ${toDoubleQuotedString(options.ruleName)}`, + "---", + "", + '```text title="Full name"', + `${config.packageNpmName}/${options.ruleName}`, + "```", + "", + "TODO: Add a description of what this rule checks and why it matters.", + "", + "#### ❌ Incorrect", + "", + "```tsp", + "// TODO: Add example of code that violates this rule", + "```", + "", + "#### ✅ Correct", + "", + "```tsp", + "// TODO: Add example of code that satisfies this rule", + "```", + "", + ].join("\n"); + + const importLine = `import { ${ruleIdentifier} } from "./rules/${options.ruleName}.js";`; + const updatedLinter = updateLinterFile(linterPath, importLine, ruleIdentifier); + + const plannedActions = [ + `create ${rulePath}`, + `create ${testPath}`, + `create ${docsPath}`, + `update ${linterPath}`, + ]; + + if (options.dryRun) { + console.log("Dry run: no files were written."); + for (const action of plannedActions) { + console.log(`- ${action}`); + } + return; + } + + writeText(rulePath, ruleFile); + writeText(testPath, testFile); + writeText(docsPath, docsFile); + writeText(linterPath, updatedLinter); + + console.log("Created linter rule scaffold:"); + for (const action of plannedActions) { + console.log(`- ${action}`); + } +} + +main(); diff --git a/eng/scripts/validate-pr.ts b/eng/scripts/validate-pr.ts new file mode 100644 index 0000000000..19d541b5ed --- /dev/null +++ b/eng/scripts/validate-pr.ts @@ -0,0 +1,608 @@ +import { spawnSync, type SpawnSyncReturns } from "child_process"; +import { readFileSync } from "fs"; +import path from "path"; +import process from "process"; + +type StepStatus = "passed" | "failed" | "warning" | "skipped"; + +type Options = { + fix: boolean; + skipBuild: boolean; + skipTest: boolean; + base: string; + verbose: boolean; + help: boolean; +}; + +type CommandResult = { + command: string; + args: string[]; + result: SpawnSyncReturns; + stdout: string; + stderr: string; +}; + +type StepResult = { + label: string; + status: StepStatus; + durationMs: number; + details: string[]; +}; + +const RESET = "\u001b[0m"; +const GREEN = "\u001b[32m"; +const RED = "\u001b[31m"; +const YELLOW = "\u001b[33m"; +const CYAN = "\u001b[36m"; +const BOLD = "\u001b[1m"; +const LINE = "━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━"; +const STATUS_ICON: Record = { + passed: `${GREEN}✅${RESET}`, + failed: `${RED}❌${RESET}`, + warning: `${YELLOW}⚠️${RESET}`, + skipped: `${YELLOW}⚠️${RESET}`, +}; + +const repoRoot = path.resolve(process.cwd()); +const options = parseArgs(process.argv.slice(2)); + +if (options.help) { + printUsage(); + process.exit(0); +} + +const diffCache = new Map(); +const changedFilesExclusions = loadChangedFilesExclusions(); +const steps: StepResult[] = []; + +printHeader(); + +steps.push(runStep("Branch is up to date", checkBranchUpToDate)); +steps.push(runStep("Build succeeds", checkBuild)); +steps.push(runStep("Tests pass", checkTests)); +steps.push(runStep("Lint passes", checkLint)); +steps.push(runStep("Format check passes", checkFormat)); +steps.push(runStep("Spelling check passes", checkSpelling)); +steps.push(runStep("Regen-docs is clean", checkRegenDocs)); +steps.push(runStep("Changeset exists", checkChangeset)); +steps.push(runStep("Diff is clean", checkDiff)); + +printSummary(steps); + +const hasFailures = steps.some((step) => step.status === "failed"); +process.exit(hasFailures ? 1 : 0); + +function parseArgs(args: string[]): Options { + const parsed: Options = { + fix: false, + skipBuild: false, + skipTest: false, + base: "origin/main", + verbose: false, + help: false, + }; + + for (let index = 0; index < args.length; index++) { + const arg = args[index]; + + switch (arg) { + case "--fix": + parsed.fix = true; + break; + case "--skip-build": + parsed.skipBuild = true; + break; + case "--skip-test": + parsed.skipTest = true; + break; + case "--base": { + const value = args[index + 1]; + if (!value) { + failWithUsage("Missing value for --base"); + } + parsed.base = value; + index++; + break; + } + case "--verbose": + parsed.verbose = true; + break; + case "--help": + parsed.help = true; + break; + default: + failWithUsage(`Unknown option: ${arg}`); + } + } + + return parsed; +} + +function failWithUsage(message: string): never { + console.error(`${RED}Error:${RESET} ${message}`); + console.log(); + printUsage(); + process.exit(1); +} + +function printUsage(): void { + console.log( + [ + "Usage: pnpm validate:pr [options]", + "", + "Options:", + " --fix Auto-fix formatting and lint issues", + " --skip-build Skip the build step", + " --skip-test Skip the test step", + " --base Base branch for diff comparison (default: origin/main)", + " --verbose Show detailed command output", + " --help Show usage", + ].join("\n"), + ); +} + +function printHeader(): void { + console.log(`${BOLD}Pre-PR Validation${RESET}`); + console.log(LINE); + console.log(); +} + +function printSummary(results: StepResult[]): void { + const passed = results.filter((step) => step.status === "passed").length; + const warnings = results.filter((step) => step.status === "warning").length; + const skipped = results.filter((step) => step.status === "skipped").length; + const failed = results.filter((step) => step.status === "failed").length; + + console.log(); + console.log(LINE); + + if (failed > 0) { + console.log(`Result: ${passed}/${results.length} passed ${RED}❌${RESET}`); + return; + } + + if (warnings > 0 || skipped > 0) { + const extra: string[] = []; + if (warnings > 0) { + extra.push(`${warnings} warning${warnings === 1 ? "" : "s"}`); + } + if (skipped > 0) { + extra.push(`${skipped} skipped`); + } + console.log( + `Result: ${passed}/${results.length} passed ${GREEN}✅${RESET} (${extra.join(", ")})`, + ); + return; + } + + console.log(`Result: ${passed}/${results.length} passed ${GREEN}✅${RESET}`); +} + +function runStep( + label: string, + action: () => Omit, +): StepResult { + const start = Date.now(); + + try { + const outcome = action(); + const step = { + label, + status: outcome.status, + details: outcome.details, + durationMs: Date.now() - start, + }; + printStep(steps.length + 1, step); + return step; + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + const step = { + label, + status: "failed" as const, + details: [message], + durationMs: Date.now() - start, + }; + printStep(steps.length + 1, step); + return step; + } +} + +function printStep(index: number, step: StepResult): void { + const prefix = ` ${index}. ${step.label} `; + const dots = ".".repeat(Math.max(2, 58 - prefix.length)); + console.log(`${prefix}${dots} ${STATUS_ICON[step.status]} (${formatDuration(step.durationMs)})`); + + for (const detail of step.details) { + if (!detail) continue; + for (const line of detail.split(/\r?\n/)) { + console.log(` ${line}`); + } + } +} + +function formatDuration(durationMs: number): string { + return `${(durationMs / 1000).toFixed(1)}s`; +} + +function checkBranchUpToDate(): Omit { + const fetchResult = fetchBaseBranch(options.base); + if (!isSuccess(fetchResult.result)) { + return failFromCommand(fetchResult, `Unable to fetch ${options.base}.`); + } + + const baseExists = runCommand("git", ["rev-parse", "--verify", options.base]); + if (!isSuccess(baseExists.result)) { + return failFromCommand(baseExists, `Base reference ${options.base} was not found.`); + } + + const ancestorCheck = runCommand("git", ["merge-base", "--is-ancestor", options.base, "HEAD"]); + const mergeTreeCheck = runCommand("git", ["merge-tree", "--write-tree", options.base, "HEAD"]); + + if (!isSuccess(mergeTreeCheck.result)) { + return { + status: "failed", + details: [ + `Potential merge conflicts detected with ${options.base}. Rebase before opening the PR.`, + ...commandOutput(mergeTreeCheck), + ], + }; + } + + if (!isSuccess(ancestorCheck.result)) { + return { + status: "warning", + details: [ + `Branch is behind ${options.base}. Run: git fetch origin main && git rebase ${options.base}`, + ], + }; + } + + return { status: "passed", details: [] }; +} + +function checkBuild(): Omit { + if (options.skipBuild) { + return { status: "skipped", details: ["Skipped by --skip-build."] }; + } + + const result = runCommand("pnpm", ["build"]); + if (!isSuccess(result.result)) { + return failFromCommand(result, "Build failed."); + } + + return { status: "passed", details: [] }; +} + +function checkTests(): Omit { + if (options.skipTest) { + return { status: "skipped", details: ["Skipped by --skip-test."] }; + } + + const result = runCommand("pnpm", ["test"]); + if (!isSuccess(result.result)) { + return failFromCommand(result, "Test failures found."); + } + + return { status: "passed", details: [] }; +} + +function checkLint(): Omit { + const command = options.fix ? ["lint:fix"] : ["lint"]; + const result = runCommand("pnpm", command); + + if (!isSuccess(result.result)) { + return failFromCommand( + result, + options.fix ? "Lint auto-fix failed." : "Lint errors found. Run: pnpm lint:fix", + ); + } + + return { status: "passed", details: [] }; +} + +function checkFormat(): Omit { + if (options.fix) { + const formatResult = runCommand("pnpm", ["format"]); + if (!isSuccess(formatResult.result)) { + return failFromCommand(formatResult, "Formatting failed."); + } + } + + const checkResult = runCommand("pnpm", ["format:check"]); + if (!isSuccess(checkResult.result)) { + return failFromCommand( + checkResult, + options.fix + ? "Formatting still has issues after pnpm format." + : "Formatting issues found. Run: pnpm format", + ); + } + + return { + status: "passed", + details: options.fix ? ["Applied formatting fixes before re-checking."] : [], + }; +} + +function checkSpelling(): Omit { + const result = runCommand("pnpm", ["cspell"]); + if (!isSuccess(result.result)) { + return failFromCommand(result, "Spelling issues found."); + } + + return { status: "passed", details: [] }; +} + +function checkRegenDocs(): Omit { + if (options.skipBuild) { + return { status: "skipped", details: ["Skipped because --skip-build also skips regen-docs."] }; + } + + const regenResult = runCommand("pnpm", ["regen-docs"]); + if (!isSuccess(regenResult.result)) { + return failFromCommand(regenResult, "Doc regeneration failed."); + } + + const diffResult = runCommand("git", ["diff", "--name-only", "--", "docs", "website"]); + if (!isSuccess(diffResult.result)) { + return failFromCommand(diffResult, "Unable to inspect regenerated docs."); + } + + const changedDocs = splitLines(diffResult.stdout); + if (changedDocs.length > 0) { + return { + status: "failed", + details: ["Generated docs are stale. Run: pnpm regen-docs", ...changedDocs], + }; + } + + return { status: "passed", details: [] }; +} + +function checkChangeset(): Omit { + const changedFiles = getDiffFiles(options.base); + if (!changedFiles) { + return { + status: "failed", + details: [`Unable to diff against ${options.base}.`], + }; + } + + const sourceFiles = changedFiles.filter((file) => { + const normalized = normalizePath(file); + if (normalized.startsWith(".chronus/changes/")) { + return false; + } + return !matchesExcludedFile(normalized); + }); + + if (sourceFiles.length === 0) { + return { status: "skipped", details: ["No source file changes detected."] }; + } + + const changesetResult = runCommand("git", [ + "diff", + "--name-only", + "--diff-filter=A", + `${options.base}...HEAD`, + "--", + ".chronus/changes/", + ]); + + if (!isSuccess(changesetResult.result)) { + return failFromCommand(changesetResult, "Unable to inspect changesets."); + } + + const addedChangesets = splitLines(changesetResult.stdout); + if (addedChangesets.length === 0) { + return { + status: "failed", + details: [ + "Source files changed but no new changeset was added under .chronus/changes/.", + "Run: pnpm change add", + ], + }; + } + + return { status: "passed", details: [] }; +} + +function checkDiff(): Omit { + const changedFiles = getDiffFiles(options.base); + if (!changedFiles) { + return { + status: "failed", + details: [`Unable to diff against ${options.base}.`], + }; + } + + const details = changedFiles.length > 0 ? [...changedFiles] : ["No files changed."]; + const warnings: string[] = []; + const hasPackageJsonChange = changedFiles.some((file) => + normalizePath(file).endsWith("package.json"), + ); + + if (changedFiles.some((file) => /(^|\/)dist\//.test(normalizePath(file)))) { + warnings.push("PR diff includes files under dist/."); + } + + if (changedFiles.some((file) => normalizePath(file).endsWith(".lock.yml"))) { + warnings.push("PR diff includes *.lock.yml files (often generated by gh-aw)."); + } + + if ( + changedFiles.some((file) => normalizePath(file) === "pnpm-lock.yaml") && + !hasPackageJsonChange + ) { + warnings.push("pnpm-lock.yaml changed without any package.json change."); + } + + if (changedFiles.some((file) => /(^|\/)node_modules\//.test(normalizePath(file)))) { + warnings.push("PR diff includes files under node_modules/."); + } + + if (warnings.length > 0) { + return { + status: "warning", + details: [...details, ...warnings.map((warning) => `Warning: ${warning}`)], + }; + } + + return { status: "passed", details }; +} + +function fetchBaseBranch(base: string): CommandResult { + const remoteMatch = /^([^/]+)\/(.+)$/.exec(base); + if (remoteMatch) { + const [, remote, branch] = remoteMatch; + return runCommand("git", ["fetch", remote, branch, "--quiet"]); + } + + return runCommand("git", ["fetch", "origin", "main", "--quiet"]); +} + +function getDiffFiles(base: string): string[] | undefined { + const cached = diffCache.get(base); + if (cached) { + return cached; + } + + const diffResult = runCommand("git", ["diff", "--name-only", `${base}...HEAD`]); + if (!isSuccess(diffResult.result)) { + if (options.verbose) { + for (const line of commandOutput(diffResult)) { + console.log(` ${line}`); + } + } + return undefined; + } + + const files = splitLines(diffResult.stdout); + diffCache.set(base, files); + return files; +} + +function loadChangedFilesExclusions(): RegExp[] { + const configPath = path.join(repoRoot, ".chronus", "config.yaml"); + + try { + const config = readFileSync(configPath, "utf-8"); + const patterns: string[] = []; + let inChangedFiles = false; + + for (const line of config.split(/\r?\n/)) { + if (!inChangedFiles) { + if (line.trim() === "changedFiles:") { + inChangedFiles = true; + } + continue; + } + + const match = /^\s*-\s*"?(![^"#]+)"?\s*$/.exec(line); + if (!match) { + if (/^\S/.test(line)) { + break; + } + continue; + } + + patterns.push(match[1]); + } + + return patterns.map(globToRegExp); + } catch { + return [/\.md$/i, /\.test\.ts$/i, /\.e2e\.ts$/i]; + } +} + +function globToRegExp(pattern: string): RegExp { + const normalized = normalizePath(pattern.replace(/^!/, "")); + const escaped = normalized.replace(/[.+^${}()|[\]\\]/g, "\\$&"); + const regexSource = escaped + .replace(/\*\*\//g, "(?:.*/)?") + .replace(/\*\*/g, ".*") + .replace(/\*/g, "[^/]*") + .replace(/\?/g, "."); + return new RegExp(`^${regexSource}$`, "i"); +} + +function matchesExcludedFile(file: string): boolean { + return changedFilesExclusions.some((pattern) => pattern.test(file)); +} + +function runCommand(command: string, args: string[]): CommandResult { + const display = formatCommand(command, args); + + if (options.verbose) { + console.log(`${CYAN}$ ${display}${RESET}`); + } + + const result = spawnSync(command, args, { + cwd: repoRoot, + shell: true, + encoding: "utf-8", + maxBuffer: 50 * 1024 * 1024, + }); + + const stdout = (result.stdout ?? "").trimEnd(); + const stderr = (result.stderr ?? "").trimEnd(); + + if (options.verbose) { + if (stdout) { + console.log(stdout); + } + if (stderr) { + console.error(stderr); + } + } + + return { command, args, result, stdout, stderr }; +} + +function isSuccess(result: SpawnSyncReturns): boolean { + return !result.error && result.status === 0; +} + +function failFromCommand( + command: CommandResult, + message: string, +): Omit { + return { + status: "failed", + details: [message, ...commandOutput(command)], + }; +} + +function commandOutput(command: CommandResult): string[] { + const lines = [`Command: ${formatCommand(command.command, command.args)}`]; + + if (command.result.error) { + lines.push(`Error: ${command.result.error.message}`); + } + if (command.stderr) { + lines.push(command.stderr); + } + if (command.stdout) { + lines.push(command.stdout); + } + if (!command.stderr && !command.stdout && typeof command.result.status === "number") { + lines.push(`Exit code: ${command.result.status}`); + } + + return lines; +} + +function formatCommand(command: string, args: string[]): string { + return [command, ...args].map((part) => (/\s/.test(part) ? `"${part}"` : part)).join(" "); +} + +function splitLines(text: string): string[] { + return text + .split(/\r?\n/) + .map((line) => line.trim()) + .filter(Boolean); +} + +function normalizePath(file: string): string { + return file.replace(/\\/g, "/"); +} diff --git a/package.json b/package.json index c8fd967b5c..0985245f57 100644 --- a/package.json +++ b/package.json @@ -11,6 +11,8 @@ "deps": "tsx eng/scripts/sync-deps.ts", "check:eng": "tsc -p ./tsconfig.eng.json --noEmit", "change": "chronus", + "create:linter-rule": "tsx eng/scripts/create-linter-rule.ts", + "validate:pr": "tsx eng/scripts/validate-pr.ts", "clean": "pnpm run-all clean", "cspell": "cspell --no-progress .", "dogfood": "pnpm install && pnpm build && pnpm run-all dogfood", diff --git a/website/src/content/docs/docs/howtos/contributing/creating-linter-rules.md b/website/src/content/docs/docs/howtos/contributing/creating-linter-rules.md new file mode 100644 index 0000000000..ceb0fc3364 --- /dev/null +++ b/website/src/content/docs/docs/howtos/contributing/creating-linter-rules.md @@ -0,0 +1,641 @@ +--- +title: Creating Linter Rules +--- + +This guide covers the full lifecycle of adding a TypeSpec linter rule for Azure +libraries in `typespec-azure`. It is written for both human developers and AI +coding agents that need a repeatable, end-to-end workflow. + +## Overview + +Linter rules enforce Azure API design guidelines on TypeSpec specifications. +Rules live in one of these packages: + +| Package path | Scope | npm name | +| ------------------------------------------ | --------------------------------- | ---------------------------------------------- | +| `packages/typespec-client-generator-core` | Client SDK generation rules | `@azure-tools/typespec-client-generator-core` | +| `packages/typespec-azure-core` | Data-plane and shared Azure rules | `@azure-tools/typespec-azure-core` | +| `packages/typespec-azure-resource-manager` | ARM-specific rules | `@azure-tools/typespec-azure-resource-manager` | + +### Where to put your rule + +- **`typespec-client-generator-core`** — Rules that pertain to decorators in `typespec-client-generator-core`, or are only about client SDK generation +- **`typespec-azure-resource-manager`** — Rules specific to ARM APIs +- **`typespec-azure-core`** — Rules that apply to both data-plane and ARM APIs, or only to data-plane APIs + +### Ruleset registration + +- Rules that apply to ARM APIs → add to `resource-manager` ruleset +- Rules that apply to data-plane APIs → add to `data-plane` ruleset +- Rules can be in both rulesets if they apply to both + +A complete rule usually touches **7+ files across 3+ packages**: + +1. Rule implementation in `packages//src/rules/.ts` +2. Linter registration in `packages//src/linter.ts` +3. Tests in `packages//test/rules/.test.ts` +4. Ruleset registration in `packages/typespec-azure-rulesets` +5. Rule documentation in `website/src/content/docs/docs/libraries//rules/` +6. Regenerated reference docs +7. A changeset in `.chronus/changes/` + +:::note +For new rules, assume the work is not finished until the rule compiles, tests +pass, docs are regenerated, `pnpm validate:pr` passes, and the PR survives the +external integration run against `Azure/azure-rest-api-specs`. +::: + +## Prerequisites + +Before you start: + +- Install **Node.js 22+** +- Enable **pnpm via corepack** +- Clone the repo **with submodules** +- Install dependencies with `pnpm install` + +```bash +corepack enable +git clone --recurse-submodules https://github.com/Azure/typespec-azure.git +cd typespec-azure +pnpm install +``` + +## Step-by-Step Process + +### Step 1: Plan the Rule + +Decide the rule metadata before touching code: + +| Decision | What to choose | +| ------------------- | --------------------------------------------------------------------------------------------- | +| Package | `typespec-client-generator-core`, `typespec-azure-core`, or `typespec-azure-resource-manager` | +| Rule name | Follow the rule naming guidelines below | +| Target ruleset(s) | Data-plane, resource-manager, or both | +| Enabled by default? | `true` or `false` | + +All linter rules use `warning` severity. This is not configurable. + +#### Rule Naming Guidelines + +- Use kebab-case (lowercase letters, numbers, hyphens only) +- DO NOT include the package name in the rule ID — the package is already part of + the fully-qualified diagnostic code (e.g., + `@azure-tools/typespec-azure-core/no-nullable`, not + `@azure-tools/typespec-azure-core/azure-core-no-nullable`) +- Use `no-` when the rule bans a construct or usage (e.g., + `no-nullable`, `no-enum`, `no-format`) +- Use `use-` when the rule points users toward a standard or + preferred pattern (e.g., `use-standard-operations`, `use-extensible-enum`) +- Keep names short and concise — prefer `no-enum` over + `no-enum-type-usage` + +Also review existing rules in `packages//src/rules/` to match local +patterns for naming, visitors, diagnostics, and fixes. + +:::note +AI agents should explicitly record the intended package, rule name, ruleset +entry, and whether the rule is enabled by default before scaffolding. +Severity does not need planning because all linter rules use `warning`. +That prevents follow-up churn across `linter.ts`, tests, and rulesets. +::: + +### Step 2: Scaffold the Files + +Use the repo scaffold command: + + +```bash +pnpm create:linter-rule --package --description "What the rule enforces" +``` + +All linter rules use `warning` severity, so the scaffold command does not +accept a severity flag. + +It creates or updates the main rule artifacts: + +- Rule source +- Rule test +- Rule docs page +- `linter.ts` + +Concretely, expect changes like: + +- `packages//src/rules/.ts` +- `packages//test/rules/.test.ts` +- `website/src/content/docs/docs/libraries//rules/.md` +- `packages//src/linter.ts` + +### Step 3: Write Tests First (TDD) + +Write or refine tests before implementing the rule logic. + +For data-plane rules, start from the Azure Core test host: + +```typescript +import { TesterWithService } from "#test/test-host.js"; +import { LinterRuleTester, createLinterRuleTester } from "@typespec/compiler/testing"; +import { beforeEach, describe, it } from "vitest"; +import { myNewRule } from "../../src/rules/my-new-rule.js"; + +let tester: LinterRuleTester; + +beforeEach(async () => { + const runner = await TesterWithService.createInstance(); + tester = createLinterRuleTester(runner, myNewRule, "@azure-tools/typespec-azure-core"); +}); +``` + +For ARM rules, use the ARM tester: + +```typescript +import { Tester } from "#test/tester.js"; +import { LinterRuleTester, createLinterRuleTester } from "@typespec/compiler/testing"; +import { beforeEach } from "vitest"; +import { myArmRule } from "../../src/rules/my-arm-rule.js"; + +let tester: LinterRuleTester; + +beforeEach(async () => { + const runner = await Tester.createInstance(); + tester = createLinterRuleTester( + runner, + myArmRule, + "@azure-tools/typespec-azure-resource-manager", + ); +}); +``` + +For client generator rules, start from one of the package-local testers: + +```typescript +import { LinterRuleTester, createLinterRuleTester } from "@typespec/compiler/testing"; +import { beforeEach } from "vitest"; +import { myClientRule } from "../../src/rules/my-client-rule.js"; +import { SimpleTester } from "../tester.js"; + +let tester: LinterRuleTester; + +beforeEach(async () => { + const runner = await SimpleTester.createInstance(); + tester = createLinterRuleTester( + runner, + myClientRule, + "@azure-tools/typespec-client-generator-core", + ); +}); +``` + +Core assertion patterns: + +```typescript +await tester.expect(`model Pet { name: string; }`).toBeValid(); + +await tester.expect(`model BadPet { owner: string | null; }`).toEmitDiagnostics([ + { + code: "@azure-tools/typespec-azure-core/my-new-rule", + severity: "warning", + message: "Expected diagnostic message", + }, +]); + +await tester.expect(`enum Color { red }`).applyCodeFix("enum-to-extensible-union").toEqual(` + union Color { + string, + red: "red", + } + `); +``` + +What to test: + +- Valid code that must stay silent +- Invalid code that must emit the diagnostic +- Equivalence classes: group inputs by how the rule handles them +- For `ModelProperty` rules, include concrete examples such as simply defined + properties, properties introduced through spread or `is`, and inherited + properties +- Boundary conditions that are specific to the rule logic +- At least one test verifying that library types in `Azure.Core` and + `Azure.ResourceManager` are not subject to the rule +- Near-misses that should **not** trigger + +Verify the tests fail before implementation: + +```bash +pnpm --filter "@azure-tools/typespec-..." build +pnpm --filter "@azure-tools/typespec-..." test +``` + +### Step 4: Implement the Rule + +A typical rule is built with `createRule()` and one or more semantic visitor +hooks: + +```typescript +import { + ModelProperty, + Namespace, + createRule, + defineCodeFix, + getService, + getSourceLocation, + paramMessage, +} from "@typespec/compiler"; + +function createReadOnlyFix(property: ModelProperty) { + return defineCodeFix({ + id: "add-readonly", + label: "Add @visibility(Lifecycle.Read)", + fix: (fixContext) => { + if (property.node === undefined) return []; + return fixContext.prependText( + getSourceLocation(property.node), + "@visibility(Lifecycle.Read)\n", + ); + }, + }); +} + +export const myNewRule = createRule({ + name: "my-new-rule", + description: "Require a specific Azure shape.", + severity: "warning", + url: "https://azure.github.io/typespec-azure/docs/libraries/azure-core/rules/my-new-rule", + messages: { + default: "This type violates the rule.", + withName: paramMessage`Property '${"propertyName"}' must meet the Azure guideline.`, + }, + create(context) { + return { + modelProperty(property: ModelProperty) { + if (property.node === undefined) return; + if (property.model?.namespace) { + const service = getService(context.program, property.model.namespace as Namespace); + if (service === undefined) return; + } + + if (property.name === "badName") { + context.reportDiagnostic({ + target: property, + messageId: "withName", + format: { propertyName: property.name }, + codefixes: [createReadOnlyFix(property)], + }); + } + }, + }; + }, +}); +``` + +Common visitor hooks: + +- `model` +- `modelProperty` +- `operation` +- `interface` +- `enum` +- `enumMember` +- `union` +- `unionVariant` +- `namespace` +- `scalar` + +Common implementation patterns: + +- Use `context.reportDiagnostic({ target })` to report on the exact symbol +- Use `paramMessage` for interpolated messages +- Use `defineCodeFix` when a deterministic fix exists +- Skip template declarations or template signatures when the declaration itself + is not the real target +- Check whether a type belongs to a service namespace before reporting +- Walk hierarchy when needed, such as `model.baseModel` or derived types + +A minimal diagnostic looks like this: + +```typescript +context.reportDiagnostic({ + target: property, +}); +``` + +A parameterized diagnostic looks like this: + +```typescript +context.reportDiagnostic({ + target: property, + messageId: "withName", + format: { propertyName: property.name }, +}); +``` + +A code-fix-enabled diagnostic looks like this: + +```typescript +context.reportDiagnostic({ + target: property, + codefixes: [createReadOnlyFix(property)], +}); +``` + +:::caution +Keep the rule narrow. A broadly written rule may pass local unit tests and still +break dozens of real specs in external integration. +::: + +### Step 5: Verify Tests Pass + +Once the rule is implemented, rebuild and rerun tests: + +```bash +pnpm --filter "@azure-tools/typespec-..." build +pnpm --filter "@azure-tools/typespec-..." test +``` + +Do not move on until the new tests pass and existing tests for that package stay +green. + +### Step 6: Register in Rulesets + +Add the rule to the appropriate ruleset file or files: + +- Data-plane: `packages/typespec-azure-rulesets/src/rulesets/data-plane.ts` +- ARM: `packages/typespec-azure-rulesets/src/rulesets/resource-manager.ts` + +Ruleset guidance: + +- `typespec-client-generator-core` rules generally go in **both** rulesets +- `typespec-azure-core` rules intended for both ARM and data-plane also go in + **both** rulesets +- Only rules that are truly ARM-specific go exclusively in the resource-manager + ruleset +- Data-plane-only rules go only in the data-plane ruleset + +Entry format: + +```typescript +"@azure-tools/typespec-/": true, +``` + +Use `false` instead of `true` when the rule should ship but stay disabled by +default. + +The test `validate-rules-defined.test.ts` catches missing ruleset entries. + +Verify ruleset registration: + +```bash +pnpm --filter "@azure-tools/typespec-azure-rulesets..." build +pnpm --filter "@azure-tools/typespec-azure-rulesets..." test +``` + +### Step 7: Write Documentation + +Each rule needs its own rule page: + +- `website/src/content/docs/docs/libraries//rules/.md` + +That page should include: + +- Frontmatter with a `title` +- A `Full name` code block +- A plain-language description +- `#### ❌ Incorrect` examples +- `#### ✅ Correct` examples + +Example rule doc shape: + +````markdown +--- +title: "my-new-rule" +--- + +```text title="Full name" +@azure-tools/typespec-azure-core/my-new-rule +``` + +Brief description of the rule. + +#### ❌ Incorrect + +```tsp +model BadThing { + badName: string; +} +``` + +#### ✅ Correct + +```tsp +model GoodThing { + goodName: string; +} +``` +```` + +After editing the rule docs, regenerate package docs: + +```bash +pnpm --filter "@azure-tools/typespec-..." build +pnpm --filter "@azure-tools/typespec-" regen-docs +``` + +### Step 8: Create a Changeset + +Create a changelog entry: + +```bash +pnpm change add +``` + +Choose: + +- `feature` for a new rule +- `fix` for a bug fix to an existing rule +- Package: `@azure-tools/typespec-` + +### Step 9: Local Validation + +Run the repo validation flow: + +```bash +pnpm validate:pr +``` + +This checks: + +- Branch sync +- Build +- Tests +- Lint +- Format +- `cspell` spelling +- `regen-docs` freshness +- Changeset presence +- Diff cleanliness + +Use `--fix` when you want auto-fixes for formatting and lint: + +```bash +pnpm validate:pr --fix +``` + +### Step 10: External Integration Check + +This step is critical for linter rules. New diagnostics must not break existing +Azure service specs in `Azure/azure-rest-api-specs`. + +After opening the PR: + +1. Apply the `int:azure-specs` label. An agent can do this with `gh pr edit --add-label "int:azure-specs"`. +2. If the agent cannot apply the label, instruct the user to apply it manually. +3. Monitor the workflow with `gh run list --workflow=external-integration.yml` and wait for the External Integration run. +4. Review any failures before requesting review. + +The workflow: + +1. Builds and packs all `typespec-azure` packages from your PR +2. Checks out `azure-rest-api-specs` main +3. Patches the packages into that repo +4. Runs `tsp-integration azure-specs --stage validate` +5. Checks for unexpected changes + +If the External Integration check fails, your rule produces diagnostics on +existing specs in `Azure/azure-rest-api-specs`. To resolve this: + +1. **Apply an API-neutral fix to the spec** (preferred) — If the fix doesn't + change API behavior (for example, adding a decorator or renaming a type to + follow conventions), submit a PR to `Azure/azure-rest-api-specs` on the + **main** branch. +2. **Suppress the rule** — If the spec cannot be fixed without changing API + behavior, add a `// suppress` comment. Suppressions can always go to the + **main** branch. +3. **Fix on typespec-next branch** — If the fix requires unreleased TypeSpec + APIs, types, or behavior that aren't yet available in the published + packages, submit the fix to the **typespec-next** branch, which uses nightly + TypeSpec builds. +4. **Link your spec fix PR** — Always link the PR that fixes the spec failures + in your linter rule PR description. Reviewers need to see both together. + +For local experimentation, see +[Testing a change in azure-rest-api-specs](https://github.com/Azure/typespec-azure?tab=contributing-ov-file#testing-a-change-in-repo-azure-rest-api-specs). + +## Reference + +### Naming Conventions + +| Item | Convention | Example | +| ---------------- | ------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------- | +| Rule name | kebab-case; omit package names; prefer `no-*` and `use-*` prefixes | `no-nullable-key` | +| Export variable | camelCase + `Rule` | `noNullableKeyRule` | +| Import extension | Always `.js` | `from "./rules/no-nullable-key.js"` | +| Diagnostic code | `@azure-tools/typespec-/` | `@azure-tools/typespec-azure-core/no-nullable-key` | +| Rule URL | `https://azure.github.io/typespec-azure/docs/libraries//rules/` | `https://azure.github.io/typespec-azure/docs/libraries/azure-core/rules/no-nullable-key` | + +**Rule Naming Guidelines:** + +- Use kebab-case (lowercase letters, numbers, hyphens only) +- DO NOT include the package name in the rule ID — the package is already part of + the fully-qualified diagnostic code (e.g., + `@azure-tools/typespec-azure-core/no-nullable`, not + `@azure-tools/typespec-azure-core/azure-core-no-nullable`) +- Use `no-` when the rule bans a construct or usage (e.g., + `no-nullable`, `no-enum`, `no-format`) +- Use `use-` when the rule points users toward a standard or + preferred pattern (e.g., `use-standard-operations`, `use-extensible-enum`) +- Keep names short and concise — prefer `no-enum` over + `no-enum-type-usage` + +### File Locations + +```text +packages/ +├── typespec-client-generator-core/ +│ ├── src/ +│ │ ├── linter.ts +│ │ └── rules/ +│ │ └── .ts +│ └── test/ +│ ├── tester.ts +│ └── rules/ +│ └── .test.ts +├── typespec-azure-core/ +│ ├── src/ +│ │ ├── linter.ts +│ │ └── rules/ +│ │ └── .ts +│ └── test/ +│ ├── test-host.ts +│ └── rules/ +│ └── .test.ts +├── typespec-azure-resource-manager/ +│ ├── src/ +│ │ ├── linter.ts +│ │ └── rules/ +│ │ └── .ts +│ └── test/ +│ ├── tester.ts +│ └── rules/ +│ └── .test.ts +├── typespec-azure-rulesets/ +│ ├── src/rulesets/data-plane.ts +│ └── src/rulesets/resource-manager.ts +website/ +└── src/content/docs/docs/libraries/ + ├── azure-core/rules/.md + ├── azure-resource-manager/rules/.md + └── typespec-client-generator-core/rules/.md +``` + +### Test Hosts + +| Package | Import | Notes | +| --------------------------------- | -------------------------------------------------------- | --------------------------------------------------------------------- | +| `typespec-client-generator-core` | `import { SimpleTester } from "../tester.js"` | Client SDK generation context; other local testers are also available | +| `typespec-azure-core` | `import { TesterWithService } from "#test/test-host.js"` | Wraps in `@service namespace` | +| `typespec-azure-resource-manager` | `import { Tester } from "#test/tester.js"` | ARM context | + +### Common Compiler APIs + +- `createRule` +- `paramMessage` +- `defineCodeFix` +- `getService` +- `Model` +- `ModelProperty` +- `Operation` +- `Namespace` +- `Type` + +### Useful Patterns from Existing Rules + +- Check decorators +- Walk model hierarchy +- Check whether the symbol is in a service namespace +- Inspect property type details +- Skip template declarations or uninstantiated template signatures + +## Troubleshooting + +| Problem | Cause | Fix | +| ------------------------------ | ------------------------- | ----------------------------------------------------------------- | +| `validate-rules-defined` fails | Rule not in ruleset | Add to `data-plane.ts` or `resource-manager.ts` | +| Tests can't find rule | Import path wrong | Check the `.js` extension and `linter.ts` | +| `pnpm format` fails | Prettier plugin not built | Run `pnpm --filter "@typespec/prettier-plugin-typespec..." build` | +| External Integration fails | Rule flags existing specs | Fix the spec, suppress the rule, or use `typespec-next` as needed | +| `regen-docs` shows changes | Forgot `regen-docs` | Run it and commit the output | +| Changeset missing | `pnpm change add` not run | Run it and select the package | + +## Checklist + +- [ ] Rule implementation complete, handles edge cases +- [ ] Tests cover valid, invalid, edge cases (≥95% branch coverage) +- [ ] Rule registered in `linter.ts` +- [ ] Rule listed in appropriate ruleset(s) +- [ ] Documentation has realistic examples +- [ ] Reference docs regenerated +- [ ] Changeset created +- [ ] `pnpm validate:pr` passes +- [ ] `int:azure-specs` External Integration check passes +- [ ] PR diff contains only expected files diff --git a/website/src/content/docs/docs/howtos/contributing/when-to-write-a-linting-rule.md b/website/src/content/docs/docs/howtos/contributing/when-to-write-a-linting-rule.md new file mode 100644 index 0000000000..7b27878adc --- /dev/null +++ b/website/src/content/docs/docs/howtos/contributing/when-to-write-a-linting-rule.md @@ -0,0 +1,279 @@ +--- +title: When to Write a Linting Rule +--- + +This guidance helps authors understand when a linting rule is the right mechanism for +enforcing an Azure spec rule, as opposed to other diagnostic mechanisms or AI +authoring/validation tools. + +## Part 1: Diagnostics and Linting Rules + +### What Is a Diagnostic? + +A **diagnostic** is the general mechanism for reporting messages during TypeSpec spec +processing. Diagnostics may be emitted by: + +- The compiler (syntax errors, type resolution failures) +- Decorators (invalid arguments, misuse) +- Library validation (semantic checks on types and models) +- Emitters (issues during code generation) +- Linting rules (spec quality violations) + +### Diagnostic Severity Levels + +| Property | Error | Warning | +| ---------------- | ------------------------------------------------------------------- | ------------------------------------------------------------------------ | +| **Effect** | Stops compilation | Does not stop compilation | +| **Suppressible** | No — must always be fixed | Yes — with `#suppress` directive and justification | +| **Blocks CI** | Always (compilation fails) | Yes in Azure specs, unless explicitly suppressed | +| **Emitted by** | Compiler, decorators, library validation, emitters | Compiler, decorators, library validation, emitters, **or linting rules** | +| **Indicates** | Fatal issue — inconsistency, syntax error, or unrecoverable problem | Non-fatal issue — may vary from advisory to serious but is not fatal | + +### What Is a Linting Rule? + +A linting rule is a specific kind of diagnostic check that: + +- Is defined in a linter library (not the compiler, a decorator, or an emitter) +- **Always** emits warning-severity diagnostics (never errors) +- Enforces rules for Azure specs — ranging from best practices and cross-service consistency + to detection of substantial API issues +- Is suppressible — authors can add `#suppress "" ""` + +Linting rules never emit errors because they enforce _rules about spec quality_, not +_correctness_. A spec that violates a linting rule is still structurally valid and will +compile — but it may have significant issues ranging from inconsistency with other Azure +services to serious API design problems. + +In Azure specs, unsuppressed warning diagnostics block CI. This means linting rule violations +must be either fixed or explicitly suppressed with justification — they are never silently +ignored. + +### Suppression Should Be Exceptional + +When designing a linting rule, authors should ensure that suppression is rare and requires +substantial thought and justification. A well-designed rule: + +- Flags patterns that genuinely need to be fixed in the vast majority of cases +- Has rare enough exceptions that suppression feels weighty, not routine +- Does not produce frequent false positives that train authors to suppress reflexively + +If you anticipate that a rule will be frequently suppressed, reconsider whether the rule is +appropriate. Frequent suppression indicates either that the detection is too broad (high false +positive rate) or that the guideline has too many legitimate exceptions to be enforced +mechanically. In either case, the guideline may be better served by AI tooling or by +narrowing the rule's scope. + +### When Should Something Be an Error Diagnostic vs. a Linting Rule? + +Use **error diagnostics** (in the compiler, a decorator, or library validation) when: + +- The spec has a fatal issue — it is inconsistent, uses invalid syntax, or cannot be compiled +- No valid use case exists for the pattern (it is always wrong, not merely inadvisable) +- The check is part of the type system, decorator contract, or emitter requirements + +Use **linting rules** when: + +- The spec is technically valid but violates Azure API rules — whether best practices, + cross-service consistency requirements, or significant API design standards +- Legitimate exceptions may exist that justify suppression, but such exceptions are rare +- The issue is important enough that violations should block CI unless explicitly justified + +## Part 2: Linting Rules vs. AI Tooling + +### Core Principle + +> Prefer deterministic linting when the guideline can be expressed as a stable, reproducible +> check with high confidence and actionable diagnostics. Prefer AI when the guideline requires +> semantic judgment, domain intent, content generation, or evaluation of qualitative adequacy. +> Use hybrid approaches when deterministic rules can identify objective gaps and AI can help +> interpret, prioritize, or remediate them. + +The decision to create a linting rule hinges primarily on **determinism of detection** — can you +reliably identify violations with low false positives? Whether the _resolution_ requires judgment +is a separate question that determines whether the rule is standalone or hybrid, not whether it +should exist. + +### The Two Axes of Decision + +When evaluating whether a guideline should become a linting rule, consider two independent +questions: + +1. **Can violations be detected deterministically with high fidelity?** (detection axis) +2. **Can violations be resolved without contextual judgment?** (resolution axis) + +These combine into four categories: + +| Detection | Resolution | Approach | +| ---------------------------------- | -------------------------- | ------------------------------------------------------- | +| Deterministic, low false positives | Unambiguous or few options | **Standalone linting rule** — detects and may offer fix | +| Deterministic, low false positives | Requires judgment/context | **Hybrid** — rule detects, AI assists resolution | +| Requires semantic judgment | Requires judgment/context | **Defer to AI tooling** | +| Requires semantic judgment | Unambiguous or few options | **Defer to AI tooling** — detection is the bottleneck | + +The key insight: **detection determinism decides whether a rule should exist.** Resolution +complexity decides whether it needs AI assistance, not whether it's worth building. + +### Create a Linting Rule When + +These criteria all relate to the **reliability of detection**: + +#### 1. The violation is mechanically identifiable from the AST + +You can write a predicate over the TypeSpec syntax tree that reliably identifies violations. The +check relies on structural or syntactic properties, not on understanding what the API means. + +- ✅ _"No resource type should use the suffix 'Resource'"_ — name check +- ✅ _"All operations must have an api-version parameter"_ — parameter presence check +- ✅ _"Enums must use the extensible pattern"_ — structural pattern check + +#### 2. False positive rate is very low + +The rule should almost never flag code that is actually correct. If you cannot distinguish +violations from valid patterns mechanically, the rule will produce noise that erodes author trust +in the linter. + +#### 3. Exceptions are rare and handleable via suppression + +The guideline doesn't need to be exceptionless. Linter warnings are suppressible by design. A +rule is appropriate as long as exceptions are infrequent enough that suppression is a reasonable +mechanism (not a routine annoyance). + +#### 4. The diagnostic is actionable + +The author can understand _what's wrong_ from the diagnostic message. They may or may not know +how to fix it without help — that determines standalone vs. hybrid, not whether the rule should +exist. + +#### 5. Speed of feedback matters + +Linting rules provide instant, in-editor feedback. For guidelines where catching violations early +prevents expensive rework, deterministic detection is strongly preferred over async AI review. + +### Defer to AI Tooling When + +These criteria indicate that **detection itself requires judgment**: + +#### 1. Identifying the violation requires semantic or domain understanding + +The guideline's applicability depends on what the API _means_, not just its structural shape. No +AST predicate can reliably determine whether the code violates the guideline. + +- 🤖 _"Resource types should have clear, concise names"_ — "clear" depends on domain context +- 🤖 _"Choose appropriate HTTP methods for operations"_ — requires understanding intent +- 🤖 _"Model structure should reflect the resource lifecycle"_ — requires domain modeling judgment + +#### 2. Detection would produce unacceptable false positives + +If the mechanical proxy for the guideline would flag many valid patterns, it should not be a +linting rule. Authors trained to suppress noise will also suppress legitimate findings. + +#### 3. The guideline evaluates subjective quality + +Assessing whether something is _good enough_ rather than _present or structurally correct_ is +inherently non-deterministic. + +- 🤖 _"Documentation is clear, accurate, and useful"_ +- 🤖 _"The API surface is ergonomic for the target scenarios"_ +- 🤖 _"Naming clearly communicates purpose to consumers"_ + +### Hybrid Approach: Deterministic Detection + AI-Assisted Resolution + +This is the middle ground where a linting rule _should_ exist because detection is reliable, but +the **fix** requires contextual judgment. The rule identifies the problem; AI helps solve it. + +| Pattern | Rule Detects (deterministic) | AI Assists (judgment) | +| ------------------------- | ---------------------------- | -------------------------------------- | +| **Missing documentation** | `@doc` decorator absent | Generates appropriate text | +| **Naming violation** | Suffix/prefix/casing wrong | Suggests contextually appropriate name | +| **Missing pagination** | List operation lacks paging | Helps structure the paging model | +| **Overly broad type** | `Record` used | Suggests appropriate typed alternative | + +**Why the rule still matters in hybrid cases:** + +- It provides a structured, reliable signal that AI tools can consume +- It ensures the issue is never silently ignored (unsuppressed warnings block CI) +- It gives instant in-editor feedback even when AI tools aren't active +- It makes the guideline auditable — you can count violations, track suppressions, measure + compliance + +### Anti-Patterns to Avoid + +#### ❌ Skipping a rule because the fix is hard + +If detection is reliable, create the rule even if the fix requires judgment. The hybrid pattern +exists for exactly this case. A diagnostic that says "this model uses `Record`, which +limits SDK usability" is valuable even without an auto-fix. + +#### ❌ Rules whose easiest fix is meaningless mechanical compliance + +If the rule incentivizes authors to add useless placeholders (e.g., `@doc("The Foo property")`) +just to silence the warning, the rule needs complementary AI review of content quality. The rule +is still worth having — it catches _absence_ — but it shouldn't be the only check. + +#### ❌ Rules encoding service-specific policy as universal Azure policy + +A pattern that's wrong for one service may be correct for another. Universal rules should reflect +truly universal guidelines. + +#### ❌ Rules that require expensive whole-program or cross-version analysis + +If the rule needs to compare against previous API versions or analyze the entire spec graph, it +may be too expensive for real-time editor feedback. Consider running such checks only in CI +rather than in-editor. + +#### ❌ AI as the sole enforcement for consistently-applied guidelines + +AI validation is non-deterministic and hard to audit. If a guideline must be reproducibly +enforced across all services, a deterministic rule (even a simple structural proxy) provides the +necessary backstop. + +## Decision Framework + +| Factor | Favors Linting Rule | Favors AI Tooling | +| --------------------------- | ------------------------------ | -------------------------------------- | +| **Detection reliability** | AST predicate, high confidence | Requires semantic understanding | +| **False positive rate** | Very low | Moderate to high | +| **Knowledge for detection** | Local structural/syntactic | Domain, historical, or cross-service | +| **Speed importance** | Critical (in-editor feedback) | Async/advisory acceptable | +| **Guideline maturity** | Well-established, stable | Evolving, subjective | +| **Auditability need** | Must track compliance | Advisory is sufficient | +| **Maintenance cost** | Simple AST check, stable APIs | Complex inference, frequent exceptions | + +Note: **fix complexity does not appear in this table** — it affects whether the rule is +standalone or hybrid, not whether it should exist. + +## Rollout Considerations + +When a new rule produces diagnostics on existing specs, each violation must be individually +addressed before the rule can ship in a ruleset. All rules in a ruleset apply universally to all +specs that ruleset covers — there is no mechanism to exempt individual specs from specific rules. + +For each existing violation, the author must decide: + +1. **Fix the violation** (preferred) — If the fix is API-neutral (doesn't require + service-specific knowledge and doesn't meaningfully change downstream artifacts), apply the + fix directly. + +2. **Suppress with FIXME** — If the violation cannot be fixed without service-specific knowledge + or would cause meaningful downstream changes, suppress it with a FIXME comment indicating why + and what would be needed to resolve it. + +The External Integration check (`int:azure-specs`) identifies which existing specs produce new +diagnostics. See the [Creating Linter Rules](./creating-linter-rules.md) guide for the workflow +of submitting spec fixes alongside linter rule PRs. + +## Summary Flowchart + +```text +Is the code invalid / will it not compile? +├── Yes → Compiler ERROR diagnostic (not a linting rule) +└── No → Does it violate a rule for Azure specs? + └── Yes → Can the violation be identified from AST/structure alone? + ├── No → Defer to AI tooling + └── Yes → Would it produce high false positives? + ├── Yes → Defer to AI tooling + └── No → CREATE A LINTING RULE ✅ + └── Can the fix be applied without contextual judgment? + ├── Yes → Standalone rule (may include code fix) + └── No → Hybrid: rule detects, AI assists resolution +```