Skip to content

Commit

Permalink
fix: type unions in variadic position leads to invalid code-gen (#3722)
Browse files Browse the repository at this point in the history
The code-gen for type unions was incorrect, as the value is typically
array-typed, and the generated code hence needs to treat the value as
such.
  • Loading branch information
RomainMuller committed Aug 24, 2022
1 parent c73c2ee commit 93aec85
Show file tree
Hide file tree
Showing 14 changed files with 644 additions and 286 deletions.
Expand Up @@ -97,5 +97,18 @@ public void NestedUnion()
}));
Assert.Equal("Expected argument unionProperty[0][\"bad\"] to be one of: Amazon.JSII.Tests.CalculatorNamespace.IStructA, Amazon.JSII.Tests.CalculatorNamespace.IStructB; received System.String (Parameter 'unionProperty')", exception3.Message);
}

[Fact(DisplayName = Prefix + nameof(Variadic))]
public void Variadic()
{
var exception1 = Assert.Throws<System.ArgumentException>(() =>
new VariadicTypeUnion(
new StructA{RequiredString = "present"},
1337.42
));
Assert.Equal("Expected argument union[1] to be one of: Amazon.JSII.Tests.CalculatorNamespace.IStructA, Amazon.JSII.Tests.CalculatorNamespace.IStructB; received System.Double (Parameter 'union')", exception1.Message);

Assert.NotNull(new VariadicTypeUnion());
}
}
}
Expand Up @@ -79,6 +79,16 @@ func TestNestedUnion(t *testing.T) {
}()
}

func TestVariadic(t *testing.T) {
func() {
defer expectPanic(t, "parameter union[1] must be one of the allowed types: *StructA, *StructB; received 1337.42 (a float64)")
jsiicalc.NewVariadicTypeUnion(jsiicalc.StructA{RequiredString: jsii.String("present")}, 1337.42)
}()

// Should not raise
jsiicalc.NewVariadicTypeUnion()
}

func expectPanic(t *testing.T, expected string) {
if err := recover(); err != nil {
actual := fmt.Sprintf("%v", err)
Expand Down
22 changes: 22 additions & 0 deletions packages/@jsii/python-runtime/tests/test_runtime_type_checking.py
Expand Up @@ -126,3 +126,25 @@ def test_anonymous_object(self):

iface = jsii_calc.anonymous.UseOptions.provide("A")
assert jsii_calc.anonymous.UseOptions.consume(iface) == "A"

def test_nested_union(self):
with pytest.raises(
TypeError,
match=re.escape(
"type of argument union_property[0] must be one of (Mapping[str, Union[jsii_calc.StructA, Dict[str, Any], jsii_calc.StructB]], Sequence[Union[jsii_calc.StructA, Dict[str, Any], jsii_calc.StructB]]); got float instead"
),
):
jsii_calc.ClassWithNestedUnion([1337.42]) # type:ignore

def test_variadic(self):
with pytest.raises(
TypeError,
match=re.escape(
"type of argument union[1] must be one of (jsii_calc.StructA, jsii_calc.StructB); got float instead"
),
):
jsii_calc.VariadicTypeUnion(
jsii_calc.StructA(required_string="present"), 1337.42 # type:ignore
)

jsii_calc.VariadicTypeUnion()
8 changes: 8 additions & 0 deletions packages/jsii-calc/lib/compliance.ts
Expand Up @@ -3082,3 +3082,11 @@ export class ClassWithNestedUnion {
>,
) {}
}

export class VariadicTypeUnion {
public union: Array<StructA | StructB>;

public constructor(...union: Array<StructA | StructB>) {
this.union = union;
}
}
73 changes: 72 additions & 1 deletion packages/jsii-calc/test/assembly.jsii
Expand Up @@ -15428,6 +15428,77 @@
"name": "VariadicMethod",
"symbolId": "lib/compliance:VariadicMethod"
},
"jsii-calc.VariadicTypeUnion": {
"assembly": "jsii-calc",
"docs": {
"stability": "stable"
},
"fqn": "jsii-calc.VariadicTypeUnion",
"initializer": {
"docs": {
"stability": "stable"
},
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 3089
},
"parameters": [
{
"name": "union",
"type": {
"union": {
"types": [
{
"fqn": "jsii-calc.StructA"
},
{
"fqn": "jsii-calc.StructB"
}
]
}
},
"variadic": true
}
],
"variadic": true
},
"kind": "class",
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 3086
},
"name": "VariadicTypeUnion",
"properties": [
{
"docs": {
"stability": "stable"
},
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 3087
},
"name": "union",
"type": {
"collection": {
"elementtype": {
"union": {
"types": [
{
"fqn": "jsii-calc.StructA"
},
{
"fqn": "jsii-calc.StructB"
}
]
}
},
"kind": "array"
}
}
}
],
"symbolId": "lib/compliance:VariadicTypeUnion"
},
"jsii-calc.VirtualMethodPlayground": {
"assembly": "jsii-calc",
"docs": {
Expand Down Expand Up @@ -18131,5 +18202,5 @@
}
},
"version": "3.20.120",
"fingerprint": "LBLJQQycukWu6zWQmp2/IbKS/Sfd+4e2zWrX+1KA+Aw="
"fingerprint": "Ze43eowG9ImRufT3MQ8yO+bW8JzOQlZIYtFsjpc960E="
}
@@ -1,3 +1,4 @@
import { CollectionKind } from '@jsii/spec';
import { CodeMaker } from 'codemaker';
import { createHash } from 'crypto';
import { Parameter, TypeReference } from 'jsii-reflect';
Expand Down Expand Up @@ -27,7 +28,14 @@ export class ParameterValidator {
argName,
expr,
`${noMangle ? '' : 'argument '}{${argName}}`,
param.type,
param.variadic
? new TypeReference(param.system, {
collection: {
kind: CollectionKind.Array,
elementtype: param.type.spec!,
},
})
: param.type,
param.optional,
);
if (validation) {
Expand Down
Expand Up @@ -83,13 +83,15 @@ export class ParameterValidator {
const descr = `parameter ${param.name}`;

const validations = new Array<Validation>();
if (!param.isOptional) {
if (!param.isOptional && !param.isVariadic) {
validations.push(Validation.nullCheck(expr, descr, param.reference));
}
const validation = Validation.forTypeMap(
expr,
descr,
param.reference.typeMap,
param.isVariadic
? { type: 'array', value: param.reference }
: param.reference.typeMap,
);
if (validation) {
validations.push(validation);
Expand Down Expand Up @@ -144,7 +146,9 @@ export class ParameterValidator {

public emitCall(code: CodeMaker): void {
const recv = this.receiver?.name ? `${this.receiver.name}.` : '';
const params = this.parameters.map((p) => p.name).join(', ');
const params = this.parameters
.map((p) => (p.isVariadic ? `&${p.name}` : p.name))
.join(', ');

code.openBlock(`if err := ${recv}${this.name}(${params}); err != nil`);
code.line(`panic(err)`);
Expand All @@ -162,7 +166,7 @@ export class ParameterValidator {
}${this.name}(${this.parameters
.map((p) =>
p.isVariadic
? `${p.name} []${p.reference.scopedReference(scope)}`
? `${p.name} *[]${p.reference.scopedReference(scope)}`
: p.toString(),
)
.join(', ')}) error`,
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 93aec85

Please sign in to comment.