Skip to content

Commit 843ef35

Browse files
chore(python): use builtins.str instead of just str in Dict keys (#3866)
Added an integration test to verify parameters named `builtins` or `str` do not shadow the relevant type names, which used to be problematic until #3848 fixed the overarching issue, hoever the existing test only covered the case where a parameter name shadowed an namespace/import. Additionally, changes various `typing.Dict` keys that were mistakenly spelled out as `str` to use `builtins.str` in order to improve overall consistency in type declarations (see also #3866). Co-authored-by: rmuller@amazon.fr --- 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
1 parent 0b7dcc8 commit 843ef35

14 files changed

Lines changed: 1198 additions & 264 deletions

File tree

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -199,3 +199,16 @@ def test_shadowed_namespaces_are_not_a_problem(self):
199199
num = Number(1337)
200200
# The parameter is named "scope" which shadows the "scope" module...
201201
assert num == subject.use_scope(num)
202+
203+
def test_shadowed_builtins_are_not_a_problem(self):
204+
"""Verifies that a parameter shadowing a built-in name does not cause errors"""
205+
206+
jsii_calc.ParamShadowsBuiltins(
207+
"${not a Python type (builtins)}",
208+
"${not a Python type (str)}",
209+
string_property="Most definitely a string",
210+
boolean_property=True,
211+
struct_property={
212+
"required_string": "Present!",
213+
},
214+
)

packages/jsii-calc/lib/compliance.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3103,3 +3103,31 @@ export class ParamShadowsScope {
31033103
return scope;
31043104
}
31053105
}
3106+
3107+
/**
3108+
* Validate that parameters named "str" or "builtins" do not shadow the actual
3109+
* type names in Python.
3110+
*/
3111+
export class ParamShadowsBuiltins {
3112+
/**
3113+
* @param builtins should be set to something that is NOT a valid expression in Python (e.g: "${NOPE}"")
3114+
* @param str should be set to something that is NOT a valid expression in Python (e.g: "${NOPE}"")
3115+
* @param props should be set to valid values.
3116+
*/
3117+
public constructor(
3118+
builtins: string,
3119+
str: string,
3120+
props: ParamShadowsBuiltinsProps,
3121+
) {
3122+
this.consumeArgs(builtins, str, props);
3123+
}
3124+
3125+
private consumeArgs(..._args: unknown[]) {
3126+
return;
3127+
}
3128+
}
3129+
export interface ParamShadowsBuiltinsProps {
3130+
readonly stringProperty: string;
3131+
readonly booleanProperty: boolean;
3132+
readonly structProperty: StructA;
3133+
}

packages/jsii-calc/test/assembly.jsii

