From d635570371792df33b0e24f892156613e2ede65e Mon Sep 17 00:00:00 2001 From: Wei Hu Date: Wed, 17 Jul 2024 17:37:50 +0800 Subject: [PATCH] Report diagnostics on `@clientName` conflicts (#1119) Resolves https://github.com/Azure/typespec-azure/issues/1096 --------- Co-authored-by: Timothee Guerin --- ...-client-name-conflicts-2024-6-5-13-7-58.md | 7 + .../src/decorators.ts | 28 +- .../src/index.ts | 1 + .../src/internal-utils.ts | 6 +- .../typespec-client-generator-core/src/lib.ts | 7 + .../src/validate.ts | 160 +++++++- .../test/decorators.test.ts | 354 ++++++++++++++++++ 7 files changed, 547 insertions(+), 16 deletions(-) create mode 100644 .chronus/changes/throw-client-name-conflicts-2024-6-5-13-7-58.md diff --git a/.chronus/changes/throw-client-name-conflicts-2024-6-5-13-7-58.md b/.chronus/changes/throw-client-name-conflicts-2024-6-5-13-7-58.md new file mode 100644 index 0000000000..02e328b216 --- /dev/null +++ b/.chronus/changes/throw-client-name-conflicts-2024-6-5-13-7-58.md @@ -0,0 +1,7 @@ +--- +changeKind: feature +packages: + - "@azure-tools/typespec-client-generator-core" +--- + +Report diagnostics on `@clientName` conflicts \ No newline at end of file diff --git a/packages/typespec-client-generator-core/src/decorators.ts b/packages/typespec-client-generator-core/src/decorators.ts index 88e97a05fd..4735775550 100644 --- a/packages/typespec-client-generator-core/src/decorators.ts +++ b/packages/typespec-client-generator-core/src/decorators.ts @@ -41,19 +41,29 @@ import { SdkServiceOperation, UsageFlags, } from "./interfaces.js"; -import { TCGCContext, parseEmitterName } from "./internal-utils.js"; +import { AllScopes, TCGCContext, clientNameKey, parseEmitterName } from "./internal-utils.js"; import { createStateSymbol, reportDiagnostic } from "./lib.js"; import { getSdkPackage } from "./package.js"; import { getLibraryName } from "./public-utils.js"; import { getSdkEnum, getSdkModel, getSdkUnion } from "./types.js"; export const namespace = "Azure.ClientGenerator.Core"; -const AllScopes = Symbol.for("@azure-core/typespec-client-generator-core/all-scopes"); -function getScopedDecoratorData(context: TCGCContext, key: symbol, target: Type): any { +function getScopedDecoratorData( + context: TCGCContext, + key: symbol, + target: Type, + languageScope?: string | typeof AllScopes +): any { const retval: Record = context.program.stateMap(key).get(target); if (retval === undefined) return retval; - if (Object.keys(retval).includes(context.emitterName)) return retval[context.emitterName]; + if (languageScope === AllScopes) { + return retval[languageScope]; + } + if (languageScope === undefined || typeof languageScope === "string") { + const scope = languageScope ?? context.emitterName; + if (Object.keys(retval).includes(scope)) return retval[scope]; + } return retval[AllScopes]; // in this case it applies to all languages } @@ -957,8 +967,6 @@ export function shouldFlattenProperty(context: TCGCContext, target: ModelPropert return getScopedDecoratorData(context, flattenPropertyKey, target) ?? false; } -const clientNameKey = createStateSymbol("clientName"); - export function $clientName( context: DecoratorContext, entity: Type, @@ -995,6 +1003,10 @@ export function $clientName( setScopedDecoratorData(context, $clientName, clientNameKey, entity, value, scope); } -export function getClientNameOverride(context: TCGCContext, entity: Type): string | undefined { - return getScopedDecoratorData(context, clientNameKey, entity); +export function getClientNameOverride( + context: TCGCContext, + entity: Type, + languageScope?: string | typeof AllScopes +): string | undefined { + return getScopedDecoratorData(context, clientNameKey, entity, languageScope); } diff --git a/packages/typespec-client-generator-core/src/index.ts b/packages/typespec-client-generator-core/src/index.ts index 15cfd85478..132e5c11c2 100644 --- a/packages/typespec-client-generator-core/src/index.ts +++ b/packages/typespec-client-generator-core/src/index.ts @@ -3,3 +3,4 @@ export * from "./interfaces.js"; export * from "./lib.js"; export * from "./public-utils.js"; export * from "./types.js"; +export { $onValidate } from "./validate.js"; diff --git a/packages/typespec-client-generator-core/src/internal-utils.ts b/packages/typespec-client-generator-core/src/internal-utils.ts index 42fece7de8..ed8610ed49 100644 --- a/packages/typespec-client-generator-core/src/internal-utils.ts +++ b/packages/typespec-client-generator-core/src/internal-utils.ts @@ -38,7 +38,7 @@ import { SdkType, SdkUnionType, } from "./interfaces.js"; -import { createDiagnostic } from "./lib.js"; +import { createDiagnostic, createStateSymbol } from "./lib.js"; import { getCrossLanguageDefinitionId, getDefaultApiVersion, @@ -47,6 +47,10 @@ import { isApiVersion, } from "./public-utils.js"; +export const AllScopes = Symbol.for("@azure-core/typespec-client-generator-core/all-scopes"); + +export const clientNameKey = createStateSymbol("clientName"); + /** * * @param emitterName Full emitter name diff --git a/packages/typespec-client-generator-core/src/lib.ts b/packages/typespec-client-generator-core/src/lib.ts index bcf8467a9d..8d39e92393 100644 --- a/packages/typespec-client-generator-core/src/lib.ts +++ b/packages/typespec-client-generator-core/src/lib.ts @@ -159,6 +159,13 @@ export const $lib = createTypeSpecLibrary({ default: `Cannot pass an empty value to the @clientName decorator`, }, }, + "duplicate-client-name": { + severity: "error", + messages: { + default: paramMessage`Client name: "${"name"}" is duplicated in language scope: "${"scope"}"`, + nonDecorator: paramMessage`Client name: "${"name"}" is defined somewhere causing nameing conflicts in language scope: "${"scope"}"`, + }, + }, }, }); diff --git a/packages/typespec-client-generator-core/src/validate.ts b/packages/typespec-client-generator-core/src/validate.ts index 833c7fcdbb..64489314c6 100644 --- a/packages/typespec-client-generator-core/src/validate.ts +++ b/packages/typespec-client-generator-core/src/validate.ts @@ -1,12 +1,158 @@ -import { Program } from "@typespec/compiler"; -import { createTCGCContext } from "./internal-utils.js"; -import { getAllModelsWithDiagnostics } from "./types.js"; +import { + AugmentDecoratorStatementNode, + DecoratorExpressionNode, + Enum, + EnumMember, + Interface, + Model, + ModelProperty, + Namespace, + Operation, + Program, + Scalar, + SyntaxKind, + Type, + Union, + UnionVariant, +} from "@typespec/compiler"; +import { DuplicateTracker } from "@typespec/compiler/utils"; +import { getClientNameOverride } from "./decorators.js"; +import { AllScopes, clientNameKey, createTCGCContext, TCGCContext } from "./internal-utils.js"; +import { reportDiagnostic } from "./lib.js"; export function $onValidate(program: Program) { - // Pass along any diagnostics that might be returned from the HTTP library const tcgcContext = createTCGCContext(program); - const [_, diagnostics] = getAllModelsWithDiagnostics(tcgcContext); - if (diagnostics.length > 0) { - program.reportDiagnostics(diagnostics); + const languageScopes = getDefinedLanguageScopes(program); + for (const scope of languageScopes) { + validateClientNamesPerNamespace(tcgcContext, scope, program.getGlobalNamespaceType()); + } +} + +function getDefinedLanguageScopes(program: Program): Set { + const languageScopes = new Set(); + for (const value of program.stateMap(clientNameKey).values()) { + if (value[AllScopes]) { + languageScopes.add(AllScopes); + } + for (const languageScope of Object.keys(value)) { + languageScopes.add(languageScope); + } + } + return languageScopes; +} + +function validateClientNamesPerNamespace( + tcgcContext: TCGCContext, + scope: string | typeof AllScopes, + namespace: Namespace +) { + // Check for duplicate client names for models, enums, and unions + validateClientNamesCore(tcgcContext, scope, [ + ...namespace.models.values(), + ...namespace.enums.values(), + ...namespace.unions.values(), + ]); + + // Check for duplicate client names for operations + validateClientNamesCore(tcgcContext, scope, namespace.operations.values()); + + // Check for duplicate client names for interfaces + validateClientNamesCore(tcgcContext, scope, namespace.interfaces.values()); + + // Check for duplicate client names for scalars + validateClientNamesCore(tcgcContext, scope, namespace.scalars.values()); + + // Check for duplicate client names for namespaces + validateClientNamesCore(tcgcContext, scope, namespace.namespaces.values()); + + // Check for duplicate client names for model properties + for (const model of namespace.models.values()) { + validateClientNamesCore(tcgcContext, scope, model.properties.values()); + } + + // Check for duplicate client names for enum members + for (const item of namespace.enums.values()) { + validateClientNamesCore(tcgcContext, scope, item.members.values()); + } + + // Check for duplicate client names for union variants + for (const item of namespace.unions.values()) { + validateClientNamesCore(tcgcContext, scope, item.variants.values()); + } + + // Check for duplicate client names for nested namespaces + for (const item of namespace.namespaces.values()) { + validateClientNamesPerNamespace(tcgcContext, scope, item); + } +} + +function validateClientNamesCore( + tcgcContext: TCGCContext, + scope: string | typeof AllScopes, + items: Iterable< + | Namespace + | Scalar + | Operation + | Interface + | Model + | Enum + | Union + | ModelProperty + | EnumMember + | UnionVariant + > +) { + const duplicateTracker = new DuplicateTracker< + string, + Type | DecoratorExpressionNode | AugmentDecoratorStatementNode + >(); + + for (const item of items) { + const clientName = getClientNameOverride(tcgcContext, item, scope); + if (clientName !== undefined) { + const clientNameDecorator = item.decorators.find((x) => x.definition?.name === "@clientName"); + if (clientNameDecorator?.node !== undefined) { + duplicateTracker.track(clientName, clientNameDecorator.node); + } + } else { + if (item.name !== undefined && typeof item.name === "string") { + duplicateTracker.track(item.name, item); + } + } + } + + reportDuplicateClientNames(tcgcContext.program, duplicateTracker, scope); +} + +function reportDuplicateClientNames( + program: Program, + duplicateTracker: DuplicateTracker< + string, + Type | DecoratorExpressionNode | AugmentDecoratorStatementNode + >, + scope: string | typeof AllScopes +) { + for (const [name, duplicates] of duplicateTracker.entries()) { + for (const item of duplicates) { + const scopeStr = scope === AllScopes ? "AllScopes" : scope; + // If the item is a decorator application node + if ( + item.kind === SyntaxKind.DecoratorExpression || + item.kind === SyntaxKind.AugmentDecoratorStatement + ) { + reportDiagnostic(program, { + code: "duplicate-client-name", + format: { name, scope: scopeStr }, + target: item, + }); + } else { + reportDiagnostic(program, { + code: "duplicate-client-name", + messageId: "nonDecorator", + format: { name, scope: scopeStr }, + target: item, + }); + } + } } } diff --git a/packages/typespec-client-generator-core/test/decorators.test.ts b/packages/typespec-client-generator-core/test/decorators.test.ts index 55daeb1319..2ef945a06e 100644 --- a/packages/typespec-client-generator-core/test/decorators.test.ts +++ b/packages/typespec-client-generator-core/test/decorators.test.ts @@ -2724,6 +2724,7 @@ describe("typespec-client-generator-core: decorators", () => { strictEqual(runner.context.sdkPackage.clients[0].methods[0].parameters[0].name, "body"); }); + it("empty client name", async () => { const diagnostics = await runner.diagnose(` @service({}) @@ -2740,6 +2741,359 @@ describe("typespec-client-generator-core: decorators", () => { code: "@azure-tools/typespec-client-generator-core/empty-client-name", }); }); + + it("duplicate model client name with a random language scope", async () => { + const diagnostics = await runner.diagnose( + ` + @service + namespace Contoso.WidgetManager; + + @clientName("Test", "random") + model Widget { + @key + id: int32; + } + + model Test { + prop1: string; + } + ` + ); + + expectDiagnostics(diagnostics, [ + { + code: "@azure-tools/typespec-client-generator-core/duplicate-client-name", + message: 'Client name: "Test" is duplicated in language scope: "random"', + }, + { + code: "@azure-tools/typespec-client-generator-core/duplicate-client-name", + message: + 'Client name: "Test" is defined somewhere causing nameing conflicts in language scope: "random"', + }, + ]); + }); + + it("duplicate model, enum, union client name with all language scopes", async () => { + const diagnostics = await runner.diagnose( + ` + @service + namespace Contoso.WidgetManager; + + @clientName("B") + enum A { + one + } + + model B {} + + @clientName("B") + union C {} + ` + ); + + expectDiagnostics(diagnostics, [ + { + code: "@azure-tools/typespec-client-generator-core/duplicate-client-name", + message: + 'Client name: "B" is defined somewhere causing nameing conflicts in language scope: "AllScopes"', + }, + { + code: "@azure-tools/typespec-client-generator-core/duplicate-client-name", + message: 'Client name: "B" is duplicated in language scope: "AllScopes"', + }, + { + code: "@azure-tools/typespec-client-generator-core/duplicate-client-name", + message: 'Client name: "B" is duplicated in language scope: "AllScopes"', + }, + ]); + }); + + it("duplicate operation with all language scopes", async () => { + const diagnostics = await runner.diagnose( + ` + @service + namespace Contoso.WidgetManager; + + @clientName("b") + @route("/a") + op a(): void; + + @route("/b") + op b(): void; + ` + ); + + expectDiagnostics(diagnostics, [ + { + code: "@azure-tools/typespec-client-generator-core/duplicate-client-name", + message: 'Client name: "b" is duplicated in language scope: "AllScopes"', + }, + { + code: "@azure-tools/typespec-client-generator-core/duplicate-client-name", + message: + 'Client name: "b" is defined somewhere causing nameing conflicts in language scope: "AllScopes"', + }, + ]); + }); + + it("duplicate scalar with all language scopes", async () => { + const diagnostics = await runner.diagnose( + ` + @service + namespace Contoso.WidgetManager; + + @clientName("b") + scalar a extends string; + + scalar b extends string; + ` + ); + + expectDiagnostics(diagnostics, [ + { + code: "@azure-tools/typespec-client-generator-core/duplicate-client-name", + message: 'Client name: "b" is duplicated in language scope: "AllScopes"', + }, + { + code: "@azure-tools/typespec-client-generator-core/duplicate-client-name", + message: + 'Client name: "b" is defined somewhere causing nameing conflicts in language scope: "AllScopes"', + }, + ]); + }); + + it("duplicate interface with all language scopes", async () => { + const diagnostics = await runner.diagnose( + ` + @service + namespace Contoso.WidgetManager; + + @clientName("B") + @route("/a") + interface A { + } + + @route("/b") + interface B { + } + ` + ); + + expectDiagnostics(diagnostics, [ + { + code: "@azure-tools/typespec-client-generator-core/duplicate-client-name", + message: 'Client name: "B" is duplicated in language scope: "AllScopes"', + }, + { + code: "@azure-tools/typespec-client-generator-core/duplicate-client-name", + message: + 'Client name: "B" is defined somewhere causing nameing conflicts in language scope: "AllScopes"', + }, + ]); + }); + + it("duplicate model property with all language scopes", async () => { + const diagnostics = await runner.diagnose( + ` + @service + namespace Contoso.WidgetManager; + + model A { + @clientName("prop2") + prop1: string; + prop2: string; + } + ` + ); + + expectDiagnostics(diagnostics, [ + { + code: "@azure-tools/typespec-client-generator-core/duplicate-client-name", + message: 'Client name: "prop2" is duplicated in language scope: "AllScopes"', + }, + { + code: "@azure-tools/typespec-client-generator-core/duplicate-client-name", + message: + 'Client name: "prop2" is defined somewhere causing nameing conflicts in language scope: "AllScopes"', + }, + ]); + }); + + it("duplicate enum member with all language scopes", async () => { + const diagnostics = await runner.diagnose( + ` + @service + namespace Contoso.WidgetManager; + + enum A { + @clientName("two") + one, + two + } + ` + ); + + expectDiagnostics(diagnostics, [ + { + code: "@azure-tools/typespec-client-generator-core/duplicate-client-name", + message: 'Client name: "two" is duplicated in language scope: "AllScopes"', + }, + { + code: "@azure-tools/typespec-client-generator-core/duplicate-client-name", + message: + 'Client name: "two" is defined somewhere causing nameing conflicts in language scope: "AllScopes"', + }, + ]); + }); + + it("duplicate union variant with all language scopes", async () => { + const diagnostics = await runner.diagnose( + ` + @service + namespace Contoso.WidgetManager; + + union Foo { + @clientName("b") + a: {}, + b: {} + } + ` + ); + + expectDiagnostics(diagnostics, [ + { + code: "@azure-tools/typespec-client-generator-core/duplicate-client-name", + message: 'Client name: "b" is duplicated in language scope: "AllScopes"', + }, + { + code: "@azure-tools/typespec-client-generator-core/duplicate-client-name", + message: + 'Client name: "b" is defined somewhere causing nameing conflicts in language scope: "AllScopes"', + }, + ]); + }); + + it("duplicate namespace with all language scopes", async () => { + const diagnostics = await runner.diagnose( + ` + @service + namespace A{ + namespace B {} + @clientName("B") + namespace C {} + } + ` + ); + + expectDiagnostics(diagnostics, [ + { + code: "@azure-tools/typespec-client-generator-core/duplicate-client-name", + message: + 'Client name: "B" is defined somewhere causing nameing conflicts in language scope: "AllScopes"', + }, + { + code: "@azure-tools/typespec-client-generator-core/duplicate-client-name", + message: 'Client name: "B" is duplicated in language scope: "AllScopes"', + }, + ]); + }); + + it("duplicate enum and model within nested namespace for all language scopes", async () => { + const diagnostics = await runner.diagnose( + ` + @service + namespace A{ + namespace B { + @clientName("B") + enum A { + one + } + + model B {} + } + + @clientName("B") + model A {} + } + ` + ); + + expectDiagnostics(diagnostics, [ + { + code: "@azure-tools/typespec-client-generator-core/duplicate-client-name", + message: + 'Client name: "B" is defined somewhere causing nameing conflicts in language scope: "AllScopes"', + }, + { + code: "@azure-tools/typespec-client-generator-core/duplicate-client-name", + message: 'Client name: "B" is duplicated in language scope: "AllScopes"', + }, + ]); + }); + + it("duplicate model with model only generation for all language scopes", async () => { + const diagnostics = await runner.diagnose( + ` + model Foo {} + + @clientName("Foo") + model Bar {} + ` + ); + + expectDiagnostics(diagnostics, [ + { + code: "@azure-tools/typespec-client-generator-core/duplicate-client-name", + message: + 'Client name: "Foo" is defined somewhere causing nameing conflicts in language scope: "AllScopes"', + }, + { + code: "@azure-tools/typespec-client-generator-core/duplicate-client-name", + message: 'Client name: "Foo" is duplicated in language scope: "AllScopes"', + }, + ]); + }); + + it("duplicate model client name with multiple language scopes", async () => { + const diagnostics = await runner.diagnose( + ` + @service + namespace Contoso.WidgetManager; + + @clientName("Test", "csharp,python,java") + model Widget { + @key + id: int32; + } + + @clientName("Widget", "java") + model Test { + prop1: string; + } + ` + ); + + expectDiagnostics(diagnostics, [ + { + code: "@azure-tools/typespec-client-generator-core/duplicate-client-name", + message: 'Client name: "Test" is duplicated in language scope: "csharp"', + }, + { + code: "@azure-tools/typespec-client-generator-core/duplicate-client-name", + message: + 'Client name: "Test" is defined somewhere causing nameing conflicts in language scope: "csharp"', + }, + { + code: "@azure-tools/typespec-client-generator-core/duplicate-client-name", + message: 'Client name: "Test" is duplicated in language scope: "python"', + }, + { + code: "@azure-tools/typespec-client-generator-core/duplicate-client-name", + message: + 'Client name: "Test" is defined somewhere causing nameing conflicts in language scope: "python"', + }, + ]); + }); }); describe("versioning projection", () => {