From a996fe68753c1a44bcb98b9d833f724743c8326c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BB=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier?= Date: Wed, 4 Aug 2021 17:31:49 +0200 Subject: [PATCH 1/2] fix(jsii-diff): renaming a positional argument is a breaking change in Python Related: #2927 --- packages/jsii-diff/test/python.test.ts | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 packages/jsii-diff/test/python.test.ts diff --git a/packages/jsii-diff/test/python.test.ts b/packages/jsii-diff/test/python.test.ts new file mode 100644 index 0000000000..edfe510b4d --- /dev/null +++ b/packages/jsii-diff/test/python.test.ts @@ -0,0 +1,24 @@ +import { expectError } from './util'; + +// ---------------------------------------------------------------------- + +test.each([ + ['class', 'constructor'], + ['class', 'method'], + ['interface', 'method'], +])( + 'not okay to rename a positional parameter', + (scope, decl) => + expectError( + /positional parameter was renamed from 'previous' to 'current'/, + // Note: name is ITest so we're good for both class & interface... Yes, this is ugly. + ` + export ${scope} ITest { + ${decl}(previous: any)${decl === 'constructor' ? '' : ': void'}${scope === 'class' ? ' { previous.use(); }' : ';'} + }`, + ` + export ${scope} ITest { + ${decl}(current: any)${decl === 'constructor' ? '' : ': void'}${scope === 'class' ? ' { current.use(); }' : ';'} + }`, + ), +); From c665b7e73a6c76ed6b342e7de2ad9b9ca76821e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BB=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier?= Date: Mon, 9 Aug 2021 14:56:09 +0200 Subject: [PATCH 2/2] feat: always use root signature parameter names In languages such as Python (<3.8) and Ruby, positional parameters can be referred to using their names, making these names part of the function signature. In order to avoid enforing parameter name consistency on the TypeScript source, the `jsii` compiler will always use the root declaration's parameter names when emitting the `.jsii` assembly file. Related #2927 --- packages/jsii/lib/validator.ts | 4 ++ packages/jsii/test/compat.test.ts | 107 ++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+) create mode 100644 packages/jsii/test/compat.test.ts diff --git a/packages/jsii/lib/validator.ts b/packages/jsii/lib/validator.ts index 5658d22260..202d0075b1 100644 --- a/packages/jsii/lib/validator.ts +++ b/packages/jsii/lib/validator.ts @@ -461,6 +461,10 @@ function _defaultValidations(): ValidationFunction[] { ), ); } + // Standardize to the root method's argument signature, because in + // Python and Ruby, positional parameters can also be referred to by + // name, which makes their names be a part of the function's signature. + actParam.name = expParam.name; } } diff --git a/packages/jsii/test/compat.test.ts b/packages/jsii/test/compat.test.ts new file mode 100644 index 0000000000..f29e7855f1 --- /dev/null +++ b/packages/jsii/test/compat.test.ts @@ -0,0 +1,107 @@ +import { sourceToAssemblyHelper } from '../lib'; + +//////////////////////////////////////////////////////////////////////////////// +// In Python and Ruby, positional parameters can be referred to by name, making +// them part of a function's signature. Always using the root implementation +// parameter names makes no difference in the undelying runtime, as JS +// positional arguments are... positional, but it reduces friction for Python +// and Ruby developers. +test('overriding methods use the overriden parameter names', async () => { + const assembly = await sourceToAssemblyHelper(` + export abstract class AbstractClass { + public abstract method(param: number): void; + } + + export class ConcreteClass extends AbstractClass { + private constructor() { super(); } + + public method(_arg: number): void { + // Nothing to do... + } + } + `); + + expect(assembly.types!['testpkg.AbstractClass']).toEqual({ + abstract: true, + assembly: 'testpkg', + fqn: 'testpkg.AbstractClass', + kind: 'class', + methods: [ + { + abstract: true, + locationInModule: { filename: 'index.ts', line: 3 }, + name: 'method', + parameters: [{ name: 'param', type: { primitive: 'number' } }], + }, + ], + initializer: {}, + locationInModule: { filename: 'index.ts', line: 2 }, + name: 'AbstractClass', + }); + + expect(assembly.types!['testpkg.ConcreteClass']).toEqual({ + assembly: 'testpkg', + base: 'testpkg.AbstractClass', + fqn: 'testpkg.ConcreteClass', + kind: 'class', + methods: [ + { + locationInModule: { filename: 'index.ts', line: 9 }, + name: 'method', + overrides: 'testpkg.AbstractClass', + parameters: [{ name: 'param', type: { primitive: 'number' } }], + }, + ], + locationInModule: { filename: 'index.ts', line: 6 }, + name: 'ConcreteClass', + }); +}); + +test('implementing methods use the interface parameter names', async () => { + const assembly = await sourceToAssemblyHelper(` + export interface IInterface { + method(param: number): void; + } + + export class ConcreteClass implements IInterface { + private constructor() {} + + public method(_arg: number): void { + // Nothing to do... + } + } + `); + + expect(assembly.types!['testpkg.IInterface']).toEqual({ + assembly: 'testpkg', + fqn: 'testpkg.IInterface', + kind: 'interface', + methods: [ + { + abstract: true, + locationInModule: { filename: 'index.ts', line: 3 }, + name: 'method', + parameters: [{ name: 'param', type: { primitive: 'number' } }], + }, + ], + locationInModule: { filename: 'index.ts', line: 2 }, + name: 'IInterface', + }); + + expect(assembly.types!['testpkg.ConcreteClass']).toEqual({ + assembly: 'testpkg', + interfaces: ['testpkg.IInterface'], + fqn: 'testpkg.ConcreteClass', + kind: 'class', + methods: [ + { + locationInModule: { filename: 'index.ts', line: 9 }, + name: 'method', + overrides: 'testpkg.IInterface', + parameters: [{ name: 'param', type: { primitive: 'number' } }], + }, + ], + locationInModule: { filename: 'index.ts', line: 6 }, + name: 'ConcreteClass', + }); +});