fix(python): correctly emit sligified positional args (#1081)

In the implementation of kernel calls, the positional argument list did
not use the slugified argument name (in case where such an argument had
a name conflict with a lifted kwarg), resulting in an incorrect argument
list to be passed to `jsii-kernel`. This typically resulted in a type
check error in the `jsii-kernel`, but could also have silently succeeded
at making an incorrect call.

Fixes aws/aws-cdk#4302

packages/@jsii/dotnet-runtime-test/test/Amazon.JSII.Runtime.IntegrationTests/ComplianceTests.cs

@@ -1387,5 +1387,16 @@ public IStructB ReturnStruct()
                 return Struct;
             }
         }
+
+        [Fact(DisplayName = Prefix + nameof(LiftedKwargWithSameNameAsPositionalArg))]
+        public void LiftedKwargWithSameNameAsPositionalArg()
+        {
+            // This is a replication of a test that mostly affects languages with keyword arguments (e.g: Python, Ruby, ...)
+            var bell = new Bell();
+            var amb = new AmbiguousParameters(bell, new StructParameterType { Scope = "Driiiing!" });
+
+            Assert.Equal(bell, amb.Scope);
+            Assert.Equal("Driiiing!", amb.Props.Scope);
+        }
     }
 }

packages/@jsii/java-runtime-test/project/src/test/java/software/amazon/jsii/testing/ComplianceTest.java

@@ -1658,4 +1658,13 @@ public StructB returnStruct() {
             return this.struct;
         }
     }
+
+    @Test
+    public void liftedKwargWithSameNameAsPositionalArg() {
+        // This is a replication of a test that mostly affects languages with keyword arguments (e.g: Python, Ruby, ...)
+        final Bell bell = new Bell();
+        final AmbiguousParameters amb = AmbiguousParameters.Builder.create(bell).scope("Driiiing!").build();
+        assertEquals(bell, amb.getScope());
+        assertEquals(StructParameterType.builder().scope("Driiiing!").build(), amb.getProps());
+    }
 }

packages/@jsii/python-runtime/tests/test_compliance.py

@@ -13,7 +13,9 @@
     Add,
     AllTypes,
     AllTypesEnum,
+    AmbiguousParameters,
     AsyncVirtualMethods,
+    Bell,
     Calculator,
     ClassWithPrivateConstructorAndAutomaticProperties,
     ConfusingToJackson,
@@ -65,6 +67,7 @@
     StructB,
     StructUnionConsumer,
     SomeTypeJsii976,
+    StructParameterType,
     AnonymousImplementationProvider,
     IAnonymousImplementationProvider
 )
@@ -1121,3 +1124,10 @@ def return_struct(self):
     delegate = ImplementsAdditionalInterface()
     consumer = ConsumePureInterface(delegate)
     assert consumer.work_it_baby() == expected
+
+def test_lifted_kwarg_with_same_name_as_positional_arg():
+    bell = Bell()
+    amb = AmbiguousParameters(bell, scope='Driiiing!')
+
+    assert amb.scope == bell
+    assert amb.props == StructParameterType(scope='Driiiing!')

packages/jsii-calc/lib/compliance.ts

@@ -2410,3 +2410,18 @@ export class ConsumePureInterface {
         return this.delegate.returnStruct();
     }
 }
+
+/**
+ * Verifies that, in languages that do keyword lifting (e.g: Python), having a
+ * struct member with the same name as a positional parameter results in the
+ * correct code being emitted.
+ *
+ * See: https://github.com/aws/aws-cdk/issues/4302
+ */
+export interface StructParameterType {
+    readonly scope: string;
+    readonly props?: boolean;
+}
+export class AmbiguousParameters {
+    public constructor(public readonly scope: Bell, public readonly props: StructParameterType) { }
+}

packages/jsii-calc/test/assembly.jsii

@@ -1027,6 +1027,68 @@
       ],
       "name": "AllowedMethodNames"
     },
