Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Nov 22, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions packages/@jsii/python-runtime/tests/test_runtime_type_checking.py
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand Down Expand Up @@ -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)
13 changes: 13 additions & 0 deletions packages/jsii-calc/lib/compliance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
48 changes: 47 additions & 1 deletion packages/jsii-calc/test/assembly.jsii
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -18565,5 +18611,5 @@
}
},
"version": "3.20.120",
"fingerprint": "b2P7abkCSB3ezfp46ujoSHTYSgRV0o7O1avtvIe5xX8="
"fingerprint": "qTr3P5AD9cBorxBMjHTT6y6sMYWUgGf/acN7eejGTgQ="
}
115 changes: 90 additions & 25 deletions packages/jsii-pacmak/lib/targets/python.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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 => {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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)`,
);
Expand Down Expand Up @@ -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__`,
);
}

Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -2530,6 +2600,7 @@ class PythonGenerator extends Generator {
resolver,
runtimeTypeChecking: this.runtimeTypeChecking,
submodule: assm.name,
typeCheckingHelper: new TypeCheckingHelper(),
typeResolver: (fqn) => resolver.dereference(fqn),
});
}
Expand Down Expand Up @@ -2605,6 +2676,7 @@ class PythonGenerator extends Generator {
undefined,
cls.initializer.docs,
false, // Never static
klass,
{ liftedProp: this.getliftedProp(cls.initializer), parent: cls },
),
);
Expand All @@ -2627,6 +2699,7 @@ class PythonGenerator extends Generator {
method.returns,
method.docs,
true, // Always static
klass,
{
abstract: method.abstract,
liftedProp: this.getliftedProp(method),
Expand All @@ -2645,6 +2718,7 @@ class PythonGenerator extends Generator {
prop.name,
prop,
prop.docs,
klass,
{
abstract: prop.abstract,
immutable: prop.immutable,
Expand All @@ -2670,6 +2744,7 @@ class PythonGenerator extends Generator {
method.returns,
method.docs,
!!method.static,
klass,
{
abstract: method.abstract,
liftedProp: this.getliftedProp(method),
Expand All @@ -2687,6 +2762,7 @@ class PythonGenerator extends Generator {
method.returns,
method.docs,
!!method.static,
klass,
{
abstract: method.abstract,
liftedProp: this.getliftedProp(method),
Expand All @@ -2706,6 +2782,7 @@ class PythonGenerator extends Generator {
prop.name,
prop,
prop.docs,
klass,
{
abstract: prop.abstract,
immutable: prop.immutable,
Expand Down Expand Up @@ -2767,6 +2844,7 @@ class PythonGenerator extends Generator {
method.returns,
method.docs,
!!method.static,
klass,
{ liftedProp: this.getliftedProp(method), parent: ifc },
),
);
Expand All @@ -2786,6 +2864,7 @@ class PythonGenerator extends Generator {
prop.name,
prop,
prop.docs,
klass,
{ immutable: prop.immutable, isStatic: prop.static, parent: ifc },
);
}
Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -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;
}

Expand All @@ -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(
Expand Down
16 changes: 13 additions & 3 deletions packages/jsii-pacmak/lib/targets/python/type-name.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
Loading