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: type unions in variadic position leads to invalid code-gen #3722

Merged
merged 1 commit into from
Aug 24, 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
Original file line number Diff line number Diff line change
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());
}
}
}
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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="
}
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
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.

Loading