-
Notifications
You must be signed in to change notification settings - Fork 391
Validate rules referenced by a library's rulesets at build time #11864
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Timothee Guerin (timotheeguerin)
wants to merge
2
commits into
microsoft:main
Choose a base branch
from
timotheeguerin:library-linter-validate-rulesets
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
22 changes: 22 additions & 0 deletions
22
.chronus/changes/library-linter-validate-rulesets-2026-8-4-22-15-0.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| --- | ||
| changeKind: feature | ||
| packages: | ||
| - "@typespec/library-linter" | ||
| --- | ||
|
|
||
| Validate that rules and rulesets referenced by the rulesets a library defines actually exist. Previously a dangling reference was only reported when a consumer happened to extend the offending ruleset. | ||
|
|
||
| ```ts | ||
| export const $linter = defineLinter({ | ||
| rules: [casingRule], | ||
| ruleSets: { | ||
| recommended: { | ||
| // warning: Rule 'removed-rule' referenced by ruleset '@typespec/best-practices/recommended' | ||
| // is not defined in library '@typespec/best-practices'. | ||
| enable: { "@typespec/best-practices/removed-rule": true }, | ||
| }, | ||
| }, | ||
| }); | ||
| ``` | ||
|
|
||
| References to a library that is not part of the compilation are skipped, and only the rulesets of the library being compiled are validated. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,144 @@ | ||
| import { | ||
| resolveLinterDefinition, | ||
| type LinterResolvedDefinition, | ||
| type LinterRuleSet, | ||
| type Program, | ||
| } from "@typespec/compiler"; | ||
| import type { JsSourceFileNode } from "@typespec/compiler/ast"; | ||
| import { reportDiagnostic } from "./lib.js"; | ||
|
|
||
| interface LoadedLinter { | ||
| readonly libName: string; | ||
| readonly linter: LinterResolvedDefinition; | ||
| /** JS file declaring the linter. Used as the diagnostic target so reports have a location. */ | ||
| readonly node: JsSourceFileNode; | ||
| /** Whether this linter belongs to the library being compiled as opposed to one of its dependencies. */ | ||
| readonly isProject: boolean; | ||
| } | ||
|
|
||
| /** | ||
| * Validate that every rule and ruleset referenced by the rulesets of the library being compiled | ||
| * actually exists. Without this a dangling reference is only reported when a consumer happens to | ||
| * extend the offending ruleset. | ||
| */ | ||
| export function validateRuleSets(program: Program) { | ||
| const { linters, knownLibraries } = collectLibraries(program); | ||
| const knownRules = new Set<string>(); | ||
| const knownRuleSets = new Set<string>(); | ||
| for (const { libName, linter } of linters) { | ||
| for (const rule of linter.rules) { | ||
| knownRules.add(rule.id); | ||
| } | ||
| for (const name of Object.keys(linter.ruleSets)) { | ||
| knownRuleSets.add(`${libName}/${name}`); | ||
| } | ||
| } | ||
|
|
||
| for (const { libName, linter, node, isProject } of linters) { | ||
| if (!isProject) continue; | ||
| for (const [name, ruleSet] of Object.entries(linter.ruleSets)) { | ||
| validateRuleSet(program, `${libName}/${name}`, ruleSet, node, { | ||
| knownLibraries, | ||
| knownRules, | ||
| knownRuleSets, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| interface KnownReferences { | ||
| readonly knownLibraries: ReadonlySet<string>; | ||
| readonly knownRules: ReadonlySet<string>; | ||
| readonly knownRuleSets: ReadonlySet<string>; | ||
| } | ||
|
|
||
| function validateRuleSet( | ||
| program: Program, | ||
| ruleSetName: string, | ||
| ruleSet: LinterRuleSet, | ||
| target: JsSourceFileNode, | ||
| known: KnownReferences, | ||
| ) { | ||
| for (const ref of ruleSet.extends ?? []) { | ||
| validateReference(program, ruleSetName, ref, "ruleset", target, known); | ||
| } | ||
| for (const ref of Object.keys(ruleSet.enable ?? {})) { | ||
| validateReference(program, ruleSetName, ref, "rule", target, known); | ||
| } | ||
| for (const ref of Object.keys(ruleSet.disable ?? {})) { | ||
| validateReference(program, ruleSetName, ref, "rule", target, known); | ||
| } | ||
| } | ||
|
|
||
| function validateReference( | ||
| program: Program, | ||
| ruleSetName: string, | ||
| ref: string, | ||
| kind: "rule" | "ruleset", | ||
| target: JsSourceFileNode, | ||
| known: KnownReferences, | ||
| ) { | ||
| const parsed = parseReference(ref); | ||
| if (parsed === undefined) { | ||
| reportDiagnostic(program, { | ||
| code: "invalid-rule-reference", | ||
| format: { ref, ruleSetName }, | ||
| target, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| // The referenced library is not part of this compilation, so there is nothing to check against. | ||
| // This happens when a ruleset references a library that the current library does not import. | ||
| if (!known.knownLibraries.has(parsed.libraryName)) { | ||
| return; | ||
| } | ||
|
|
||
| const exists = kind === "rule" ? known.knownRules.has(ref) : known.knownRuleSets.has(ref); | ||
| if (!exists) { | ||
| reportDiagnostic(program, { | ||
| code: kind === "rule" ? "unknown-rule" : "unknown-rule-set", | ||
| format: { name: parsed.name, libraryName: parsed.libraryName, ruleSetName }, | ||
| target, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| function parseReference(ref: string): { libraryName: string; name: string } | undefined { | ||
| const segments = ref.split("/"); | ||
| const name = segments.pop(); | ||
| const libraryName = segments.join("/"); | ||
| if (!libraryName || !name) { | ||
| return undefined; | ||
| } | ||
| return { libraryName, name }; | ||
| } | ||
|
|
||
| function collectLibraries(program: Program): { | ||
| linters: LoadedLinter[]; | ||
| knownLibraries: Set<string>; | ||
| } { | ||
| const linters: LoadedLinter[] = []; | ||
| // Every library loaded in this compilation, including those defining no linter: a reference into | ||
| // such a library is known to be broken, unlike one pointing at a library that was never loaded. | ||
| const knownLibraries = new Set<string>(); | ||
| for (const jsFile of program.jsSourceFiles.values()) { | ||
| const lib = jsFile.esmExports.$lib; | ||
| if (typeof lib?.name !== "string") { | ||
| continue; | ||
| } | ||
| knownLibraries.add(lib.name); | ||
|
|
||
| const linter = jsFile.esmExports.$linter; | ||
| if (linter === undefined) { | ||
| continue; | ||
| } | ||
| linters.push({ | ||
| libName: lib.name, | ||
| linter: resolveLinterDefinition(lib.name, linter), | ||
| node: jsFile, | ||
| isProject: program.getSourceFileLocationContext(jsFile.file).type === "project", | ||
| }); | ||
| } | ||
| return { linters, knownLibraries }; | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,151 @@ | ||
| import { expectDiagnosticEmpty, expectDiagnostics, mockFile } from "@typespec/compiler/testing"; | ||
| import { describe, it } from "vitest"; | ||
| import { Tester } from "./test-host.js"; | ||
|
|
||
| function libFile(name: string, linter?: unknown) { | ||
| return mockFile.js({ | ||
| $lib: { name }, | ||
| ...(linter === undefined ? {} : { $linter: linter }), | ||
| }); | ||
| } | ||
|
|
||
| const casingRule = { | ||
| name: "casing", | ||
| severity: "warning", | ||
| description: "casing", | ||
| messages: { default: "casing" }, | ||
| create: () => ({}), | ||
| }; | ||
|
|
||
| async function diagnoseLib(linter: unknown, extraFiles: Record<string, any> = {}) { | ||
| const imports = ["./mylib.js", ...Object.keys(extraFiles)] | ||
| .map((x) => `import "${x}";`) | ||
| .join("\n"); | ||
| return Tester.files({ | ||
| "./mylib.js": libFile("@test/mylib", linter), | ||
| ...extraFiles, | ||
| }).diagnose(imports); | ||
| } | ||
|
|
||
| describe("validate rulesets", () => { | ||
| it("emits no diagnostic when a ruleset references a rule of its own library", async () => { | ||
| const diagnostics = await diagnoseLib({ | ||
| rules: [casingRule], | ||
| ruleSets: { recommended: { enable: { "@test/mylib/casing": true } } }, | ||
| }); | ||
| expectDiagnosticEmpty(diagnostics); | ||
| }); | ||
|
|
||
| it("emits a diagnostic when a ruleset enables a rule that does not exist", async () => { | ||
| const diagnostics = await diagnoseLib({ | ||
| rules: [casingRule], | ||
| ruleSets: { recommended: { enable: { "@test/mylib/removed": true } } }, | ||
| }); | ||
| expectDiagnostics(diagnostics, { | ||
| code: "@typespec/library-linter/unknown-rule", | ||
| severity: "warning", | ||
| message: | ||
| "Rule 'removed' referenced by ruleset '@test/mylib/recommended' is not defined in library '@test/mylib'.", | ||
| }); | ||
| }); | ||
|
|
||
| it("emits a diagnostic when a ruleset disables a rule that does not exist", async () => { | ||
| const diagnostics = await diagnoseLib({ | ||
| rules: [casingRule], | ||
| ruleSets: { recommended: { disable: { "@test/mylib/removed": "gone" } } }, | ||
| }); | ||
| expectDiagnostics(diagnostics, { | ||
| code: "@typespec/library-linter/unknown-rule", | ||
| message: | ||
| "Rule 'removed' referenced by ruleset '@test/mylib/recommended' is not defined in library '@test/mylib'.", | ||
| }); | ||
| }); | ||
|
|
||
| it("emits a diagnostic when a ruleset extends a ruleset that does not exist", async () => { | ||
| const diagnostics = await diagnoseLib({ | ||
| rules: [casingRule], | ||
| ruleSets: { recommended: { extends: ["@test/mylib/missing"] } }, | ||
| }); | ||
| expectDiagnostics(diagnostics, { | ||
| code: "@typespec/library-linter/unknown-rule-set", | ||
| message: | ||
| "Ruleset 'missing' referenced by ruleset '@test/mylib/recommended' is not defined in library '@test/mylib'.", | ||
| }); | ||
| }); | ||
|
|
||
| it("emits a diagnostic when a reference is not in the '<library-name>/<name>' format", async () => { | ||
| const diagnostics = await diagnoseLib({ | ||
| rules: [casingRule], | ||
| ruleSets: { recommended: { enable: { removed: true } } }, | ||
| }); | ||
| expectDiagnostics(diagnostics, { | ||
| code: "@typespec/library-linter/invalid-rule-reference", | ||
| message: `Reference 'removed' in ruleset '@test/mylib/recommended' is invalid. It must be in the format "<library-name>/<name>".`, | ||
| }); | ||
| }); | ||
|
|
||
| it("resolves references to the auto generated `all` ruleset", async () => { | ||
| const diagnostics = await diagnoseLib({ | ||
| rules: [casingRule], | ||
| ruleSets: { recommended: { extends: ["@test/mylib/all"] } }, | ||
| }); | ||
| expectDiagnosticEmpty(diagnostics); | ||
| }); | ||
|
|
||
| it("resolves references to rules of another library in the compilation", async () => { | ||
| const diagnostics = await diagnoseLib( | ||
| { rules: [], ruleSets: { recommended: { enable: { "@test/other/casing": true } } } }, | ||
| { "./other.js": libFile("@test/other", { rules: [casingRule] }) }, | ||
| ); | ||
| expectDiagnosticEmpty(diagnostics); | ||
| }); | ||
|
|
||
| it("emits a diagnostic for a missing rule of another library in the compilation", async () => { | ||
| const diagnostics = await diagnoseLib( | ||
| { rules: [], ruleSets: { recommended: { enable: { "@test/other/removed": true } } } }, | ||
| { "./other.js": libFile("@test/other", { rules: [casingRule] }) }, | ||
| ); | ||
| expectDiagnostics(diagnostics, { | ||
| code: "@typespec/library-linter/unknown-rule", | ||
| message: | ||
| "Rule 'removed' referenced by ruleset '@test/mylib/recommended' is not defined in library '@test/other'.", | ||
| }); | ||
| }); | ||
|
|
||
| it("emits a diagnostic for a rule of a library in the compilation that defines no linter", async () => { | ||
| const diagnostics = await diagnoseLib( | ||
| { rules: [], ruleSets: { recommended: { enable: { "@test/other/casing": true } } } }, | ||
| { "./other.js": libFile("@test/other") }, | ||
| ); | ||
| expectDiagnostics(diagnostics, { | ||
| code: "@typespec/library-linter/unknown-rule", | ||
| message: | ||
| "Rule 'casing' referenced by ruleset '@test/mylib/recommended' is not defined in library '@test/other'.", | ||
| }); | ||
| }); | ||
|
|
||
| it("validates every ruleset defined in the project being compiled", async () => { | ||
| const diagnostics = await diagnoseLib( | ||
| { rules: [casingRule] }, | ||
| { | ||
| "./other.js": libFile("@test/other", { | ||
| rules: [], | ||
| ruleSets: { recommended: { enable: { "@test/other/removed": true } } }, | ||
| }), | ||
| }, | ||
| ); | ||
| expectDiagnostics(diagnostics, { | ||
| code: "@typespec/library-linter/unknown-rule", | ||
| message: | ||
| "Rule 'removed' referenced by ruleset '@test/other/recommended' is not defined in library '@test/other'.", | ||
| }); | ||
| }); | ||
|
|
||
| it("ignores references to a library that is not part of the compilation", async () => { | ||
| const diagnostics = await diagnoseLib({ | ||
| rules: [casingRule], | ||
| ruleSets: { recommended: { enable: { "@test/not-installed/some-rule": true } } }, | ||
| }); | ||
| expectDiagnosticEmpty(diagnostics); | ||
| }); | ||
| }); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.