Lines changed: 116 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11432,6 +11432,121 @@
1143211432
"name": "OverrideReturnsObject",
1143311433
"symbolId": "lib/compliance:OverrideReturnsObject"
1143411434
},
11435+
"jsii-calc.ParamShadowsBuiltins": {
11436+
"assembly": "jsii-calc",
11437+
"docs": {
11438+
"stability": "stable",
11439+
"summary": "Validate that parameters named \"str\" or \"builtins\" do not shadow the actual type names in Python."
11440+
},
11441+
"fqn": "jsii-calc.ParamShadowsBuiltins",
11442+
"initializer": {
11443+
"docs": {
11444+
"stability": "stable"
11445+
},
11446+
"locationInModule": {
11447+
"filename": "lib/compliance.ts",
11448+
"line": 3117
11449+
},
11450+
"parameters": [
11451+
{
11452+
"docs": {
11453+
"summary": "should be set to something that is NOT a valid expression in Python (e.g: \"${NOPE}\"\")."
11454+
},
11455+
"name": "builtins",
11456+
"type": {
11457+
"primitive": "string"
11458+
}
11459+
},
11460+
{
11461+
"docs": {
11462+
"summary": "should be set to something that is NOT a valid expression in Python (e.g: \"${NOPE}\"\")."
11463+
},
11464+
"name": "str",
11465+
"type": {
11466+
"primitive": "string"
11467+
}
11468+
},
11469+
{
11470+
"docs": {
11471+
"summary": "should be set to valid values."
11472+
},
11473+
"name": "props",
11474+
"type": {
11475+
"fqn": "jsii-calc.ParamShadowsBuiltinsProps"
11476+
}
11477+
}
11478+
]
11479+
},
11480+
"kind": "class",
11481+
"locationInModule": {
11482+
"filename": "lib/compliance.ts",
11483+
"line": 3111
11484+
},
11485+
"name": "ParamShadowsBuiltins",
11486+
"symbolId": "lib/compliance:ParamShadowsBuiltins"
11487+
},
11488+
"jsii-calc.ParamShadowsBuiltinsProps": {
11489+
"assembly": "jsii-calc",
11490+
"datatype": true,
11491+
"docs": {
11492+
"stability": "stable"
11493+
},
11494+
"fqn": "jsii-calc.ParamShadowsBuiltinsProps",
11495+
"kind": "interface",
11496+
"locationInModule": {
11497+
"filename": "lib/compliance.ts",
11498+
"line": 3129
11499+
},
11500+
"name": "ParamShadowsBuiltinsProps",
11501+
"properties": [
11502+
{
11503+
"abstract": true,
11504+
"docs": {
11505+
"stability": "stable"
11506+
},
11507+
"immutable": true,
11508+
"locationInModule": {
11509+
"filename": "lib/compliance.ts",
11510+
"line": 3131
11511+
},
11512+
"name": "booleanProperty",
11513+
"type": {
11514+
"primitive": "boolean"
11515+
}
11516+
},
11517+
{
11518+
"abstract": true,
11519+
"docs": {
11520+
"stability": "stable"
11521+
},
11522+
"immutable": true,
11523+
"locationInModule": {
11524+
"filename": "lib/compliance.ts",
11525+
"line": 3130
11526+
},
11527+
"name": "stringProperty",
11528+
"type": {
11529+
"primitive": "string"
11530+
}
11531+
},
11532+
{
11533+
"abstract": true,
11534+
"docs": {
11535+
"stability": "stable"
11536+
},
11537+
"immutable": true,
11538+
"locationInModule": {
11539+
"filename": "lib/compliance.ts",
11540+
"line": 3132
11541+
},
11542+
"name": "structProperty",
11543+
"type": {
11544+
"fqn": "jsii-calc.StructA"
11545+
}
11546+
}
11547+
],
11548+
"symbolId": "lib/compliance:ParamShadowsBuiltinsProps"
11549+
},
1143511550
"jsii-calc.ParamShadowsScope": {
1143611551
"assembly": "jsii-calc",
1143711552
"docs": {
@@ -18684,5 +18799,5 @@
1868418799
}
1868518800
},
1868618801
"version": "3.20.120",
18687-
"fingerprint": "rlyZv1z894G2z+Tv8sRGq/ICddvDv3o3auHbiPYCPZI="
18802+
"fingerprint": "kmlc5is+t/xffykk7b4m8jurrxFDkj9pjv2cW/V1J50="
1868818803
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1207,7 +1207,7 @@ class Struct extends BasePythonClassType {
12071207
// Required properties, those will always be put into the dict
12081208
assignDictionary(
12091209
code,
1210-
`${implicitParameter}._values: typing.Dict[str, typing.Any]`,
1210+
`${implicitParameter}._values: typing.Dict[builtins.str, typing.Any]`,
12111211
members
12121212
.filter((m) => !m.optional)
12131213
.map(

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -311,7 +311,7 @@ class UserType implements TypeName {
311311
const wrapType =
312312
typeAnnotation && parameterType && isStruct
313313
? (pyType: string) =>
314-
`typing.Union[${pyType}, typing.Dict[str, typing.Any]]`
314+
`typing.Union[${pyType}, typing.Dict[builtins.str, typing.Any]]`
315315
: (pyType: string) => pyType;
316316

317317
// Emit aliased imports for dependencies (this avoids name collisions)

packages/jsii-pacmak/test/generated-code/__snapshots__/examples.test.js.snap

Lines changed: 4 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)