diff --git a/.chronus/changes/witemple-no-override-required-as-optional-2024-5-25-14-39-2.md b/.chronus/changes/witemple-no-override-required-as-optional-2024-5-25-14-39-2.md new file mode 100644 index 0000000000..2287d803c7 --- /dev/null +++ b/.chronus/changes/witemple-no-override-required-as-optional-2024-5-25-14-39-2.md @@ -0,0 +1,23 @@ +--- +changeKind: breaking +packages: + - "@typespec/compiler" +--- + +Disallows overriding a required inherited property with an optional property. + +In previous versions of TypeSpec, it was possible to override a required property with an optional property. This is no longer allowed. This change may result in errors in your code if you were relying on this bug, but specifications that used this behavior are likely to have been exposed to errors resulting from incoherent type checking behavior. + +The following example demonstrates the behavior that is no longer allowed: + +```tsp +model Base { + example: string; +} + +model Child extends Base { + example?: string; +} +``` + +In this example, the `Child` model overrides the `example` property from the `Base` model with an optional property. This is no longer allowed. diff --git a/packages/compiler/src/core/checker.ts b/packages/compiler/src/core/checker.ts index 5af258ad4b..fe52cdf802 100644 --- a/packages/compiler/src/core/checker.ts +++ b/packages/compiler/src/core/checker.ts @@ -4618,6 +4618,8 @@ export function createChecker(program: Program): Checker { const parentType = getTypeName(overriddenProp.type); const newPropType = getTypeName(newProp.type); + let invalid = false; + if (!isAssignable) { reportCheckerDiagnostic( createDiagnostic({ @@ -4626,8 +4628,22 @@ export function createChecker(program: Program): Checker { target: diagnosticTarget ?? newProp, }) ); - return; + invalid = true; } + + if (!overriddenProp.optional && newProp.optional) { + reportCheckerDiagnostic( + createDiagnostic({ + code: "override-property-mismatch", + messageId: "disallowedOptionalOverride", + format: { propName: overriddenProp.name }, + target: diagnosticTarget ?? newProp, + }) + ); + invalid = true; + } + + if (invalid) return; } properties.set(newProp.name, newProp); diff --git a/packages/compiler/src/core/messages.ts b/packages/compiler/src/core/messages.ts index ad69922172..025d50a5ef 100644 --- a/packages/compiler/src/core/messages.ts +++ b/packages/compiler/src/core/messages.ts @@ -357,6 +357,7 @@ const diagnostics = { severity: "error", messages: { default: paramMessage`Model has an inherited property named ${"propName"} of type ${"propType"} which cannot override type ${"parentType"}`, + disallowedOptionalOverride: paramMessage`Model has a required inherited property named ${"propName"} which cannot be overridden as optional`, }, }, "extend-scalar": { diff --git a/packages/compiler/test/checker/model.test.ts b/packages/compiler/test/checker/model.test.ts index 5d0ccba8fb..9bcc3d65fa 100644 --- a/packages/compiler/test/checker/model.test.ts +++ b/packages/compiler/test/checker/model.test.ts @@ -445,6 +445,73 @@ describe("compiler: models", () => { ]); }); + it("disallows subtype overriding required parent property with optional property", async () => { + testHost.addTypeSpecFile( + "main.tsp", + ` + model A { x: int32; } + model B extends A { x?: int32; } + ` + ); + + const diagnostics = await testHost.diagnose("main.tsp"); + expectDiagnostics(diagnostics, [ + { + code: "override-property-mismatch", + severity: "error", + message: + "Model has a required inherited property named x which cannot be overridden as optional", + }, + ]); + }); + + it("disallows subtype overriding required parent property with optional through multiple levels of inheritance", async () => { + testHost.addTypeSpecFile( + "main.tsp", + ` + model A { x: int32; } + model B extends A { } + model C extends B { x?: int16; } + ` + ); + + const diagnostics = await testHost.diagnose("main.tsp"); + expectDiagnostics(diagnostics, [ + { + code: "override-property-mismatch", + severity: "error", + message: + "Model has a required inherited property named x which cannot be overridden as optional", + }, + ]); + }); + + it("shows both errors when an override is optional and not assignable", async () => { + testHost.addTypeSpecFile( + "main.tsp", + ` + model A { x: int32; } + model B extends A { x?: string; } + ` + ); + + const diagnostics = await testHost.diagnose("main.tsp"); + expectDiagnostics(diagnostics, [ + { + code: "override-property-mismatch", + severity: "error", + message: + "Model has an inherited property named x of type string which cannot override type int32", + }, + { + code: "override-property-mismatch", + severity: "error", + message: + "Model has a required inherited property named x which cannot be overridden as optional", + }, + ]); + }); + it("allow multiple overrides", async () => { testHost.addTypeSpecFile( "main.tsp",