From e4344f6fa6c42b544714a943077d4e88603b374a Mon Sep 17 00:00:00 2001 From: Luc Talatinian Date: Wed, 22 Jan 2025 18:24:21 -0500 Subject: [PATCH 1/3] coerce nil numbers to 0 in jmespath codegen --- .../GoJmespathExpressionGenerator.java | 27 +++++++- .../GoJmespathExpressionGeneratorTest.java | 62 +++++++++++++++++++ 2 files changed, 88 insertions(+), 1 deletion(-) diff --git a/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/GoJmespathExpressionGenerator.java b/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/GoJmespathExpressionGenerator.java index ec11c078..a36d5242 100644 --- a/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/GoJmespathExpressionGenerator.java +++ b/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/GoJmespathExpressionGenerator.java @@ -15,6 +15,7 @@ package software.amazon.smithy.go.codegen; +import static software.amazon.smithy.go.codegen.GoWriter.emptyGoTemplate; import static software.amazon.smithy.go.codegen.GoWriter.goTemplate; import static software.amazon.smithy.go.codegen.SymbolUtils.isNilable; import static software.amazon.smithy.go.codegen.SymbolUtils.isPointable; @@ -407,8 +408,30 @@ private GoWriter.Writable compareVariables(String ident, Variable left, Variable return goTemplate("$1L := $5L($2L) $4L $5L($3L)", ident, left.ident, right.ident, cmp, cast); } + // undocumented jmespath behavior: null in numeric comparisons coerces to 0 + // this means the subsequent nil checks for numerics are moot, but it's either this or branch the codegen even + // further for questionable benefit + var nilCoerceLeft = emptyGoTemplate(); + var nilCoerceRight = emptyGoTemplate(); + if (isLPtr && left.shape instanceof NumberShape) { + nilCoerceLeft = goTemplate(""" + if ($1L == nil) { + $1L = new($2T) + *$1L = 0 + }""", left.ident, left.type); + } + if (isRPtr && right.shape instanceof NumberShape) { + nilCoerceRight = goTemplate(""" + if ($1L == nil) { + $1L = new($2T) + *$1L = 0 + }""", right.ident, right.type); + } + return goTemplate(""" var $ident:L bool + $nilCoerceLeft:W + $nilCoerceRight:W if $lif:L $amp:L $rif:L { $ident:L = $cast:L($lhs:L) $cmp:L $cast:L($rhs:L) }""", @@ -420,7 +443,9 @@ private GoWriter.Writable compareVariables(String ident, Variable left, Variable "cmp", cmp, "lhs", isLPtr ? "*" + left.ident : left.ident, "rhs", isRPtr ? "*" + right.ident : right.ident, - "cast", cast + "cast", cast, + "nilCoerceLeft", nilCoerceLeft, + "nilCoerceRight", nilCoerceRight )); } diff --git a/codegen/smithy-go-codegen/src/test/java/software/amazon/smithy/go/codegen/GoJmespathExpressionGeneratorTest.java b/codegen/smithy-go-codegen/src/test/java/software/amazon/smithy/go/codegen/GoJmespathExpressionGeneratorTest.java index 764c3537..336ebfce 100644 --- a/codegen/smithy-go-codegen/src/test/java/software/amazon/smithy/go/codegen/GoJmespathExpressionGeneratorTest.java +++ b/codegen/smithy-go-codegen/src/test/java/software/amazon/smithy/go/codegen/GoJmespathExpressionGeneratorTest.java @@ -43,6 +43,8 @@ public class GoJmespathExpressionGeneratorTest { objectList: ObjectList objectMap: ObjectMap nested: NestedStruct + nullableIntegerA: Integer + nullableIntegerB: Integer } structure Object { @@ -318,6 +320,7 @@ public void testComparatorStringLHSNil() { } v4 := "foo" var v5 bool + if v2 != nil { v5 = string(*v2) == string(v4) } @@ -345,6 +348,7 @@ public void testComparatorStringRHSNil() { v3 = v4 } var v5 bool + if v3 != nil { v5 = string(v1) == string(*v3) } @@ -372,6 +376,7 @@ public void testComparatorStringBothNil() { } v4 := input.SimpleShape var v5 bool + if v2 != nil && v4 != nil { v5 = string(*v2) == string(*v4) } @@ -546,4 +551,61 @@ public void testMultiSelectFlatten() { } """)); } + + @Test + public void testComparatorNumberCoercesLeftNullable() { + var expr = "nullableIntegerA > `9`"; + + var writer = testWriter(); + var generator = new GoJmespathExpressionGenerator(testContext(), writer); + var actual = generator.generate(JmespathExpression.parse(expr), new GoJmespathExpressionGenerator.Variable( + TEST_MODEL.expectShape(ShapeId.from("smithy.go.test#Struct")), + "input" + )); + assertThat(actual.shape().toShapeId().toString(), Matchers.equalTo("smithy.api#PrimitiveBoolean")); + assertThat(actual.ident(), Matchers.equalTo("v3")); + assertThat(writer.toString(), Matchers.containsString(""" + v1 := input.NullableIntegerA + v2 := 9 + var v3 bool + if (v1 == nil) { + v1 = new(int32) + *v1 = 0 + } + + if v1 != nil { + v3 = int64(*v1) > int64(v2) + } + """)); + } + + @Test + public void testComparatorNumberCoercesBothNullable() { + var expr = "nullableIntegerA > nullableIntegerB"; + + var writer = testWriter(); + var generator = new GoJmespathExpressionGenerator(testContext(), writer); + var actual = generator.generate(JmespathExpression.parse(expr), new GoJmespathExpressionGenerator.Variable( + TEST_MODEL.expectShape(ShapeId.from("smithy.go.test#Struct")), + "input" + )); + assertThat(actual.shape().toShapeId().toString(), Matchers.equalTo("smithy.api#PrimitiveBoolean")); + assertThat(actual.ident(), Matchers.equalTo("v3")); + assertThat(writer.toString(), Matchers.containsString(""" + v1 := input.NullableIntegerA + v2 := input.NullableIntegerB + var v3 bool + if (v1 == nil) { + v1 = new(int32) + *v1 = 0 + } + if (v2 == nil) { + v2 = new(int32) + *v2 = 0 + } + if v1 != nil && v2 != nil { + v3 = int64(*v1) > int64(*v2) + } + """)); + } } From 13573445ad8394812924123a976b61578922b37d Mon Sep 17 00:00:00 2001 From: Luc Talatinian Date: Wed, 22 Jan 2025 19:19:44 -0500 Subject: [PATCH 2/3] only order cmp --- .../GoJmespathExpressionGenerator.java | 45 ++++++++++++------- .../GoJmespathExpressionGeneratorTest.java | 6 +-- 2 files changed, 33 insertions(+), 18 deletions(-) diff --git a/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/GoJmespathExpressionGenerator.java b/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/GoJmespathExpressionGenerator.java index a36d5242..840af016 100644 --- a/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/GoJmespathExpressionGenerator.java +++ b/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/GoJmespathExpressionGenerator.java @@ -408,33 +408,40 @@ private GoWriter.Writable compareVariables(String ident, Variable left, Variable return goTemplate("$1L := $5L($2L) $4L $5L($3L)", ident, left.ident, right.ident, cmp, cast); } - // undocumented jmespath behavior: null in numeric comparisons coerces to 0 + // undocumented jmespath behavior: null in numeric _ordering_ comparisons coerces to 0 // this means the subsequent nil checks for numerics are moot, but it's either this or branch the codegen even // further for questionable benefit var nilCoerceLeft = emptyGoTemplate(); var nilCoerceRight = emptyGoTemplate(); - if (isLPtr && left.shape instanceof NumberShape) { - nilCoerceLeft = goTemplate(""" - if ($1L == nil) { - $1L = new($2T) - *$1L = 0 - }""", left.ident, left.type); - } - if (isRPtr && right.shape instanceof NumberShape) { - nilCoerceRight = goTemplate(""" - if ($1L == nil) { - $1L = new($2T) - *$1L = 0 - }""", right.ident, right.type); + if (isOrderComparator(cmp)) { + if (isLPtr && left.shape instanceof NumberShape) { + nilCoerceLeft = goTemplate(""" + if ($1L == nil) { + $1L = new($2T) + *$1L = 0 + }""", left.ident, left.type); + } + if (isRPtr && right.shape instanceof NumberShape) { + nilCoerceRight = goTemplate(""" + if ($1L == nil) { + $1L = new($2T) + *$1L = 0 + }""", right.ident, right.type); + } } + // also, if they're both pointers, and it's equality, there's an additional true case where both are nil + var elseCmpBothNull = !isOrderComparator(cmp) && isLPtr && isRPtr + ? goTemplate("else { $L = $L == nil && $L == nil }", ident, left.ident, right.ident) + : emptyGoTemplate(); + return goTemplate(""" var $ident:L bool $nilCoerceLeft:W $nilCoerceRight:W if $lif:L $amp:L $rif:L { $ident:L = $cast:L($lhs:L) $cmp:L $cast:L($rhs:L) - }""", + }$elseCmpBothNull:W""", Map.of( "ident", ident, "lif", isLPtr ? left.ident + " != nil" : "", @@ -446,9 +453,17 @@ private GoWriter.Writable compareVariables(String ident, Variable left, Variable "cast", cast, "nilCoerceLeft", nilCoerceLeft, "nilCoerceRight", nilCoerceRight + ), + Map.of( + "elseCmpBothNull", elseCmpBothNull )); } + private static boolean isOrderComparator(ComparatorType cmp) { + return cmp == ComparatorType.GREATER_THAN || cmp == ComparatorType.LESS_THAN + || cmp == ComparatorType.GREATER_THAN_EQUAL || cmp == ComparatorType.LESS_THAN_EQUAL; + } + /** * Represents a variable (input, intermediate, or final output) of a JMESPath traversal. * @param shape The underlying shape referenced by this variable. For certain jmespath expressions (e.g. diff --git a/codegen/smithy-go-codegen/src/test/java/software/amazon/smithy/go/codegen/GoJmespathExpressionGeneratorTest.java b/codegen/smithy-go-codegen/src/test/java/software/amazon/smithy/go/codegen/GoJmespathExpressionGeneratorTest.java index 336ebfce..a4b96e76 100644 --- a/codegen/smithy-go-codegen/src/test/java/software/amazon/smithy/go/codegen/GoJmespathExpressionGeneratorTest.java +++ b/codegen/smithy-go-codegen/src/test/java/software/amazon/smithy/go/codegen/GoJmespathExpressionGeneratorTest.java @@ -379,7 +379,7 @@ public void testComparatorStringBothNil() { if v2 != nil && v4 != nil { v5 = string(*v2) == string(*v4) - } + }else { v5 = v2 == nil && v4 == nil } """)); } @@ -553,7 +553,7 @@ public void testMultiSelectFlatten() { } @Test - public void testComparatorNumberCoercesLeftNullable() { + public void testOrderComparatorNumberCoercesLeftNullable() { var expr = "nullableIntegerA > `9`"; var writer = testWriter(); @@ -580,7 +580,7 @@ public void testComparatorNumberCoercesLeftNullable() { } @Test - public void testComparatorNumberCoercesBothNullable() { + public void testOrderComparatorNumberCoercesBothNullable() { var expr = "nullableIntegerA > nullableIntegerB"; var writer = testWriter(); From 1e4fa56447f3cdae81d891f9e83ab19f7307adfc Mon Sep 17 00:00:00 2001 From: Luc Talatinian Date: Wed, 22 Jan 2025 19:37:23 -0500 Subject: [PATCH 3/3] sigh --- .../GoJmespathExpressionGenerator.java | 20 +++++--- .../GoJmespathExpressionGeneratorTest.java | 46 +++++++++++++++++++ 2 files changed, 60 insertions(+), 6 deletions(-) diff --git a/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/GoJmespathExpressionGenerator.java b/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/GoJmespathExpressionGenerator.java index 840af016..89a87f62 100644 --- a/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/GoJmespathExpressionGenerator.java +++ b/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/GoJmespathExpressionGenerator.java @@ -430,10 +430,18 @@ private GoWriter.Writable compareVariables(String ident, Variable left, Variable } } - // also, if they're both pointers, and it's equality, there's an additional true case where both are nil - var elseCmpBothNull = !isOrderComparator(cmp) && isLPtr && isRPtr - ? goTemplate("else { $L = $L == nil && $L == nil }", ident, left.ident, right.ident) - : emptyGoTemplate(); + // also, if they're both pointers, and it's (in)equality, there's an additional true case where both are nil, + // or both are different + var elseCheckPtrs = emptyGoTemplate(); + if (isLPtr && isRPtr) { + if (cmp == ComparatorType.EQUAL) { + elseCheckPtrs = goTemplate("else { $L = $L == nil && $L == nil }", + ident, left.ident, right.ident); + } else if (cmp == ComparatorType.NOT_EQUAL) { + elseCheckPtrs = goTemplate("else { $1L = ($2L == nil && $3L != nil) || ($2L != nil && $3L == nil) }", + ident, left.ident, right.ident); + } + } return goTemplate(""" var $ident:L bool @@ -441,7 +449,7 @@ private GoWriter.Writable compareVariables(String ident, Variable left, Variable $nilCoerceRight:W if $lif:L $amp:L $rif:L { $ident:L = $cast:L($lhs:L) $cmp:L $cast:L($rhs:L) - }$elseCmpBothNull:W""", + }$elseCheckPtrs:W""", Map.of( "ident", ident, "lif", isLPtr ? left.ident + " != nil" : "", @@ -455,7 +463,7 @@ private GoWriter.Writable compareVariables(String ident, Variable left, Variable "nilCoerceRight", nilCoerceRight ), Map.of( - "elseCmpBothNull", elseCmpBothNull + "elseCheckPtrs", elseCheckPtrs )); } diff --git a/codegen/smithy-go-codegen/src/test/java/software/amazon/smithy/go/codegen/GoJmespathExpressionGeneratorTest.java b/codegen/smithy-go-codegen/src/test/java/software/amazon/smithy/go/codegen/GoJmespathExpressionGeneratorTest.java index a4b96e76..0a6b01a3 100644 --- a/codegen/smithy-go-codegen/src/test/java/software/amazon/smithy/go/codegen/GoJmespathExpressionGeneratorTest.java +++ b/codegen/smithy-go-codegen/src/test/java/software/amazon/smithy/go/codegen/GoJmespathExpressionGeneratorTest.java @@ -608,4 +608,50 @@ public void testOrderComparatorNumberCoercesBothNullable() { } """)); } + + @Test + public void testEqualBothNullable() { + var expr = "nullableIntegerA == nullableIntegerB"; + + var writer = testWriter(); + var generator = new GoJmespathExpressionGenerator(testContext(), writer); + var actual = generator.generate(JmespathExpression.parse(expr), new GoJmespathExpressionGenerator.Variable( + TEST_MODEL.expectShape(ShapeId.from("smithy.go.test#Struct")), + "input" + )); + assertThat(actual.shape().toShapeId().toString(), Matchers.equalTo("smithy.api#PrimitiveBoolean")); + assertThat(actual.ident(), Matchers.equalTo("v3")); + assertThat(writer.toString(), Matchers.containsString(""" + v1 := input.NullableIntegerA + v2 := input.NullableIntegerB + var v3 bool + + if v1 != nil && v2 != nil { + v3 = int64(*v1) == int64(*v2) + }else { v3 = v1 == nil && v2 == nil } + """)); + } + + @Test + public void testNotEqualBothNullable() { + var expr = "nullableIntegerA != nullableIntegerB"; + + var writer = testWriter(); + var generator = new GoJmespathExpressionGenerator(testContext(), writer); + var actual = generator.generate(JmespathExpression.parse(expr), new GoJmespathExpressionGenerator.Variable( + TEST_MODEL.expectShape(ShapeId.from("smithy.go.test#Struct")), + "input" + )); + assertThat(actual.shape().toShapeId().toString(), Matchers.equalTo("smithy.api#PrimitiveBoolean")); + assertThat(actual.ident(), Matchers.equalTo("v3")); + assertThat(writer.toString(), Matchers.containsString(""" + v1 := input.NullableIntegerA + v2 := input.NullableIntegerB + var v3 bool + + if v1 != nil && v2 != nil { + v3 = int64(*v1) != int64(*v2) + }else { v3 = (v1 == nil && v2 != nil) || (v1 != nil && v2 == nil) } + """)); + } }