diff --git a/.chronus/changes/fix181-2025-0-6-16-32-31.md b/.chronus/changes/fix181-2025-0-6-16-32-31.md new file mode 100644 index 0000000000..9d7423db75 --- /dev/null +++ b/.chronus/changes/fix181-2025-0-6-16-32-31.md @@ -0,0 +1,7 @@ +--- +changeKind: feature +packages: + - "@typespec/compiler" +--- + +Support diagnostics for unused template parameter in model template \ No newline at end of file diff --git a/.chronus/changes/fix181-2025-0-7-14-51-31.md b/.chronus/changes/fix181-2025-0-7-14-51-31.md new file mode 100644 index 0000000000..2a7bf972ca --- /dev/null +++ b/.chronus/changes/fix181-2025-0-7-14-51-31.md @@ -0,0 +1,7 @@ +--- +changeKind: internal +packages: + - "@typespec/protobuf" +--- + +Suppress unused-template-parameter diagnostic for Map model diff --git a/packages/compiler/src/core/checker.ts b/packages/compiler/src/core/checker.ts index 9f8967b663..e1ebf5d317 100644 --- a/packages/compiler/src/core/checker.ts +++ b/packages/compiler/src/core/checker.ts @@ -4,6 +4,7 @@ import { MultiKeyMap, Mutable, createRekeyableMap, isArray, mutate } from "../ut import { createSymbol, getSymNode } from "./binder.js"; import { createChangeIdentifierCodeFix } from "./compiler-code-fixes/change-identifier.codefix.js"; import { createModelToObjectValueCodeFix } from "./compiler-code-fixes/model-to-object-literal.codefix.js"; +import { removeUnusedTemplateParameterCodeFix } from "./compiler-code-fixes/remove-unused-template-parameter.codefix.js"; import { createTupleToArrayValueCodeFix } from "./compiler-code-fixes/tuple-to-array-value.codefix.js"; import { getDeprecationDetails, markDeprecated } from "./deprecation.js"; import { ProjectionError, compilerAssert, ignoreDiagnostics } from "./diagnostics.js"; @@ -1108,7 +1109,6 @@ export function createChecker(program: Program, resolver: NameResolver): Checker ); } } - return mapper ? mapper.getMappedType(type) : type; } @@ -1860,6 +1860,9 @@ export function createChecker(program: Program, resolver: NameResolver): Checker if (type === neverType) { continue; } + if (type.kind === "TemplateParameter") { + resolver.setTemplateParameterDeclarationNode(type.node); + } if (type.kind === "Union" && type.expression) { for (const [name, variant] of type.variants) { unionType.variants.set(name, variant); @@ -3551,6 +3554,24 @@ export function createChecker(program: Program, resolver: NameResolver): Checker if (mapper === undefined) { for (const templateParameter of node.templateParameters) { checkTemplateParameterDeclaration(templateParameter, undefined); + /* current we only check unused template parameter for model template. + * TODO: We should add more check for other template types in the future. + */ + if (node.kind === SyntaxKind.ModelStatement) { + if (!resolver.isUsedTemplateParameterDeclarationNode(templateParameter)) { + reportCheckerDiagnostic( + createDiagnostic({ + code: "unused-template-parameter", + format: { + parameterName: templateParameter.id.sv, + type: node.symbol.name, + }, + target: templateParameter, + codefixes: [removeUnusedTemplateParameterCodeFix(templateParameter)], + }), + ); + } + } } } } @@ -3573,7 +3594,6 @@ export function createChecker(program: Program, resolver: NameResolver): Checker // we're not instantiating this model and we've already checked it return links.declaredType as any; } - checkTemplateDeclaration(node, mapper); const decorators: DecoratorApplication[] = []; const type: Model = createType({ @@ -3664,6 +3684,8 @@ export function createChecker(program: Program, resolver: NameResolver): Checker } lateBindMemberContainer(type); lateBindMembers(type); + + checkTemplateDeclaration(node, mapper); return type; } @@ -4335,6 +4357,7 @@ export function createChecker(program: Program, resolver: NameResolver): Checker if (isType(entity)) { if (entity.kind === "TemplateParameter") { + resolver.setTemplateParameterDeclarationNode(entity.node); if (entity.constraint === undefined || entity.constraint.type !== undefined) { // means this template constraint will accept values reportCheckerDiagnostic( @@ -4582,6 +4605,13 @@ export function createChecker(program: Program, resolver: NameResolver): Checker return undefined; } + if (heritageType.kind === "Model") { + for (const arg of heritageType.templateMapper?.args ?? []) { + if ("kind" in arg && arg.kind === "TemplateParameter") { + resolver.setTemplateParameterDeclarationNode(arg.node); + } + } + } if (heritageType.kind !== "Model") { reportCheckerDiagnostic(createDiagnostic({ code: "extend-model", target: heritageRef })); return undefined; @@ -4648,6 +4678,12 @@ export function createChecker(program: Program, resolver: NameResolver): Checker return; } + for (const arg of isType.templateMapper?.args ?? []) { + if ("kind" in arg && arg.kind === "TemplateParameter") { + resolver.setTemplateParameterDeclarationNode(arg.node); + } + } + if (isType.name === "") { reportCheckerDiagnostic( createDiagnostic({ code: "is-model", messageId: "modelExpression", target: isExpr }), @@ -4666,6 +4702,9 @@ export function createChecker(program: Program, resolver: NameResolver): Checker ): [ModelProperty[], ModelIndexer | undefined] { const targetType = getTypeForNode(targetNode, mapper); + if (targetType.kind === "TemplateParameter") { + resolver.setTemplateParameterDeclarationNode(targetType.node); + } if (targetType.kind === "TemplateParameter" || isErrorType(targetType)) { return [[], undefined]; } @@ -4783,6 +4822,9 @@ export function createChecker(program: Program, resolver: NameResolver): Checker } } + if (type.type.kind === "TemplateParameter") { + resolver.setTemplateParameterDeclarationNode(type.type.node); + } type.decorators = checkDecorators(type, prop, mapper); const parentTemplate = getParentTemplateNode(prop); linkMapper(type, mapper); @@ -5220,6 +5262,12 @@ export function createChecker(program: Program, resolver: NameResolver): Checker for (const decNode of decoratorNodes) { const decorator = checkDecoratorApplication(targetType, decNode, mapper); if (decorator) { + /* check template parameters */ + for (const arg of decorator.args) { + if ("kind" in arg.value && arg.value.kind === "TemplateParameter") { + resolver.setTemplateParameterDeclarationNode(arg.value.node); + } + } decorators.unshift(decorator); } } @@ -5716,6 +5764,9 @@ export function createChecker(program: Program, resolver: NameResolver): Checker const name = variantNode.id ? variantNode.id.sv : Symbol("name"); const type = getTypeForNode(variantNode.value, mapper); + if (type.kind === "TemplateParameter") { + resolver.setTemplateParameterDeclarationNode(type.node); + } const variantType: UnionVariant = createType({ kind: "UnionVariant", name, diff --git a/packages/compiler/src/core/compiler-code-fixes/remove-unused-template-parameter.codefix.ts b/packages/compiler/src/core/compiler-code-fixes/remove-unused-template-parameter.codefix.ts new file mode 100644 index 0000000000..7d21ea46ef --- /dev/null +++ b/packages/compiler/src/core/compiler-code-fixes/remove-unused-template-parameter.codefix.ts @@ -0,0 +1,44 @@ +import { defineCodeFix, getSourceLocation } from "../diagnostics.js"; +import { TemplateParameterDeclarationNode } from "../types.js"; + +/** + * Quick fix that remove unused template parameter. + */ +export function removeUnusedTemplateParameterCodeFix(node: TemplateParameterDeclarationNode) { + return defineCodeFix({ + id: "remove-unused-template-parameter", + label: `Remove unused template parameter ${node.id.sv}`, + fix: (context) => { + let location = getSourceLocation(node); + const parent = node.parent; + if (parent) { + const length = parent.templateParameters.length; + if (length === 1) { + location = { + file: location.file, + pos: parent.templateParametersRange.pos, + end: parent.templateParametersRange.end, + }; + } else { + const index = parent.templateParameters.findIndex((param) => param === node); + if (index !== -1) { + if (index !== parent.templateParameters.length - 1) { + location = { + file: location.file, + pos: location.pos, + end: parent.templateParameters[index + 1].pos, + }; + } else { + location = { + file: location.file, + pos: parent.templateParameters[index - 1].end, + end: location.end, + }; + } + } + } + } + return context.replaceText(location, ""); + }, + }); +} diff --git a/packages/compiler/src/core/diagnostics.ts b/packages/compiler/src/core/diagnostics.ts index f234a97512..eb4033ae22 100644 --- a/packages/compiler/src/core/diagnostics.ts +++ b/packages/compiler/src/core/diagnostics.ts @@ -33,7 +33,7 @@ export type DiagnosticHandler = (diagnostic: Diagnostic) => void; export function logDiagnostics(diagnostics: readonly Diagnostic[], logger: LogSink) { for (const diagnostic of diagnostics) { logger.log({ - level: diagnostic.severity, + level: diagnostic.severity === "hint" ? "trace" : diagnostic.severity, message: diagnostic.message, code: diagnostic.code, url: diagnostic.url, @@ -52,7 +52,7 @@ export function formatDiagnostic(diagnostic: Diagnostic, options: FormatDiagnost return formatLog( { code: diagnostic.code, - level: diagnostic.severity, + level: diagnostic.severity === "hint" ? "trace" : diagnostic.severity, message: diagnostic.message, url: diagnostic.url, sourceLocation: getSourceLocation(diagnostic.target, { locateId: true }), diff --git a/packages/compiler/src/core/messages.ts b/packages/compiler/src/core/messages.ts index a0b74d2d58..b96acf73b1 100644 --- a/packages/compiler/src/core/messages.ts +++ b/packages/compiler/src/core/messages.ts @@ -479,6 +479,12 @@ const diagnostics = { default: paramMessage`Object value may only specify known properties, and '${"propertyName"}' does not exist in type '${"type"}'.`, }, }, + "unused-template-parameter": { + severity: "hint", + messages: { + default: paramMessage`Templates should use all specified parameters, and parameter '${"parameterName"}' does not exist in type '${"type"}'. Consider removing this parameter.`, + }, + }, "extends-interface": { severity: "error", messages: { diff --git a/packages/compiler/src/core/name-resolver.ts b/packages/compiler/src/core/name-resolver.ts index 3641bdb78b..64393d004d 100644 --- a/packages/compiler/src/core/name-resolver.ts +++ b/packages/compiler/src/core/name-resolver.ts @@ -124,6 +124,12 @@ export interface NameResolver { /** Return augment decorator nodes that are bound to this symbol */ getAugmentDecoratorsForSym(symbol: Sym): AugmentDecoratorStatementNode[]; + /** Check if a template parameter delare node is used or not */ + isUsedTemplateParameterDeclarationNode(node: TemplateParameterDeclarationNode): boolean; + + /** Set used template parameter */ + setTemplateParameterDeclarationNode(node: TemplateParameterDeclarationNode): void; + /** * Resolve the member expression using the given symbol as base. * This can be used to follow the name resolution for template instance which are not statically linked. @@ -180,6 +186,11 @@ export function createResolver(program: Program): NameResolver { const nullSym = createSymbol(undefined, "null", SymbolFlags.None); const augmentDecoratorsForSym = new Map(); + /** + * Tracking the template parameters that are used. + */ + const usedTemplateParameterDeclarationNodes = new Set(); + return { symbols: { global: globalNamespaceSym, null: nullSym }, resolveProgram() { @@ -222,6 +233,8 @@ export function createResolver(program: Program): NameResolver { resolveTypeReference, getAugmentDecoratorsForSym, + isUsedTemplateParameterDeclarationNode, + setTemplateParameterDeclarationNode, }; function getAugmentDecoratorsForSym(sym: Sym) { @@ -233,6 +246,15 @@ export function createResolver(program: Program): NameResolver { return mergedSymbols.get(sym) || sym; } + function isUsedTemplateParameterDeclarationNode(node: TemplateParameterDeclarationNode): boolean { + if (!node) return false; + return usedTemplateParameterDeclarationNodes.has(node); + } + + function setTemplateParameterDeclarationNode(node: TemplateParameterDeclarationNode): void { + if (!node) return; + usedTemplateParameterDeclarationNodes.add(node); + } /** * @internal */ diff --git a/packages/compiler/src/core/type-relation-checker.ts b/packages/compiler/src/core/type-relation-checker.ts index 3e21da2023..9ffd1e7c38 100644 --- a/packages/compiler/src/core/type-relation-checker.ts +++ b/packages/compiler/src/core/type-relation-checker.ts @@ -59,6 +59,11 @@ export interface TypeRelation { isReflectionType(type: Type): type is Model & { name: ReflectionTypeName }; areScalarsRelated(source: Scalar, target: Scalar): boolean; + isUnusedTemplateParameter( + source: Value, + target: Type, + diagnosticTarget: Entity | Node, + ): [boolean, readonly Diagnostic[]]; } enum Related { @@ -74,7 +79,8 @@ interface TypeRelationError { | "missing-index" | "property-required" | "missing-property" - | "unexpected-property"; + | "unexpected-property" + | "unused-template-parameter"; message: string; children: readonly TypeRelationError[]; target: Entity | Node; @@ -110,6 +116,7 @@ export function createTypeRelationChecker(program: Program, checker: Checker): T isValueOfType, isReflectionType, areScalarsRelated, + isUnusedTemplateParameter, }; /** @@ -147,6 +154,17 @@ export function createTypeRelationChecker(program: Program, checker: Checker): T return false; } + function isUnusedTemplateParameter( + source: Value, + target: Type, + diagnosticTarget: Entity | Node, + ): [boolean, readonly Diagnostic[]] { + const errors: TypeRelationError[] = [ + createUnusedTemplateParameterDiagnostic(source, target, diagnosticTarget), + ]; + return [true, convertErrorsToDiagnostics(errors, diagnosticTarget)]; + } + function convertErrorsToDiagnostics( errors: readonly TypeRelationError[], diagnosticBase: Entity | Node, @@ -1024,4 +1042,19 @@ function createUnassignableDiagnostic( details, }); } + +function createUnusedTemplateParameterDiagnostic( + source: Value, + target: Type, + diagnosticTarget: Entity | Node, +): TypeRelationError { + return createTypeRelationError({ + code: "unused-template-parameter", + format: { + parameterName: getEntityName(source), + type: getEntityName(target), + }, + diagnosticTarget, + }); +} // #endregion diff --git a/packages/compiler/src/core/types.ts b/packages/compiler/src/core/types.ts index ec31817968..2b58e36b81 100644 --- a/packages/compiler/src/core/types.ts +++ b/packages/compiler/src/core/types.ts @@ -2300,7 +2300,7 @@ export interface TemplateInstanceTarget { export type DiagnosticTarget = TypeSpecDiagnosticTarget | SourceLocation; -export type DiagnosticSeverity = "error" | "warning"; +export type DiagnosticSeverity = "error" | "warning" | "hint"; export interface Diagnostic { code: string; @@ -2529,8 +2529,9 @@ export interface DiagnosticDefinition { * Diagnostic severity. * - `warning` - Suppressable, should be used to represent potential issues but not blocking. * - `error` - Non-suppressable, should be used to represent failure to move forward. + * - `hint` - Suppressable, should be used to represent suggestions. */ - readonly severity: "warning" | "error"; + readonly severity: "warning" | "error" | "hint"; /** Messages that can be reported with the diagnostic. */ readonly messages: M; /** Short description of the diagnostic */ diff --git a/packages/compiler/src/server/diagnostics.ts b/packages/compiler/src/server/diagnostics.ts index f9dbe4718a..5c5f687676 100644 --- a/packages/compiler/src/server/diagnostics.ts +++ b/packages/compiler/src/server/diagnostics.ts @@ -141,11 +141,13 @@ function getVSLocationWithTypeInfo( }; } -function convertSeverity(severity: "warning" | "error"): DiagnosticSeverity { +function convertSeverity(severity: "warning" | "error" | "hint"): DiagnosticSeverity { switch (severity) { case "warning": return DiagnosticSeverity.Warning; case "error": return DiagnosticSeverity.Error; + case "hint": + return DiagnosticSeverity.Hint; } } diff --git a/packages/compiler/src/server/serverlib.ts b/packages/compiler/src/server/serverlib.ts index eb484b0da8..5de41ceb5e 100644 --- a/packages/compiler/src/server/serverlib.ts +++ b/packages/compiler/src/server/serverlib.ts @@ -317,7 +317,7 @@ export function createServer(host: ServerHost): Server { if (!validationResult.valid) { for (const diag of validationResult.diagnostics) { log({ - level: diag.severity, + level: diag.severity === "hint" ? "trace" : diag.severity, message: diag.message, detail: { code: diag.code, @@ -491,6 +491,13 @@ export function createServer(host: ServerHost): Server { if (each.code === "deprecated") { diagnostic.tags = [DiagnosticTag.Deprecated]; } + if (each.severity === "hint") { + if (diagnostic.tags) { + diagnostic.tags.push(DiagnosticTag.Unnecessary); + } else { + diagnostic.tags = [DiagnosticTag.Unnecessary]; + } + } diagnostic.data = { id: diagnosticIdCounter++ }; const diagnostics = diagnosticMap.get(diagDocument); compilerAssert( diff --git a/packages/compiler/test/checker/unused-template-parameter.test.ts b/packages/compiler/test/checker/unused-template-parameter.test.ts new file mode 100644 index 0000000000..4af38945c1 --- /dev/null +++ b/packages/compiler/test/checker/unused-template-parameter.test.ts @@ -0,0 +1,133 @@ +import { strictEqual } from "assert"; +import { beforeEach, describe, it } from "vitest"; +import { createTestHost } from "../../src/testing/test-host.js"; +import { TestHost } from "../../src/testing/types.js"; + +describe("compiler: unused template parameter in model template", () => { + let testHost: TestHost; + + beforeEach(async () => { + testHost = await createTestHost(); + }); + + it("report unused template parameter", async () => { + testHost.addTypeSpecFile( + "main.tsp", + ` + model A { + id: string; + } + `, + ); + const diagnostics = await testHost.diagnose("main.tsp"); + strictEqual(diagnostics.length, 1); + strictEqual(diagnostics[0].code, "unused-template-parameter"); + strictEqual( + diagnostics[0].message, + "Template may only specify used parameter, and 'T' does not exist in type 'A'.", + ); + }); + + it("no unused template parameter diagnose when the template parameter used in spread property", async () => { + testHost.addTypeSpecFile( + "main.tsp", + ` + model A { + ...T; + } + `, + ); + const diagnostics = await testHost.diagnose("main.tsp"); + strictEqual(diagnostics.length, 0); + }); + + it("no unused template parameter diagnose when the template parameter used in property", async () => { + testHost.addTypeSpecFile( + "main.tsp", + ` + model A { + prop:T; + } + `, + ); + const diagnostics = await testHost.diagnose("main.tsp"); + strictEqual(diagnostics.length, 0); + }); + + it("no unused template parameter diagnose when the template parameter used in property whose type is Union", async () => { + testHost.addTypeSpecFile( + "main.tsp", + ` + model A { + unionProp: T | string; + } + `, + ); + const diagnostics = await testHost.diagnose("main.tsp"); + strictEqual(diagnostics.length, 0); + }); + + it("no unused template parameter diagnose when the template parameter used in decorator", async () => { + testHost.addTypeSpecFile( + "main.tsp", + ` + @friendlyName(NameTemplate, T) + model A< + T extends Reflection.Model, + NameTemplate extends valueof string = "CreateOrUpdate{name}" + > { + ...T; + id: string; + } + `, + ); + const diagnostics = await testHost.diagnose("main.tsp"); + strictEqual(diagnostics.length, 0); + }); + + it("no unused template parameter diagnose when the template parameter used in base Model", async () => { + testHost.addTypeSpecFile( + "main.tsp", + ` + model A { + prop:T; + } + model IsModel is A; + `, + ); + const diagnostics = await testHost.diagnose("main.tsp"); + strictEqual(diagnostics.length, 0); + }); + + it("no unused template parameter diagnose when the template parameter used extended Model", async () => { + testHost.addTypeSpecFile( + "main.tsp", + ` + model A { + prop:T; + } + model ExtendModel extends A { + } + `, + ); + const diagnostics = await testHost.diagnose("main.tsp"); + strictEqual(diagnostics.length, 0); + }); + + it("no unused template parameter diagnose when the template parameter in typeof expression", async () => { + testHost.addTypeSpecFile( + "main.tsp", + ` + model A { + prop:T; + } + model ModelWithTypeOfExpression + is A { + contentType: typeof ContentType; + } + `, + ); + const diagnostics = await testHost.diagnose("main.tsp"); + strictEqual(diagnostics.length, 0); + }); +});