From 10349f37ba3f8add489085d06edd9c8c8e9afc96 Mon Sep 17 00:00:00 2001 From: "Garen J. Torikian" Date: Mon, 18 May 2026 13:18:08 -0400 Subject: [PATCH] fix(compat): method-param reorder severity follows passing style MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Method parameter `sensitivity.order` was being sourced from `policy.constructorOrderMatters`, which conflates constructor positional semantics with method call-site semantics. For Ruby/Python/Elixir (`constructorOrderMatters: true`), this flagged kwarg-only method reorders as breaking even though callers literally cannot observe parameter declaration order. Method param reorder severity is now decided by the param's `passing` style: positional → breaking, named → soft-risk, keyword/options_object → not reported. Constructor handling is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/compat/classify.ts | 51 ++++++++++--- src/compat/ir.ts | 8 +- test/compat/fixtures/language-matrix.test.ts | 77 ++++++++++++++++++++ 3 files changed, 124 insertions(+), 12 deletions(-) diff --git a/src/compat/classify.ts b/src/compat/classify.ts index 9faef65..2adc3fc 100644 --- a/src/compat/classify.ts +++ b/src/compat/classify.ts @@ -241,17 +241,48 @@ function classifyParameterChanges( ); } - // Position changed (order-sensitive) + // Position changed. + // + // Constructors are governed by `policy.constructorOrderMatters` — some + // languages treat ctor positional order as part of the public API even + // when method args are named. + // + // For methods, the parameter's `passing` style decides: + // - 'positional' → callers reference by index; reorder is breaking + // - 'named' → both styles work; reorder is soft-risk (positional callers exist) + // - 'keyword' → callers MUST use names (Ruby kwargs, Python kw-only, Elixir); position invisible + // - 'options_object' → callers pass a single object literal (Node); position invisible if (baseParam.position !== candParam.position) { - const orderMatters = isConstructor ? policy.constructorOrderMatters : baseParam.sensitivity.order; - - if (orderMatters) { - const category: CompatChangeCategory = isConstructor - ? 'constructor_position_changed_order_sensitive' - : 'parameter_position_changed_order_sensitive'; + if (isConstructor) { + if (policy.constructorOrderMatters) { + changes.push( + makeChange({ + category: 'constructor_position_changed_order_sensitive', + symbol: baseline.fqName, + old: { parameter: baseParam.publicName, position: String(baseParam.position) }, + new: { parameter: candParam.publicName, position: String(candParam.position) }, + message: `Parameter "${baseParam.publicName}" moved from position ${baseParam.position} to ${candParam.position} on "${baseline.displayName}"`, + policy, + specRef, + }), + ); + } else { + changes.push( + makeChange({ + category: 'constructor_reordered_named_friendly', + symbol: baseline.fqName, + old: { parameter: baseParam.publicName, position: String(baseParam.position) }, + new: { parameter: candParam.publicName, position: String(candParam.position) }, + message: `Parameter "${baseParam.publicName}" reordered on "${baseline.displayName}" (named-friendly language)`, + policy, + specRef, + }), + ); + } + } else if (baseParam.passing === 'positional') { changes.push( makeChange({ - category, + category: 'parameter_position_changed_order_sensitive', symbol: baseline.fqName, old: { parameter: baseParam.publicName, position: String(baseParam.position) }, new: { parameter: candParam.publicName, position: String(candParam.position) }, @@ -260,8 +291,7 @@ function classifyParameterChanges( specRef, }), ); - } else { - // Reordered but in a named-friendly language → soft-risk + } else if (baseParam.passing === 'named') { changes.push( makeChange({ category: 'constructor_reordered_named_friendly', @@ -274,6 +304,7 @@ function classifyParameterChanges( }), ); } + // keyword / options_object: position is invisible to callers — no change emitted. } } diff --git a/src/compat/ir.ts b/src/compat/ir.ts index 0ef8eff..83d092d 100644 --- a/src/compat/ir.ts +++ b/src/compat/ir.ts @@ -294,16 +294,20 @@ export function apiSurfaceToSnapshot(surface: ApiSurface): CompatSnapshot { /** Convert a legacy ApiParam to a CompatParameter. */ function apiParamToCompatParam(param: ApiParam, position: number, language: LanguageId): CompatParameter { const policy = getDefaultPolicy(language); + const passing = param.passingStyle ?? inferPassingStyle(language); return { publicName: param.name, position, required: !param.optional, nullable: false, hasDefault: param.optional, - passing: param.passingStyle ?? inferPassingStyle(language), + passing, type: { name: param.type }, sensitivity: { - order: policy.constructorOrderMatters, + // Method-call order matters only when callers pass positionally. keyword + // (Ruby/Python/Elixir) and options_object (Node) hide position entirely; + // named (PHP/Kotlin/C#) is handled as soft-risk in classify. + order: passing === 'positional', publicName: policy.methodParameterNamesArePublicApi, requiredness: true, type: true, diff --git a/test/compat/fixtures/language-matrix.test.ts b/test/compat/fixtures/language-matrix.test.ts index dde9906..0b795a3 100644 --- a/test/compat/fixtures/language-matrix.test.ts +++ b/test/compat/fixtures/language-matrix.test.ts @@ -605,3 +605,80 @@ describe('PHP Case 4: Named-argument breaks', () => { expect(renames.every((c) => c.severity === 'breaking')).toBe(true); }); }); + +// --------------------------------------------------------------------------- +// Method-parameter reorder: passing-style decides severity +// (kwarg-only call sites cannot observe parameter declaration order) +// --------------------------------------------------------------------------- +describe('Method param reorder by passing style', () => { + const reorderedPair = (language: LanguageId, passing: CompatParameter['passing']) => { + const mk = (a: number, b: number) => + makeSnapshot(language, [ + callable('Authorization.listResources', [ + param('search', a, { passing, required: false, hasDefault: true }), + param('requestOptions', b, { passing, required: false, hasDefault: true }), + ]), + ]); + return { baseline: mk(0, 1), candidate: mk(1, 0) }; + }; + + it('ruby keyword-arg method reorder emits no change', () => { + const { baseline, candidate } = reorderedPair('ruby', 'keyword'); + const result = diffSnapshots(baseline, candidate); + const posChanges = result.changes.filter( + (c) => + c.category === 'parameter_position_changed_order_sensitive' || + c.category === 'constructor_reordered_named_friendly', + ); + expect(posChanges).toHaveLength(0); + }); + + it('python keyword-arg method reorder emits no change', () => { + const { baseline, candidate } = reorderedPair('python', 'keyword'); + const result = diffSnapshots(baseline, candidate); + const posChanges = result.changes.filter( + (c) => + c.category === 'parameter_position_changed_order_sensitive' || + c.category === 'constructor_reordered_named_friendly', + ); + expect(posChanges).toHaveLength(0); + }); + + it('elixir keyword-arg method reorder emits no change', () => { + const { baseline, candidate } = reorderedPair('elixir', 'keyword'); + const result = diffSnapshots(baseline, candidate); + const posChanges = result.changes.filter( + (c) => + c.category === 'parameter_position_changed_order_sensitive' || + c.category === 'constructor_reordered_named_friendly', + ); + expect(posChanges).toHaveLength(0); + }); + + it('node options_object method reorder emits no change', () => { + const { baseline, candidate } = reorderedPair('node', 'options_object'); + const result = diffSnapshots(baseline, candidate); + const posChanges = result.changes.filter( + (c) => + c.category === 'parameter_position_changed_order_sensitive' || + c.category === 'constructor_reordered_named_friendly', + ); + expect(posChanges).toHaveLength(0); + }); + + it('php named-arg method reorder is soft-risk (positional callers exist)', () => { + const { baseline, candidate } = reorderedPair('php', 'named'); + const result = diffSnapshots(baseline, candidate); + const posChanges = result.changes.filter((c) => c.category === 'constructor_reordered_named_friendly'); + expect(posChanges.length).toBeGreaterThan(0); + expect(posChanges.every((c) => c.severity === 'soft-risk')).toBe(true); + }); + + it('go positional method reorder is breaking', () => { + const { baseline, candidate } = reorderedPair('go', 'positional'); + const result = diffSnapshots(baseline, candidate); + const posChanges = result.changes.filter((c) => c.category === 'parameter_position_changed_order_sensitive'); + expect(posChanges.length).toBeGreaterThan(0); + expect(posChanges.every((c) => c.severity === 'breaking')).toBe(true); + }); +});