fix(python): parameter names shadow imported modules (#3848)

In certain cases, a function parameter may shadow an imported module name, resultin in run-time errors either during type-checking or during the type-cast that is performed before returning a kernel call's result. This change adds a test case that covers this particular scenario in Python, and changes how foreign modules are imported so that an alias is always generated for those, removing the risk for collisions. It also moves the type-checking stubs out to the root of the module to remove the risk of more "runtime context" polluting the type evaluation. Fixes aws/aws-cdk#22975 --- By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license]. [Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0
aws · Nov 22, 2022 · 8bac012 · 8bac012
1 parent 6d20e27
commit 8bac012
Show file tree

Hide file tree

Showing 15 changed files with 3,267 additions and 1,601 deletions.
diff --git a/packages/@jsii/python-runtime/tests/test_runtime_type_checking.py b/packages/@jsii/python-runtime/tests/test_runtime_type_checking.py
@@ -2,6 +2,7 @@
 import re
 
 from scope.jsii_calc_lib.custom_submodule_name import NestingClass
+from scope.jsii_calc_lib import Number
 import jsii_calc
 
 
@@ -187,3 +188,14 @@ def test_homonymous_forward_references(self):
         jsii_calc.homonymous_forward_references.bar.Consumer.consume(
             homonymous={"numeric_property": 1337}
         )
+
+    def test_shadowed_namespaces_are_not_a_problem(self):
+        """Verifies that a parameter shadowing a namespace does not cause errors
+
+        This has caused https://github.com/aws/aws-cdk/issues/22975.
+        """
+
+        subject = jsii_calc.ParamShadowsScope()
+        num = Number(1337)
+        # The parameter is named "scope" which shadows the "scope" module...
+        assert num == subject.use_scope(num)
diff --git a/packages/jsii-calc/lib/compliance.ts b/packages/jsii-calc/lib/compliance.ts
@@ -3090,3 +3090,16 @@ export class VariadicTypeUnion {
     this.union = union;
   }
 }
+
+/**
+ * Validate that namespaces being shadowed by local variables does not cause
+ * type checking issues.
+ *
+ * @see https://github.com/aws/aws-cdk/issues/22975
+ */
+export class ParamShadowsScope {
+  // @scope/* packages are under the "scope." namespace in Python.
+  public useScope(scope: LibNumber) {
+    return scope;
+  }
+}
diff --git a/packages/jsii-calc/test/assembly.jsii b/packages/jsii-calc/test/assembly.jsii
@@ -11425,6 +11425,52 @@
       "name": "OverrideReturnsObject",
       "symbolId": "lib/compliance:OverrideReturnsObject"
     },
+    "jsii-calc.ParamShadowsScope": {
+      "assembly": "jsii-calc",
+      "docs": {
+        "see": "https://github.com/aws/aws-cdk/issues/22975",
+        "stability": "stable",
+        "summary": "Validate that namespaces being shadowed by local variables does not cause type checking issues."
+      },
+      "fqn": "jsii-calc.ParamShadowsScope",
+      "initializer": {
+        "docs": {
+          "stability": "stable"
+        }
+      },
+      "kind": "class",
+      "locationInModule": {
+        "filename": "lib/compliance.ts",
+        "line": 3100
+      },
+      "methods": [
+        {
+          "docs": {
+            "stability": "stable"
+          },
+          "locationInModule": {
+            "filename": "lib/compliance.ts",
+            "line": 3102
+          },
+          "name": "useScope",
+          "parameters": [
+            {
+              "name": "scope",
+              "type": {
+                "fqn": "@scope/jsii-calc-lib.Number"
+              }
+            }
+          ],
+          "returns": {
+            "type": {
+              "fqn": "@scope/jsii-calc-lib.Number"
+            }
+          }
+        }
+      ],
+      "name": "ParamShadowsScope",
+      "symbolId": "lib/compliance:ParamShadowsScope"
+    },
     "jsii-calc.ParentStruct982": {
       "assembly": "jsii-calc",
       "datatype": true,
@@ -18565,5 +18611,5 @@
     }
   },
   "version": "3.20.120",
-  "fingerprint": "b2P7abkCSB3ezfp46ujoSHTYSgRV0o7O1avtvIe5xX8="
+  "fingerprint": "qTr3P5AD9cBorxBMjHTT6y6sMYWUgGf/acN7eejGTgQ="
 }
diff --git a/packages/jsii-pacmak/lib/targets/python.ts b/packages/jsii-pacmak/lib/targets/python.ts
@@ -1,6 +1,7 @@
 import * as spec from '@jsii/spec';
 import * as assert from 'assert';
 import { CodeMaker, toSnakeCase } from 'codemaker';
+import * as crypto from 'crypto';
 import * as escapeStringRegexp from 'escape-string-regexp';
 import * as fs from 'fs-extra';
 import * as reflect from 'jsii-reflect';
