fix(python): type-checking may require incorrect type (#3820)

Since dae724c, Python type-checking relies on a nested stub function as a type annotations source. The parameter signature of that stub was copied verbatim from the surrounding function, including any forward type references. However, the forward references can safely be replaced with regular type references as the module is guaranteed to be fully loaded by the time the stub is created, and using forward-references there results in `typeguard` possibly evaluating those in a different context than the one where the surrounding function was defined. The consequence of this is that multiple different foward references by the same name may be incorrectly treated as referring to the same type, despite coming from different modules. This is fixed by turning forward type references in the stub with regular type references (in other words, removing any `"` from the parameter signature of the stub), which lets the type be resolved from the local definition context instead of the final runtime context in which the function is called. Fixes #3818 --- 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 2, 2022 · e9d4084 · e9d4084
1 parent 2dd0209
commit e9d4084
Show file tree

Hide file tree

Showing 17 changed files with 2,162 additions and 74 deletions.
diff --git a/.all-contributorsrc b/.all-contributorsrc
@@ -1486,6 +1486,24 @@
       "contributions": [
         "bug"
       ]
+    },
+    {
+      "login": "shamelesscookie",
+      "name": "Christian Moore",
+      "avatar_url": "https://avatars.githubusercontent.com/u/36210509?v=4",
+      "profile": "https://christianmoore.me/",
+      "contributions": [
+        "bug"
+      ]
+    },
+    {
+      "login": "khellan",
+      "name": "Knut O. Hellan",
+      "avatar_url": "https://avatars.githubusercontent.com/u/51441?v=4",
+      "profile": "http://findable.no/",
+      "contributions": [
+        "bug"
+      ]
     }
   ],
   "repoType": "github",

diff --git a/README.md b/README.md
diff --git a/packages/@jsii/python-runtime/tests/test_runtime_type_checking.py b/packages/@jsii/python-runtime/tests/test_runtime_type_checking.py
@@ -172,3 +172,18 @@ def test_variadic(self):
             )
 
         jsii_calc.VariadicTypeUnion()
+
+    def test_homonymous_forward_references(self):
+        """Verifies homonymous forward references don't trip the type checker
+
+        This has been an issue when stub functions were introduced to create a reliable source for type checking
+        information, which was reported in https://github.com/aws/jsii/issues/3818.
+        """
+        # This uses a ForwardRef["Homonymous"] that should resolve to jsii_calc.homonymous_forward_references.foo.Homonymous
+        jsii_calc.homonymous_forward_references.foo.Consumer.consume(
+            homonymous={"string_property": "Check!"}
+        )
+        # This uses a ForwardRef["Homonymous"] that should resolve to jsii_calc.homonymous_forward_references.bar.Homonymous
+        jsii_calc.homonymous_forward_references.bar.Consumer.consume(
+            homonymous={"numeric_property": 1337}
+        )
diff --git a/packages/jsii-calc/lib/homonymous/README.md b/packages/jsii-calc/lib/homonymous/README.md
@@ -0,0 +1,4 @@
+Verifies homonymous forward references don't trip the Python type checker
+
+This has been an issue when stub functions were introduced to create a reliable source for type checking
+information, which was reported in https://github.com/aws/jsii/issues/3818.
diff --git a/packages/jsii-calc/lib/homonymous/bar.ts b/packages/jsii-calc/lib/homonymous/bar.ts
@@ -0,0 +1,15 @@
+export interface Homonymous {
+  readonly numericProperty: number;
+}
+
+export interface ConsumerProps {
+  readonly homonymous: Homonymous;
+}
+
+export class Consumer {
+  public static consume(props: ConsumerProps) {
+    return props.homonymous;
+  }
+
+  private constructor() {}
+}
diff --git a/packages/jsii-calc/lib/homonymous/foo.ts b/packages/jsii-calc/lib/homonymous/foo.ts
@@ -0,0 +1,15 @@
+export interface Homonymous {
+  readonly stringProperty: string;
+}
+
+export interface ConsumerProps {
+  readonly homonymous: Homonymous;
+}
+
+export class Consumer {
+  public static consume(props: ConsumerProps) {
+    return props.homonymous;
+  }
+
+  private constructor() {}
+}
diff --git a/packages/jsii-calc/lib/homonymous/index.ts b/packages/jsii-calc/lib/homonymous/index.ts
@@ -0,0 +1,2 @@
+export * as bar from './bar';
+export * as foo from './foo';
diff --git a/packages/jsii-calc/lib/index.ts b/packages/jsii-calc/lib/index.ts
@@ -26,3 +26,4 @@ export * as jsii3656 from './jsii3656';
 
 export * as anonymous from './anonymous';
 export * as union from './union';
