Skip to content

Commit 1140c23

Browse files
tadeleshtadeleshCopilot
authored
[TCGC] Fix @override check regression (#4730)
## Problem The `@override` `@path`-preservation check (added in #4644, reworked in #4693) produced false `override-parameters-mismatch` diagnostics on real ARM specs. Three distinct issues: 1. **#4693** switched parameter matching to name-based, which over-relaxed the check. 2. **#4644** compared parameters by sorted **position**; when an override adds/reorders parameters the index misaligns, flagging params that already carry `@path` (e.g. botservice `channelName`). 3. Determining "is a path parameter" from the `@path` decorator in the **type graph** (`isPathParam`) is unreliable: a parameter can carry `@path` without being part of the realized route (e.g. an ARM key surfaced by a templated `ArmProviderActionSync`, as in cognitiveservices `calculateModelCapacity`), and conversely a `@path` nested inside a plain model or `@bodyRoot` IS realized. ## Fix - **Revert #4693** to restore positional structural comparison of the parameter lists. - **Resolve the original operation's realized HTTP route** (`getHttpOperation`) to get its actual path parameters — the ground truth that handles both phantom `@path` (orphaned, not in route → not enforced) and nested `@path` (in route → enforced). - **Match the corresponding override parameter by name** (not position) and require it to be a path parameter (`isPathParam`). This removes the `finishType` workaround and the body-param heuristics entirely. ## Result - All previously-failing ARM specs compile: storage, security, recoveryservicesbackup, quota, policyinsights, iothub, cognitiveservices, botservice, apimanagement, frontdoor, databoxedge. Overrides that genuinely dropped `@path` on a realized path param add `@path` (correct); phantom cases (cognitiveservices `calculateModelCapacity` `name`) need no spec change. - `override.test.ts` 16/16; full TCGC suite 1330 pass. --------- Co-authored-by: tadelesh <chenjieshi@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent c461ceb commit 1140c23

4 files changed

Lines changed: 77 additions & 69 deletions

File tree

.chronus/changes/tcgc-fixPathInOverride-2026-5-16-15-19-10.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,4 +4,4 @@ packages:
44
- "@azure-tools/typespec-client-generator-core"
55
---
66

7-
Verify all `@path` parameters are present in override
7+
Verify all path parameters are preserved in `@override` customizations. The check now resolves the original operation's realized HTTP route to determine its path parameters, instead of relying on the `@path` decorator in the type graph. This correctly handles parameters that carry `@path` but are not part of the realized route (for example an ARM key surfaced by a templated provider action) as well as `@path` parameters nested inside a plain model or `@bodyRoot`. The corresponding override parameter is matched by name rather than position so overrides that add or reorder parameters no longer produce false `override-parameters-mismatch` diagnostics.

.chronus/changes/tcgc-override-match-params-by-name-2026-6-23-16-30-0.md

Lines changed: 0 additions & 7 deletions
This file was deleted.

.chronus/changes/tcgc-override-realized-path-2026-6-23-2-30-0.md

Lines changed: 0 additions & 7 deletions
This file was deleted.

packages/typespec-client-generator-core/src/decorators.ts

Lines changed: 76 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -689,7 +689,8 @@ export function getClientNameOverride(
689689
return getScopedDecoratorData(context, clientNameKey, entity, languageScope);
690690
}
691691

692-
// Recursive function to collect parameter names
692+
// Recursive function to collect all (possibly nested) leaf parameters of an operation,
693+
// used to structurally compare the original and override parameter lists.
693694
function collectParams(
694695
program: Program,
695696
properties: RekeyableMap<string, ModelProperty>,
@@ -709,26 +710,22 @@ function collectParams(
709710
return params;
710711
}
711712

712-
// Collect the names of the parameters that are realized as path parameters in
713-
// the resolved HTTP route of an operation. Returns `undefined` when the HTTP
714-
// route cannot be resolved (for example for non-HTTP operations), in which case
715-
// callers should fall back to inspecting the `@path` decorator directly.
716-
function getRealizedPathParamNames(
717-
program: Program,
718-
operation: Operation,
719-
): Set<string> | undefined {
720-
try {
721-
const [httpOperation] = getHttpOperation(program, operation);
722-
const names = new Set<string>();
723-
for (const parameter of httpOperation.parameters.parameters) {
724-
if (parameter.type === "path") {
725-
names.add(parameter.param.name);
726-
}
713+
// Collect the names of the parameters that are realized as `path` parameters in the
714+
// operation's actual HTTP route. This is the ground truth for "is a path parameter":
715+
// a `@path` decorator in the type graph (isPathParam) is not sufficient because a
716+
// parameter can carry `@path` without being part of the realized route (for example an
717+
// ARM key surfaced by a templated provider action), and conversely a `@path` nested
718+
// inside a plain model or `@bodyRoot` is realized. Resolving the HTTP route handles
719+
// both cases correctly.
720+
function getRealizedPathParamNames(program: Program, operation: Operation): Set<string> {
721+
const result = new Set<string>();
722+
const httpOperation = ignoreDiagnostics(getHttpOperation(program, operation));
723+
for (const parameter of httpOperation.parameters.parameters) {
724+
if (parameter.type === "path") {
725+
result.add(parameter.param.name);
727726
}
728-
return names;
729-
} catch {
730-
return undefined;
731727
}
728+
return result;
732729
}
733730

734731
export const $override = (
@@ -740,38 +737,47 @@ export const $override = (
740737
// omit all override operation
741738
context.program.stateMap(omitOperation).set(override, true);
742739

743-
// Collect the parameters of both operations. Parameters are matched by name
744-
// (see below) rather than by position, so the lists do not need to be sorted.
740+
// Collect the parameters of both operations. Parameters are matched by name (see
741+
// below) rather than by position: overrides are allowed to add, remove, or regroup
742+
// parameters (for example wrapping several parameters in a customization model), so
743+
// comparing the two lists by sorted position produces false mismatches whenever the
744+
// parameter sets differ in shape.
745745
const originalParams = collectParams(context.program, original.parameters.properties);
746746
const overrideParams = collectParams(context.program, override.parameters.properties);
747+
// Group override parameters by name so that parameters can be matched by name
748+
// rather than by position (overrides may add or reorder parameters). Parameters
749+
// that share a name (for example a realized path parameter and a body property of
750+
// the same name) are matched in declaration order.
751+
const overrideParamsByName = new Map<string, ModelProperty[]>();
752+
for (const param of overrideParams) {
753+
const existing = overrideParamsByName.get(param.name);
754+
if (existing) {
755+
existing.push(param);
756+
} else {
757+
overrideParamsByName.set(param.name, [param]);
758+
}
759+
}
760+
const consumedOverrideParams = new Map<string, number>();
747761

748-
// Match override parameters to original parameters by name. Overrides are
749-
// allowed to add, remove, or regroup parameters (for example wrapping several
750-
// parameters in a customization model), so comparing the two lists by sorted
751-
// position produces false `override-parameters-mismatch` diagnostics whenever
752-
// the parameter sets differ in shape.
753-
const overrideParamsByName = new Map(overrideParams.map((p) => [p.name, p]));
754-
755-
// Determine which parameters are realized as path parameters in the resolved
756-
// HTTP route of the original operation. Checking the `@path` decorator alone is
757-
// not sufficient, because some templated parameters (for example ARM scope
758-
// models) carry `@path` in the type graph without being realized as path
759-
// parameters in the operation's actual route.
760-
const realizedPathParamNames = getRealizedPathParamNames(context.program, original);
761-
762-
// A `@clientLocation` on any override parameter indicates an intentional
763-
// customization where non-path params are just pass-throughs, so the `@path`
764-
// preservation check below is skipped in that case.
762+
// A `@clientLocation` on any override parameter indicates an intentional customization
763+
// where non-path params are just pass-throughs, so the `@path` preservation check is
764+
// skipped in that case.
765765
const overrideHasClientLocation = overrideParams.some((p) =>
766766
p.decorators.some((d) => d.decorator.name === "$clientLocation"),
767767
);
768768

769-
// Check that every required original parameter has a matching override
770-
// parameter, omitting optional parameters.
769+
// Check that every required original parameter has a matching override parameter,
770+
// omitting optional parameters. While matching, collect the parameters that carry
771+
// `@path` in the original but lost it in the override; these are the only candidates
772+
// for an `override-parameters-mismatch`, and only they require resolving the (more
773+
// expensive) HTTP route to confirm they are realized path parameters.
771774
let parametersMatch = true;
772775
let checkParameter: ModelProperty | undefined = undefined;
776+
const droppedPathParamNames: string[] = [];
773777
for (const originalParam of originalParams) {
774-
const overrideParam = overrideParamsByName.get(originalParam.name);
778+
const candidates = overrideParamsByName.get(originalParam.name);
779+
const consumed = consumedOverrideParams.get(originalParam.name) ?? 0;
780+
const overrideParam = candidates?.[consumed];
775781
if (
776782
overrideParam === undefined ||
777783
!compareModelProperties(context.program, originalParam, overrideParam)
@@ -784,24 +790,18 @@ export const $override = (
784790
continue;
785791
}
786792
}
793+
consumedOverrideParams.set(originalParam.name, consumed + 1);
787794

788-
// Warn if original param has @path but the matching override param doesn't.
789-
const originalIsRealizedPathParam = realizedPathParamNames
790-
? realizedPathParamNames.has(originalParam.name)
791-
: isPathParam(context.program, originalParam);
795+
// Gating on `isPathParam(originalParam)` selects the realized path parameter over a
796+
// body property that shares its name. The realized-route confirmation is deferred
797+
// (see below) so the HTTP route is only resolved when an override actually drops a
798+
// `@path`.
792799
if (
793-
originalIsRealizedPathParam &&
800+
isPathParam(context.program, originalParam) &&
794801
!isPathParam(context.program, overrideParam) &&
795802
!overrideHasClientLocation
796803
) {
797-
reportDiagnostic(context.program, {
798-
code: "override-parameters-mismatch",
799-
target: context.decoratorTarget,
800-
format: {
801-
methodName: original.name,
802-
checkParameter: overrideParam.name,
803-
},
804-
});
804+
droppedPathParamNames.push(overrideParam.name);
805805
}
806806

807807
// Apply the alternate type to the original parameter
@@ -821,6 +821,28 @@ export const $override = (
821821
);
822822
}
823823

824+
// Only resolve the original operation's HTTP route (which is comparatively expensive)
825+
// when at least one parameter dropped its `@path`, and report the ones that are
826+
// actually realized path parameters of the route. A `@path` decorator in the type
827+
// graph is not sufficient on its own: it can be carried by a parameter that is not
828+
// part of the realized route (for example an ARM key surfaced by a templated provider
829+
// action), which must not be reported.
830+
if (droppedPathParamNames.length > 0) {
831+
const originalRealizedPathParamNames = getRealizedPathParamNames(context.program, original);
832+
for (const name of droppedPathParamNames) {
833+
if (originalRealizedPathParamNames.has(name)) {
834+
reportDiagnostic(context.program, {
835+
code: "override-parameters-mismatch",
836+
target: context.decoratorTarget,
837+
format: {
838+
methodName: original.name,
839+
checkParameter: name,
840+
},
841+
});
842+
}
843+
}
844+
}
845+
824846
if (!parametersMatch) {
825847
reportDiagnostic(context.program, {
826848
code: "override-parameters-mismatch",

0 commit comments

Comments
 (0)