From aa23f6ea99d94d5a159dbadde80d6d0876558280 Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Tue, 26 May 2026 15:17:45 -0400 Subject: [PATCH] Report unused suppressions --- ...used-suppression-diagnostics-2026-05-26.md | 12 + packages/compiler/src/core/messages.ts | 1 + packages/compiler/src/core/program.ts | 73 +++---- .../compiler/src/core/suppression-tracking.ts | 205 ++++++++++++++++++ packages/compiler/src/server/serverlib.ts | 3 + .../compiler/test/checker/values/utils.ts | 2 +- .../test/server/unused-suppression.test.ts | 20 ++ packages/compiler/test/suppression.test.ts | 180 ++++++++++----- 8 files changed, 398 insertions(+), 98 deletions(-) create mode 100644 .chronus/changes/unused-suppression-diagnostics-2026-05-26.md create mode 100644 packages/compiler/src/core/suppression-tracking.ts create mode 100644 packages/compiler/test/server/unused-suppression.test.ts diff --git a/.chronus/changes/unused-suppression-diagnostics-2026-05-26.md b/.chronus/changes/unused-suppression-diagnostics-2026-05-26.md new file mode 100644 index 00000000000..56d82f6fd26 --- /dev/null +++ b/.chronus/changes/unused-suppression-diagnostics-2026-05-26.md @@ -0,0 +1,12 @@ +--- +changeKind: feature +packages: + - "@typespec/compiler" +--- + +Dim unused `#suppress` directives for available compiler and library diagnostics in editor scenarios. + +```typespec +#suppress "deprecated" "old suppression" +model Widget {} +``` diff --git a/packages/compiler/src/core/messages.ts b/packages/compiler/src/core/messages.ts index 457ae774915..67edaf9dd8d 100644 --- a/packages/compiler/src/core/messages.ts +++ b/packages/compiler/src/core/messages.ts @@ -1131,4 +1131,5 @@ const diagnostics = { } as const; export type CompilerDiagnostics = TypeOfDiagnostics; +export const compilerDiagnosticCodes = new Set(Object.keys(diagnostics)); export const { createDiagnostic, reportDiagnostic } = createDiagnosticCreator(diagnostics); diff --git a/packages/compiler/src/core/program.ts b/packages/compiler/src/core/program.ts index 19699c1d779..4fdb41ef7e0 100644 --- a/packages/compiler/src/core/program.ts +++ b/packages/compiler/src/core/program.ts @@ -41,11 +41,13 @@ import { } from "./source-loader.js"; import { createStateAccessors } from "./state-accessors.js"; import { ComplexityStats, RuntimeStats, Stats } from "./stats.js"; +import { + createSuppressionTracker, + findDirectiveSuppressingOnNode, +} from "./suppression-tracking.js"; import { CompilerHost, Diagnostic, - Directive, - DirectiveExpressionNode, EmitContext, EmitterFunc, Entity, @@ -64,7 +66,6 @@ import { Sym, SymbolFlags, SymbolTable, - SyntaxKind, TemplateInstanceTarget, Tracer, Type, @@ -118,6 +119,8 @@ export interface Program { /** @internal */ reportDuplicateSymbols(symbols: SymbolTable | undefined): void; + /** @internal */ + reportUnusedSuppressions(): void; getGlobalNamespaceType(): Namespace; @@ -200,6 +203,7 @@ export async function compile( } emitStats.total = timer.end(); program.stats.runtime.emit = emitStats; + program.reportUnusedSuppressions(); return program; } @@ -218,9 +222,12 @@ async function createProgram( const emitters: EmitterRef[] = []; const requireImports = new Map(); const complexityStats: ComplexityStats = {} as any; - let sourceResolution: SourceResolution; + let sourceResolution: SourceResolution = undefined!; let error = false; let continueToNextStage = true; + const suppressionTracker: { + current?: ReturnType; + } = {}; const logger = createLogger({ sink: host.logSink }); const tracer = createTracer(logger, { filter: options.trace }); @@ -250,6 +257,7 @@ async function createProgram( reportDiagnostic, reportDiagnostics, reportDuplicateSymbols, + reportUnusedSuppressions, hasError() { return error; }, @@ -295,6 +303,11 @@ async function createProgram( // let GC reclaim old program, we do not reuse it beyond this point. oldProgram = undefined; + suppressionTracker.current = createSuppressionTracker({ + addDiagnostic: (diagnostic) => diagnostics.push(diagnostic), + sourceResolution, + }); + const resolver = (program.resolver = createResolver(program)); runtimeStats.resolver = perf.time(() => resolver.resolveProgram()); @@ -330,6 +343,10 @@ async function createProgram( runtimeStats.linter = lintResult.stats.runtime; program.reportDiagnostics(lintResult.diagnostics); + if (emit.length === 0) { + reportUnusedSuppressions(); + } + return { program, shouldAbort: false }; /** @@ -830,56 +847,16 @@ async function createProgram( return false; } else { + suppressionTracker.current?.markUsed(suppressing.node); return true; } } return false; } - function findDirectiveSuppressingOnNode(code: string, node: Node): Directive | undefined { - let current: Node | undefined = node; - do { - if (current.directives) { - const directive = findDirectiveSuppressingCode(code, current.directives); - if (directive) { - return directive; - } - } - } while ((current = current.parent)); - return undefined; - } - - /** - * Returns the directive node that is suppressing this code. - * @param code Code to check for suppression. - * @param directives List of directives. - * @returns Directive suppressing this code if found, `undefined` otherwise - */ - function findDirectiveSuppressingCode( - code: string, - directives: readonly DirectiveExpressionNode[], - ): Directive | undefined { - for (const directive of directives.map((x) => parseDirective(x))) { - if (directive.name === "suppress") { - if (directive.code === code) { - return directive; - } - } - } - return undefined; - } - - function parseDirective(node: DirectiveExpressionNode): Directive { - const args = node.arguments.map((x) => { - return x.kind === SyntaxKind.Identifier ? x.sv : x.value; - }); - switch (node.target.sv) { - case "suppress": - return { name: "suppress", code: args[0], message: args[1], node }; - case "deprecated": - return { name: "deprecated", message: args[0], node }; - default: - throw new Error("Unexpected directive name."); + function reportUnusedSuppressions(): void { + if (program.compilerOptions.designTimeBuild) { + suppressionTracker.current?.reportUnusedSuppressions(); } } diff --git a/packages/compiler/src/core/suppression-tracking.ts b/packages/compiler/src/core/suppression-tracking.ts new file mode 100644 index 00000000000..8b8aeb118a2 --- /dev/null +++ b/packages/compiler/src/core/suppression-tracking.ts @@ -0,0 +1,205 @@ +import { defineCodeFix, getSourceLocation } from "./diagnostics.js"; +import { builtInLinterLibraryName } from "./linter.js"; +import { compilerDiagnosticCodes } from "./messages.js"; +import { visitChildren } from "./parser.js"; +import { SourceResolution } from "./source-loader.js"; +import { + CodeFix, + Diagnostic, + Directive, + DirectiveExpressionNode, + Node, + SuppressDirective, + SyntaxKind, +} from "./types.js"; + +interface SuppressionRecord { + directive: SuppressDirective; + used: boolean; +} + +interface SuppressionTracker { + markUsed(directiveNode: DirectiveExpressionNode): void; + reportUnusedSuppressions(): void; +} + +export function createSuppressionTracker({ + addDiagnostic, + sourceResolution, +}: { + addDiagnostic: (diagnostic: Diagnostic) => void; + sourceResolution: SourceResolution; +}): SuppressionTracker { + const suppressions = collectSuppressions(sourceResolution); + let unusedSuppressionsReported = false; + + return { + markUsed(directiveNode) { + const suppression = suppressions.get(directiveNode); + if (suppression) { + suppression.used = true; + } + }, + reportUnusedSuppressions() { + if (unusedSuppressionsReported) { + return; + } + unusedSuppressionsReported = true; + + for (const suppression of suppressions.values()) { + if (suppression.used) { + continue; + } + + const availability = getSuppressionSourceAvailability( + suppression.directive.code, + sourceResolution, + ); + if (availability === "unavailable") { + continue; + } + + addDiagnostic({ + code: "unused-suppression", + severity: "warning", + message: `Suppression for "${suppression.directive.code}" is unused.`, + target: suppression.directive.node, + codefixes: [createRemoveUnusedSuppressionCodeFix(suppression.directive.node)], + }); + } + }, + }; +} + +function collectSuppressions( + sourceResolution: SourceResolution, +): Map { + const suppressions = new Map(); + for (const script of sourceResolution.sourceFiles.values()) { + if (sourceResolution.locationContexts.get(script.file)?.type !== "project") { + continue; + } + + visit(script); + } + + return suppressions; + + function visit(node: Node) { + for (const directiveNode of node.directives ?? []) { + const directive = parseDirective(directiveNode); + if (directive?.name === "suppress") { + suppressions.set(directive.node, { directive, used: false }); + } + } + + visitChildren(node, visit); + } +} + +function getSuppressionSourceAvailability( + code: string, + sourceResolution: SourceResolution, +): "available" | "unavailable" { + if ( + compilerDiagnosticCodes.has(code) || + matchesDiagnosticSource(code, builtInLinterLibraryName) + ) { + return "available"; + } + + for (const libraryName of sourceResolution.loadedLibraries.keys()) { + if (matchesDiagnosticSource(code, libraryName)) { + return "available"; + } + } + + return "unavailable"; +} + +function matchesDiagnosticSource(code: string, source: string): boolean { + return code.startsWith(`${source}/`); +} + +export function findDirectiveSuppressingOnNode(code: string, node: Node): Directive | undefined { + let current: Node | undefined = node; + do { + if (current.directives) { + const directive = findDirectiveSuppressingCode(code, current.directives); + if (directive) { + return directive; + } + } + } while ((current = current.parent)); + return undefined; +} + +/** + * Returns the directive node that is suppressing this code. + * @param code Code to check for suppression. + * @param directives List of directives. + * @returns Directive suppressing this code if found, `undefined` otherwise + */ +export function findDirectiveSuppressingCode( + code: string, + directives: readonly DirectiveExpressionNode[], +): Directive | undefined { + for (const directiveNode of directives) { + const directive = parseDirective(directiveNode); + if (directive?.name === "suppress") { + if (directive.code === code) { + return directive; + } + } + } + return undefined; +} + +export function parseDirective(node: DirectiveExpressionNode): Directive | undefined { + const args = node.arguments.map((x) => { + return x.kind === SyntaxKind.Identifier ? x.sv : x.value; + }); + switch (node.target.sv) { + case "suppress": + if (typeof args[0] !== "string") { + return undefined; + } + return { name: "suppress", code: args[0], message: args[1] ?? "", node }; + case "deprecated": + if (typeof args[0] !== "string") { + return undefined; + } + return { name: "deprecated", message: args[0], node }; + default: + return undefined; + } +} + +export function createRemoveUnusedSuppressionCodeFix(node: DirectiveExpressionNode): CodeFix { + return defineCodeFix({ + id: "remove-unused-suppression", + label: "Remove unused suppression", + fix: (context) => { + const location = getSourceLocation(node); + const text = location.file.text; + const lineStart = text.lastIndexOf("\n", location.pos - 1) + 1; + const textBeforeDirective = text.slice(lineStart, location.pos); + + if (textBeforeDirective.trim() !== "") { + return context.replaceText(location, ""); + } + + let end = location.end; + while (end < text.length && (text[end] === " " || text[end] === "\t")) { + end++; + } + if (text[end] === "\r" && text[end + 1] === "\n") { + end += 2; + } else if (text[end] === "\n" || text[end] === "\r") { + end++; + } + + return context.replaceText({ ...location, pos: lineStart, end }, ""); + }, + }); +} diff --git a/packages/compiler/src/server/serverlib.ts b/packages/compiler/src/server/serverlib.ts index b23b6e29913..23c208f1afa 100644 --- a/packages/compiler/src/server/serverlib.ts +++ b/packages/compiler/src/server/serverlib.ts @@ -774,6 +774,9 @@ export function createServer( // if the unused template parameter is not configured by user explicitly, report it as hint by default diagnostic.severity = DiagnosticSeverity.Hint; } + } else if (each.code === "unused-suppression") { + diagnostic.tags = [DiagnosticTag.Unnecessary]; + diagnostic.severity = DiagnosticSeverity.Hint; } diagnostic.data = { id: diagnosticIdCounter++, diff --git a/packages/compiler/test/checker/values/utils.ts b/packages/compiler/test/checker/values/utils.ts index 2f5a690b89c..87f1d0a8fd5 100644 --- a/packages/compiler/test/checker/values/utils.ts +++ b/packages/compiler/test/checker/values/utils.ts @@ -71,7 +71,7 @@ export async function compileAndDiagnoseValueOrType( import "./collect.js"; extern dec collect(target, value: ${constraint}); - ${disableDeprecatedSuppression ? "" : `#suppress "deprecated" "for testing"`} + ${disableDeprecatedSuppression === false ? `#suppress "deprecated" "for testing"` : ""} @collect(${code}) model ${t.model("Test")} {} ${other ?? ""} diff --git a/packages/compiler/test/server/unused-suppression.test.ts b/packages/compiler/test/server/unused-suppression.test.ts new file mode 100644 index 00000000000..b6788f06579 --- /dev/null +++ b/packages/compiler/test/server/unused-suppression.test.ts @@ -0,0 +1,20 @@ +import { strictEqual } from "assert"; +import { it } from "vitest"; +import { DiagnosticSeverity, DiagnosticTag } from "vscode-languageserver"; +import { createTestServerHost } from "../../src/testing/test-server-host.js"; + +it("hint by default", async () => { + const testHost = await createTestServerHost(); + const mainFile = testHost.addOrUpdateDocument( + "./main.tsp", + '#suppress "deprecated" "not needed anymore"\nmodel Foo {}', + ); + + await testHost.server.compile(mainFile, undefined, { mode: "full" }); + const diags = testHost.getDiagnostics("main.tsp"); + strictEqual(diags.length, 1); + strictEqual(diags[0].code, "unused-suppression"); + strictEqual(diags[0].severity, DiagnosticSeverity.Hint); + strictEqual(diags[0].tags?.[0], DiagnosticTag.Unnecessary); + strictEqual(diags[0].message, 'Suppression for "deprecated" is unused.'); +}); diff --git a/packages/compiler/test/suppression.test.ts b/packages/compiler/test/suppression.test.ts index 661c14ea4a6..c2a2cad1944 100644 --- a/packages/compiler/test/suppression.test.ts +++ b/packages/compiler/test/suppression.test.ts @@ -1,42 +1,44 @@ -import { describe, it } from "vitest"; +import { it } from "vitest"; +import { SyntaxKind } from "../src/ast/index.js"; import { navigateProgram } from "../src/core/semantic-walker.js"; +import { createRemoveUnusedSuppressionCodeFix } from "../src/core/suppression-tracking.js"; +import { expectCodeFixOnAst } from "../src/testing/code-fix-testing.js"; import { expectDiagnosticEmpty, expectDiagnostics } from "../src/testing/index.js"; import { Tester } from "./tester.js"; -describe("compiler: suppress", () => { - async function run(typespec: string) { - const { program } = await Tester.compile(typespec, { - compilerOptions: { nostdlib: true }, - }); - - navigateProgram(program, { - model: (model) => { - if (model.name === "") { - program.reportDiagnostic({ - severity: "warning", - code: "no-inline-model", - message: "Inline models are not recommended", - target: model, - }); - } - }, - modelProperty: (prop) => { - if (prop.name === "id") { - program.reportDiagnostic({ - severity: "error", - code: "no-id-property", - message: "Id properties on models are forbidden", - target: prop, - }); - } - }, - }); - - return program.diagnostics; - } - - it("emit warning diagnostics when there is no suppression", async () => { - const diagnostics = await run(` +async function run(typespec: string) { + const { program } = await Tester.compile(typespec, { + compilerOptions: { nostdlib: true }, + }); + + navigateProgram(program, { + model: (model) => { + if (model.name === "") { + program.reportDiagnostic({ + severity: "warning", + code: "no-inline-model", + message: "Inline models are not recommended", + target: model, + }); + } + }, + modelProperty: (prop) => { + if (prop.name === "id") { + program.reportDiagnostic({ + severity: "error", + code: "no-id-property", + message: "Id properties on models are forbidden", + target: prop, + }); + } + }, + }); + + return program.diagnostics; +} + +it("emit warning diagnostics when there is no suppression", async () => { + const diagnostics = await run(` model Foo { inline: { name: 123; @@ -44,11 +46,11 @@ describe("compiler: suppress", () => { } `); - expectDiagnostics(diagnostics, { code: "no-inline-model" }); - }); + expectDiagnostics(diagnostics, { code: "no-inline-model" }); +}); - it("suppress warning diagnostic on item itself", async () => { - const diagnostics = await run(` +it("suppress warning diagnostic on item itself", async () => { + const diagnostics = await run(` model Foo { #suppress "no-inline-model" "This is needed" inline: { @@ -57,11 +59,11 @@ describe("compiler: suppress", () => { } `); - expectDiagnosticEmpty(diagnostics); - }); + expectDiagnosticEmpty(diagnostics); +}); - it("suppress warning diagnostic on parent node", async () => { - const diagnostics = await run(` +it("suppress warning diagnostic on parent node", async () => { + const diagnostics = await run(` #suppress "no-inline-model" "This is needed" model Foo { inline: { @@ -69,17 +71,97 @@ describe("compiler: suppress", () => { }; } `); - expectDiagnosticEmpty(diagnostics); - }); + expectDiagnosticEmpty(diagnostics); +}); - it("error diagnostics cannot be suppressed and emit another error", async () => { - const diagnostics = await run(` +it("error diagnostics cannot be suppressed and emit another error", async () => { + const diagnostics = await run(` model Foo { #suppress "no-id-property" "This is needed" id: 123; } `); - expectDiagnostics(diagnostics, [{ code: "suppress-error" }, { code: "no-id-property" }]); - }); + expectDiagnostics(diagnostics, [{ code: "suppress-error" }, { code: "no-id-property" }]); +}); + +it("does not report unused suppression in normal compile", async () => { + const diagnostics = await Tester.diagnose( + ` + #suppress "deprecated" "not needed anymore" + model Foo {} + `, + { + compilerOptions: { nostdlib: true }, + }, + ); + + expectDiagnosticEmpty(diagnostics); +}); + +it("provides a code fix to remove an unused suppression", async () => { + await expectCodeFixOnAst( + `#┆suppress "deprecated" "not needed anymore" +model Foo {} +`, + (node) => { + if ( + node.kind !== SyntaxKind.Identifier || + node.parent?.kind !== SyntaxKind.DirectiveExpression + ) { + throw new Error("Expected a directive expression node."); + } + return createRemoveUnusedSuppressionCodeFix(node.parent); + }, + ).toChangeTo(`model Foo {} +`); +}); + +it("does not report unused suppression when diagnostic was suppressed", async () => { + const diagnostics = await Tester.diagnose( + ` + #deprecated "Old is deprecated" + model Old {} + + model Foo { + #suppress "deprecated" "intentional" + prop: Old; + } + `, + { + compilerOptions: { nostdlib: true }, + }, + ); + + expectDiagnosticEmpty(diagnostics); +}); + +it("does not report unused suppression for unavailable diagnostic source by default", async () => { + const diagnostics = await Tester.diagnose( + ` + #suppress "test-emitter/not-run" "only emitted by another tool" + model Foo {} + `, + { + compilerOptions: { nostdlib: true }, + }, + ); + + expectDiagnosticEmpty(diagnostics); +}); + +it("does not report unused suppression for errors as replacement for suppress-error", async () => { + const diagnostics = await Tester.diagnose( + ` + model Foo { + #suppress "invalid-ref" "errors cannot be suppressed" + prop: Unknown; + } + `, + { + compilerOptions: { nostdlib: true }, + }, + ); + + expectDiagnostics(diagnostics, [{ code: "suppress-error" }, { code: "invalid-ref" }]); });