+    "jsii-calc.AmbiguousParameters": {
+      "assembly": "jsii-calc",
+      "docs": {
+        "stability": "experimental"
+      },
+      "fqn": "jsii-calc.AmbiguousParameters",
+      "initializer": {
+        "docs": {
+          "stability": "experimental"
+        },
+        "parameters": [
+          {
+            "name": "scope",
+            "type": {
+              "fqn": "jsii-calc.Bell"
+            }
+          },
+          {
+            "name": "props",
+            "type": {
+              "fqn": "jsii-calc.StructParameterType"
+            }
+          }
+        ]
+      },
+      "kind": "class",
+      "locationInModule": {
+        "filename": "lib/compliance.ts",
+        "line": 2425
+      },
+      "name": "AmbiguousParameters",
+      "properties": [
+        {
+          "docs": {
+            "stability": "experimental"
+          },
+          "immutable": true,
+          "locationInModule": {
+            "filename": "lib/compliance.ts",
+            "line": 2426
+          },
+          "name": "props",
+          "type": {
+            "fqn": "jsii-calc.StructParameterType"
+          }
+        },
+        {
+          "docs": {
+            "stability": "experimental"
+          },
+          "immutable": true,
+          "locationInModule": {
+            "filename": "lib/compliance.ts",
+            "line": 2426
+          },
+          "name": "scope",
+          "type": {
+            "fqn": "jsii-calc.Bell"
+          }
+        }
+      ]
+    },
     "jsii-calc.AnonymousImplementationProvider": {
       "assembly": "jsii-calc",
       "docs": {
@@ -10049,6 +10111,55 @@
         }
       ]
     },
+    "jsii-calc.StructParameterType": {
+      "assembly": "jsii-calc",
+      "datatype": true,
+      "docs": {
+        "remarks": "See: https://github.com/aws/aws-cdk/issues/4302",
+        "stability": "experimental",
+        "summary": "Verifies that, in languages that do keyword lifting (e.g: Python), having a struct member with the same name as a positional parameter results in the correct code being emitted."
+      },
+      "fqn": "jsii-calc.StructParameterType",
+      "kind": "interface",
+      "locationInModule": {
+        "filename": "lib/compliance.ts",
+        "line": 2421
+      },
+      "name": "StructParameterType",
+      "properties": [
+        {
+          "abstract": true,
+          "docs": {
+            "stability": "experimental"
+          },
+          "immutable": true,
+          "locationInModule": {
+            "filename": "lib/compliance.ts",
+            "line": 2422
+          },
+          "name": "scope",
+          "type": {
+            "primitive": "string"
+          }
+        },
+        {
+          "abstract": true,
+          "docs": {
+            "stability": "experimental"
+          },
+          "immutable": true,
+          "locationInModule": {
+            "filename": "lib/compliance.ts",
+            "line": 2423
+          },
+          "name": "props",
+          "optional": true,
+          "type": {
+            "primitive": "boolean"
+          }
+        }
+      ]
+    },
     "jsii-calc.StructPassing": {
       "assembly": "jsii-calc",
       "docs": {
@@ -11748,5 +11859,5 @@
     }
   },
   "version": "0.20.8",
-  "fingerprint": "sZ6Jp7HsZKHGh8JYB08PYxyFX0bYBgK1FMDhzjtPjVQ="
+  "fingerprint": "4Bqzy6fanRSSO2qQVmaQtsFCqqZVG+7rA/YXuHTbAec="
 }

packages/jsii-pacmak/lib/targets/python.ts

@@ -110,9 +110,26 @@ const toPythonPropertyName = (name: string, constant = false, protectedItem = fa
   return value;
 };
 