@@ -125,6 +126,57 @@ interface EmitContext extends NamingContext {
 
   /** Whether to runtime type check keyword arguments (i.e: struct constructors) */
   readonly runtimeTypeCheckKwargs?: boolean;
+
+  /** The numerical IDs used for type annotation data storing */
+  readonly typeCheckingHelper: TypeCheckingHelper;
+}
+
+class TypeCheckingHelper {
+  #stubs = new Array<TypeCheckingStub>();
+
+  public getTypeHints(fqn: string, args: readonly string[]): string {
+    const stub = new TypeCheckingStub(fqn, args);
+    this.#stubs.push(stub);
+    return `typing.get_type_hints(${stub.name})`;
+  }
+
+  /** Emits instructions that create the annotations data... */
+  public flushStubs(code: CodeMaker) {
+    for (const stub of this.#stubs) {
+      stub.emit(code);
+    }
+    // Reset the stubs list
+    this.#stubs = [];
+  }
+}
+
+class TypeCheckingStub {
+  static readonly #PREFIX = '_typecheckingstub__';
+
+  readonly #arguments: readonly string[];
+  readonly #hash: string;
+
+  public constructor(fqn: string, args: readonly string[]) {
+    // Removing the quoted type names -- this will be emitted at the very end of the module.
+    this.#arguments = args.map((arg) => arg.replace(/"/g, ''));
+    this.#hash = crypto
+      .createHash('sha256')
+      .update(TypeCheckingStub.#PREFIX)
+      .update(fqn)
+      .digest('hex');
+  }
+
+  public get name(): string {
+    return `${TypeCheckingStub.#PREFIX}${this.#hash}`;
+  }
+
+  public emit(code: CodeMaker) {
+    code.line();
+    openSignature(code, 'def', this.name, this.#arguments, 'None');
+    code.line(`"""Type checking stubs"""`);
+    code.line('pass');
+    code.closeBlock();
+  }
 }
 
 const pythonModuleNameToFilename = (name: string): string => {
@@ -472,6 +524,7 @@ abstract class BaseMethod implements PythonBase {
     private readonly returns: spec.OptionalValue | undefined,
     public readonly docs: spec.Docs | undefined,
     public readonly isStatic: boolean,
+    private readonly pythonParent: PythonType,
     opts: BaseMethodOpts,
   ) {
     this.abstract = !!opts.abstract;
@@ -666,7 +719,14 @@ abstract class BaseMethod implements PythonBase {
       (this.shouldEmitBody || forceEmitBody) &&
       (!renderAbstract || !this.abstract)
     ) {
-      emitParameterTypeChecks(code, context, pythonParams.slice(1));
+      emitParameterTypeChecks(
+        code,
+        context,
+        pythonParams.slice(1),
+        `${this.pythonParent.fqn ?? this.pythonParent.pythonName}#${
+          this.pythonName
+        }`,
+      );
     }
     this.emitBody(
       code,
@@ -858,6 +918,7 @@ abstract class BaseProperty implements PythonBase {
     private readonly jsName: string,
     private readonly type: spec.OptionalValue,
     public readonly docs: spec.Docs | undefined,
+    private readonly pythonParent: PythonType,
     opts: BasePropertyOpts,
   ) {
     const { abstract = false, immutable = false, isStatic = false } = opts;
@@ -940,7 +1001,14 @@ abstract class BaseProperty implements PythonBase {
         (this.shouldEmitBody || forceEmitBody) &&
         (!renderAbstract || !this.abstract)
       ) {
-        emitParameterTypeChecks(code, context, [`value: ${pythonType}`]);
+        emitParameterTypeChecks(
+          code,
+          context,
+          [`value: ${pythonType}`],
+          `${this.pythonParent.fqn ?? this.pythonParent.pythonName}#${
+            this.pythonName
+          }`,
+        );
         code.line(
           `jsii.${this.jsiiSetMethod}(${this.implicitParameter}, "${this.jsName}", value)`,
         );
@@ -1132,6 +1200,7 @@ class Struct extends BasePythonClassType {
         // Runtime type check keyword args as this is a struct __init__ function.
         { ...context, runtimeTypeCheckKwargs: true },
         ['*', ...kwargs],
+        `${this.fqn ?? this.pythonName}#__init__`,
       );
     }
 
@@ -1965,6 +2034,7 @@ class Package {
 
       code.openFile(filename);
       mod.emit(code, context);
+      context.typeCheckingHelper.flushStubs(code);
       code.closeFile(filename);
 
       scripts.push(...mod.emitBinScripts(code));
@@ -2530,6 +2600,7 @@ class PythonGenerator extends Generator {
       resolver,
       runtimeTypeChecking: this.runtimeTypeChecking,
       submodule: assm.name,
+      typeCheckingHelper: new TypeCheckingHelper(),
       typeResolver: (fqn) => resolver.dereference(fqn),
     });
   }
@@ -2605,6 +2676,7 @@ class PythonGenerator extends Generator {
           undefined,
           cls.initializer.docs,
           false, // Never static
+          klass,
           { liftedProp: this.getliftedProp(cls.initializer), parent: cls },
         ),
       );
@@ -2627,6 +2699,7 @@ class PythonGenerator extends Generator {
         method.returns,
         method.docs,
         true, // Always static
+        klass,
         {
           abstract: method.abstract,
           liftedProp: this.getliftedProp(method),
@@ -2645,6 +2718,7 @@ class PythonGenerator extends Generator {
         prop.name,
         prop,
         prop.docs,
+        klass,
         {
           abstract: prop.abstract,
           immutable: prop.immutable,
@@ -2670,6 +2744,7 @@ class PythonGenerator extends Generator {
           method.returns,
           method.docs,
           !!method.static,
+          klass,
           {
             abstract: method.abstract,
             liftedProp: this.getliftedProp(method),
@@ -2687,6 +2762,7 @@ class PythonGenerator extends Generator {
           method.returns,
           method.docs,
           !!method.static,
+          klass,
           {
             abstract: method.abstract,
             liftedProp: this.getliftedProp(method),
@@ -2706,6 +2782,7 @@ class PythonGenerator extends Generator {
         prop.name,
         prop,
         prop.docs,
+        klass,
         {
           abstract: prop.abstract,
           immutable: prop.immutable,
@@ -2767,6 +2844,7 @@ class PythonGenerator extends Generator {
         method.returns,
         method.docs,
         !!method.static,
+        klass,
         { liftedProp: this.getliftedProp(method), parent: ifc },
       ),
     );
@@ -2786,6 +2864,7 @@ class PythonGenerator extends Generator {
         prop.name,
         prop,
         prop.docs,
+        klass,
         { immutable: prop.immutable, isStatic: prop.static, parent: ifc },
       );
     }
@@ -3043,14 +3122,16 @@ function openSignature(
  * @param code        the CodeMaker to use for emitting code.
  * @param context     the emit context used when emitting this code.
  * @param params      the parameter signatures to be type-checked.
+ * @params pythonName the name of the Python function being checked (qualified).
  */
 function emitParameterTypeChecks(
   code: CodeMaker,
   context: EmitContext,
   params: readonly string[],
-): void {
+  fqn: string,
+): boolean {
   if (!context.runtimeTypeChecking) {
-    return;
+    return false;
   }
 
   const paramInfo = params.map((param) => {
@@ -3082,28 +3163,9 @@ function emitParameterTypeChecks(
 
     if (!openedBlock) {
       code.openBlock('if __debug__');
-      const stubVar = slugifyAsNeeded('stub', [...paramNames, typesVar]);
-      // Inline a stub function to be able to have the required type hints regardless of what customers do with the
-      // code. Using a reference to the `Type.function` may result in incorrect data if some function was replaced (e.g.
-      // by a decorated version with different type annotations). We also cannot construct the actual value expected by
-      // typeguard's `check_type` because Python does not expose the APIs necessary to build many of these objects in
-      // regular Python code.
-      //
-      // Since the nesting function will only be callable once this module is fully loaded, we can convert forward type
-      // references into regular references, so that the type checker is not confused by multiple type references
-      // sharing the same leaf type name (the ForwardRef resolution may be cached in the execution scope, which causes
-      // order-of-initialization problems, as can be seen in aws/jsii#3818).
-      openSignature(
-        code,
-        'def',
-        stubVar,
-        params.map((param) => param.replace(/"/g, '')),
-        'None',
+      code.line(
+        `${typesVar} = ${context.typeCheckingHelper.getTypeHints(fqn, params)}`,
       );
-      code.line('...');
-      code.closeBlock();
-
-      code.line(`${typesVar} = typing.get_type_hints(${stubVar})`);
       openedBlock = true;
     }
 
@@ -3123,7 +3185,10 @@ function emitParameterTypeChecks(
   }
   if (openedBlock) {
     code.closeBlock();
+    return true;
   }
+  // We did not reference type annotations data if we never opened a type-checking block.
+  return false;
 }
 
 function assignCallResult(

diff --git a/packages/jsii-pacmak/lib/targets/python/type-name.ts b/packages/jsii-pacmak/lib/targets/python/type-name.ts
@@ -314,20 +314,30 @@ class UserType implements TypeName {
             `typing.Union[${pyType}, typing.Dict[str, typing.Any]]`
         : (pyType: string) => pyType;
 
+    // Emit aliased imports for dependencies (this avoids name collisions)
     if (assemblyName !== assembly.name) {
+      const aliasSuffix = createHash('sha256')
+        .update(assemblyName)
+        .update('.*')
+        .digest('hex')
+        .substring(0, 8);
+      const alias = `_${packageName.replace(/\./g, '_')}_${aliasSuffix}`;
+
+      const aliasedFqn = `${alias}${pythonFqn.slice(packageName.length)}`;
+
       return {
         // If it's a struct, then we allow passing as a dict, too...
-        pythonType: wrapType(pythonFqn),
+        pythonType: wrapType(aliasedFqn),
         requiredImport: {
-          sourcePackage: packageName,
+          sourcePackage: `${packageName} as ${alias}`,
           item: '',
         },
       };
     }
 
     const submodulePythonName = toPythonFqn(submodule, assembly).pythonFqn;
     const typeSubmodulePythonName = toPythonFqn(
-      findParentSubmodule(assembly.types![this.#fqn], assembly),
+      findParentSubmodule(type, assembly),
       assembly,
     ).pythonFqn;