only order cmp

aws · Jan 23, 2025 · 1357344 · 1357344
1 parent e4344f6
commit 1357344
Show file tree

Hide file tree

Showing 2 changed files with 33 additions and 18 deletions.
diff --git a/...odegen/src/main/java/software/amazon/smithy/go/codegen/GoJmespathExpressionGenerator.java b/...odegen/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/...en/src/test/java/software/amazon/smithy/go/codegen/GoJmespathExpressionGeneratorTest.java b/...en/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();