-const toPythonParameterName = (name: string): string => {
-  return toPythonIdentifier(toSnakeCase(name));
-};
+/**
+ * Converts a given signature's parameter name to what should be emitted in Python. It slugifies the
+ * positional parameter names that collide with a lifted prop by appending trailing `_`. There is no
+ * risk of conflicting with an other positional parameter that ends with a `_` character because
+ * this is prohibited by the `jsii` compiler (parameter names MUST be camelCase, and only a single
+ * `_` is permitted when it is on **leading** position)
+ *
+ * @param name              the name of the parameter that needs conversion.
+ * @param liftedParamNames  the list of "lifted" keyword parameters in this signature. This must be
+ *                          omitted when generating a name for a parameter that **is** lifted.
+ */
+function toPythonParameterName(name: string, liftedParamNames = new Set<string>()): string {
+  let result = toPythonIdentifier(toSnakeCase(name));
+
+  while (liftedParamNames.has(result)) {
+    result += '_';
+  }
+
+  return result;
+}
 
 const setDifference = (setA: Set<any>, setB: Set<any>): Set<any> => {
   const difference = new Set(setA);
@@ -339,18 +356,18 @@ abstract class BaseMethod implements PythonBase {
       // initializers, because our keyword lifting will allow two names to clash.
       // This can hopefully be removed once we get https://github.com/aws/jsii/issues/288
       // resolved.
-      let paramName: string = toPythonParameterName(param.name);
-      while (liftedPropNames.has(paramName)) {
-        paramName = `${paramName}_`;
-      }
+      const paramName: string = toPythonParameterName(param.name, liftedPropNames);
 
       const paramType = resolver.resolve(param, { forwardReferences: false });
       const paramDefault = param.optional ? '=None' : '';
 
       pythonParams.push(`${paramName}: ${paramType}${paramDefault}`);
     }
 
-    const documentableArgs = [...this.parameters];
+    const documentableArgs = this.parameters
+      // If there's liftedProps, the last argument is the struct and it won't be _actually_ emitted.
+      .filter((_, index) => this.liftedProp != null ? index < this.parameters.length - 1 : true)
+      .map(param => ({ ...param, name: toPythonParameterName(param.name, liftedPropNames) }));
 
     // If we have a lifted parameter, then we'll drop the last argument to our params
     // and then we'll lift all of the params of the lifted type as keyword arguments
@@ -407,25 +424,31 @@ abstract class BaseMethod implements PythonBase {
 
     code.openBlock(`def ${this.pythonName}(${pythonParams.join(', ')}) -> ${returnType}`);
     this.generator.emitDocString(code, this.docs, { arguments: documentableArgs, documentableItem: `method-${this.pythonName}` });
-    this.emitBody(code, resolver, renderAbstract, forceEmitBody);
+    this.emitBody(code, resolver, renderAbstract, forceEmitBody, liftedPropNames);
     code.closeBlock();
   }
 
-  private emitBody(code: CodeMaker, resolver: TypeResolver, renderAbstract: boolean, forceEmitBody: boolean) {
+  private emitBody(
+    code: CodeMaker,
+    resolver: TypeResolver,
+    renderAbstract: boolean,
+    forceEmitBody: boolean,
+    liftedPropNames: Set<string>
+  ) {
     if ((!this.shouldEmitBody && !forceEmitBody) || (renderAbstract && this.abstract)) {
       code.line('...');
     } else {
       if (this.liftedProp !== undefined) {
-        this.emitAutoProps(code, resolver);
+        this.emitAutoProps(code, resolver, liftedPropNames);
       }
 
-      this.emitJsiiMethodCall(code, resolver);
+      this.emitJsiiMethodCall(code, resolver, liftedPropNames);
     }
   }
 
-  private emitAutoProps(code: CodeMaker, resolver: TypeResolver) {
+  private emitAutoProps(code: CodeMaker, resolver: TypeResolver, liftedPropNames: Set<string>) {
     const lastParameter = this.parameters.slice(-1)[0];
-    const argName = toPythonParameterName(lastParameter.name);
+    const argName = toPythonParameterName(lastParameter.name, liftedPropNames);
     const typeName = resolver.resolve(lastParameter, { ignoreOptional: true });
 
     // We need to build up a list of properties, which are mandatory, these are the
@@ -439,7 +462,7 @@ abstract class BaseMethod implements PythonBase {
     code.line();
   }
 
-  private emitJsiiMethodCall(code: CodeMaker, resolver: TypeResolver) {
+  private emitJsiiMethodCall(code: CodeMaker, resolver: TypeResolver, liftedPropNames: Set<string>) {
     const methodPrefix: string = this.returnFromJSIIMethod ? 'return ' : '';
 
     const jsiiMethodParams: string[] = [];
@@ -457,7 +480,7 @@ abstract class BaseMethod implements PythonBase {
     // If the last arg is variadic, expand the tuple
     const params: string[] = [];
     for (const param of this.parameters) {
-      let expr = toPythonParameterName(param.name);
+      let expr = toPythonParameterName(param.name, liftedPropNames);
       if (param.variadic) { expr = `*${expr}`; }
       params.push(expr);
     }
@@ -1502,10 +1525,14 @@ class PythonGenerator extends Generator {
     this.types = new Map();
   }
 
-  public emitDocString(code: CodeMaker, docs: spec.Docs | undefined, options: {
-    arguments?: DocumentableArgument[];
-    documentableItem?: string;
-  } = {}) {
+  public emitDocString(
+    code: CodeMaker,
+    docs: spec.Docs | undefined,
+    options: {
+      arguments?: DocumentableArgument[];
+      documentableItem?: string;
+    } = {}
+  ) {
     if ((!docs || Object.keys(docs).length === 0) && !options.arguments) { return; }
     if (!docs) { docs = {}; }