+export * as homonymousForwardReferences from './homonymous';
diff --git a/packages/jsii-calc/test/assembly.jsii b/packages/jsii-calc/test/assembly.jsii
@@ -235,6 +235,30 @@
       },
       "symbolId": "lib/calculator:composition"
     },
+    "jsii-calc.homonymousForwardReferences": {
+      "locationInModule": {
+        "filename": "lib/index.ts",
+        "line": 29
+      },
+      "readme": {
+        "markdown": "Verifies homonymous forward references don't trip the Python type checker\n\nThis has been an issue when stub functions were introduced to create a reliable source for type checking\ninformation, which was reported in https://github.com/aws/jsii/issues/3818.\n"
+      },
+      "symbolId": "lib/homonymous/index:"
+    },
+    "jsii-calc.homonymousForwardReferences.bar": {
+      "locationInModule": {
+        "filename": "lib/homonymous/index.ts",
+        "line": 1
+      },
+      "symbolId": "lib/homonymous/bar:"
+    },
+    "jsii-calc.homonymousForwardReferences.foo": {
+      "locationInModule": {
+        "filename": "lib/homonymous/index.ts",
+        "line": 2
+      },
+      "symbolId": "lib/homonymous/foo:"
+    },
     "jsii-calc.jsii3656": {
       "locationInModule": {
         "filename": "lib/index.ts",
@@ -16208,6 +16232,220 @@
       "namespace": "composition.CompositeOperation",
       "symbolId": "lib/calculator:composition.CompositeOperation.CompositionStringStyle"
     },
+    "jsii-calc.homonymousForwardReferences.bar.Consumer": {
+      "assembly": "jsii-calc",
+      "docs": {
+        "stability": "stable"
+      },
+      "fqn": "jsii-calc.homonymousForwardReferences.bar.Consumer",
+      "kind": "class",
+      "locationInModule": {
+        "filename": "lib/homonymous/bar.ts",
+        "line": 9
+      },
+      "methods": [
+        {
+          "docs": {
+            "stability": "stable"
+          },
+          "locationInModule": {
+            "filename": "lib/homonymous/bar.ts",
+            "line": 10
+          },
+          "name": "consume",
+          "parameters": [
+            {
+              "name": "props",
+              "type": {
+                "fqn": "jsii-calc.homonymousForwardReferences.bar.ConsumerProps"
+              }
+            }
+          ],
+          "returns": {
+            "type": {
+              "fqn": "jsii-calc.homonymousForwardReferences.bar.Homonymous"
+            }
+          },
+          "static": true
+        }
+      ],
+      "name": "Consumer",
+      "namespace": "homonymousForwardReferences.bar",
+      "symbolId": "lib/homonymous/bar:Consumer"
+    },
+    "jsii-calc.homonymousForwardReferences.bar.ConsumerProps": {
+      "assembly": "jsii-calc",
+      "datatype": true,
+      "docs": {
+        "stability": "stable"
+      },
+      "fqn": "jsii-calc.homonymousForwardReferences.bar.ConsumerProps",
+      "kind": "interface",
+      "locationInModule": {
+        "filename": "lib/homonymous/bar.ts",
+        "line": 5
+      },
+      "name": "ConsumerProps",
+      "namespace": "homonymousForwardReferences.bar",
+      "properties": [
+        {
+          "abstract": true,
+          "docs": {
+            "stability": "stable"
+          },
+          "immutable": true,
+          "locationInModule": {
+            "filename": "lib/homonymous/bar.ts",
+            "line": 6
+          },
+          "name": "homonymous",
+          "type": {
+            "fqn": "jsii-calc.homonymousForwardReferences.bar.Homonymous"
+          }
+        }
+      ],
+      "symbolId": "lib/homonymous/bar:ConsumerProps"
+    },
+    "jsii-calc.homonymousForwardReferences.bar.Homonymous": {
+      "assembly": "jsii-calc",
+      "datatype": true,
+      "docs": {
+        "stability": "stable"
+      },
+      "fqn": "jsii-calc.homonymousForwardReferences.bar.Homonymous",
+      "kind": "interface",
+      "locationInModule": {
+        "filename": "lib/homonymous/bar.ts",
+        "line": 1
+      },
+      "name": "Homonymous",
+      "namespace": "homonymousForwardReferences.bar",
+      "properties": [
+        {
+          "abstract": true,
+          "docs": {
+            "stability": "stable"
+          },
+          "immutable": true,
+          "locationInModule": {
+            "filename": "lib/homonymous/bar.ts",
+            "line": 2
+          },
+          "name": "numericProperty",
+          "type": {
+            "primitive": "number"
+          }
+        }
+      ],
+      "symbolId": "lib/homonymous/bar:Homonymous"
+    },
+    "jsii-calc.homonymousForwardReferences.foo.Consumer": {
+      "assembly": "jsii-calc",
+      "docs": {
+        "stability": "stable"
+      },
+      "fqn": "jsii-calc.homonymousForwardReferences.foo.Consumer",
+      "kind": "class",
+      "locationInModule": {
+        "filename": "lib/homonymous/foo.ts",
+        "line": 9
+      },
+      "methods": [
+        {
+          "docs": {
+            "stability": "stable"
+          },
+          "locationInModule": {
+            "filename": "lib/homonymous/foo.ts",
+            "line": 10
+          },
+          "name": "consume",
+          "parameters": [
+            {
+              "name": "props",
+              "type": {
+                "fqn": "jsii-calc.homonymousForwardReferences.foo.ConsumerProps"
+              }
+            }
+          ],
+          "returns": {
+            "type": {
+              "fqn": "jsii-calc.homonymousForwardReferences.foo.Homonymous"
+            }
+          },
+          "static": true
+        }
+      ],
+      "name": "Consumer",
+      "namespace": "homonymousForwardReferences.foo",
+      "symbolId": "lib/homonymous/foo:Consumer"
+    },
+    "jsii-calc.homonymousForwardReferences.foo.ConsumerProps": {
+      "assembly": "jsii-calc",
+      "datatype": true,
+      "docs": {
+        "stability": "stable"
+      },
+      "fqn": "jsii-calc.homonymousForwardReferences.foo.ConsumerProps",
+      "kind": "interface",
+      "locationInModule": {
+        "filename": "lib/homonymous/foo.ts",
+        "line": 5
+      },
+      "name": "ConsumerProps",
+      "namespace": "homonymousForwardReferences.foo",
+      "properties": [
+        {
+          "abstract": true,
+          "docs": {
+            "stability": "stable"
+          },
+          "immutable": true,
+          "locationInModule": {
+            "filename": "lib/homonymous/foo.ts",
+            "line": 6
+          },
+          "name": "homonymous",
+          "type": {
+            "fqn": "jsii-calc.homonymousForwardReferences.foo.Homonymous"
+          }
+        }
+      ],
+      "symbolId": "lib/homonymous/foo:ConsumerProps"
+    },
+    "jsii-calc.homonymousForwardReferences.foo.Homonymous": {
+      "assembly": "jsii-calc",
+      "datatype": true,
+      "docs": {
+        "stability": "stable"
+      },
+      "fqn": "jsii-calc.homonymousForwardReferences.foo.Homonymous",
+      "kind": "interface",
+      "locationInModule": {
+        "filename": "lib/homonymous/foo.ts",
+        "line": 1
+      },
+      "name": "Homonymous",
+      "namespace": "homonymousForwardReferences.foo",
+      "properties": [
+        {
+          "abstract": true,
+          "docs": {
+            "stability": "stable"
+          },
+          "immutable": true,
+          "locationInModule": {
+            "filename": "lib/homonymous/foo.ts",
+            "line": 2
+          },
+          "name": "stringProperty",
+          "type": {
+            "primitive": "string"
+          }
+        }
+      ],
+      "symbolId": "lib/homonymous/foo:Homonymous"
+    },
     "jsii-calc.jsii3656.ImplementMeOpts": {
       "assembly": "jsii-calc",
       "datatype": true,
@@ -18327,5 +18565,5 @@
     }
   },
   "version": "3.20.120",
-  "fingerprint": "O7e7hA2s4dwiCigDIFE0ANjlmeXIQydXdSodi9WHja4="
+  "fingerprint": "b2P7abkCSB3ezfp46ujoSHTYSgRV0o7O1avtvIe5xX8="
 }
diff --git a/packages/jsii-pacmak/lib/targets/python.ts b/packages/jsii-pacmak/lib/targets/python.ts
@@ -3088,7 +3088,18 @@ function emitParameterTypeChecks(
       // 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.
-      openSignature(code, 'def', stubVar, params, 'None');
+      //
+      // 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('...');
       code.closeBlock();