Repository: smithy-lang/smithy (java)

Issue:
NumberNode's equal definition is impacted by the underlying representation 
I'm not sure whether this qualifies as a bug, as it's directly coming from what I'd qualify as a java gotcha, but the two nodes values defined below are not equal with one another : 

* `Node.from(new java.math.BigDecimal("1.0e+10"))`
* `Node.from(1.0e+10)`

This makes comparing some models hard. It'd be nice to have the `equals` method of number nodes protect against this gotcha. 

Pull request:
Improve node number equality
Attempts to use the most optimal comparisons of number nodes, using .equals if both number values are of the same type, then the string representation of each value, finally followed by comparing the BigDecimal representation of each value using the string cache. If a BigDecimal is needed, it is cached on the number node for future checks.

Closes #1378

*Issue #, if available:*

*Description of changes:*


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.


Patch:
diff --git a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/SelectCommand.java b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/SelectCommand.java
index ae44f7729..3f2548cb9 100644
--- a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/SelectCommand.java
+++ b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/SelectCommand.java
@@ -21,6 +21,7 @@ import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.TreeMap;
+import java.util.TreeSet;
 import java.util.function.Consumer;
 import java.util.logging.Level;
 import java.util.logging.Logger;
@@ -86,30 +87,88 @@ final class SelectCommand implements Command {
     }
 
     private String getDocumentation(ColorFormatter colors) {
-        return "By default, each matching shape ID is printed to stdout on a new line. Pass --show-vars to print out "
-               + "a JSON array that contains a 'shape' and 'vars' property, where the 'vars' property is a map of "
-               + "each variable that was captured when the shape was matched.";
+        return "By default, each matching shape ID is printed to stdout on a new line. Pass --show or --show-traits "
+               + "to get JSON array output.";
     }
 
     private static final class Options implements ArgumentReceiver {
-        private boolean showVars;
         private Selector selector;
         private final List<ShapeId> showTraits = new ArrayList<>();
+        private final Set<Show> show = new TreeSet<>();
+
+        private enum Show {
+            TYPE("type") {
+                @Override
+                protected void inject(Selector.ShapeMatch match, ObjectNode.Builder builder) {
+                    builder.withMember("type", match.getShape().getType().toString());
+                }
+            },
+
+            FILE("file") {
+                @Override
+                protected void inject(Selector.ShapeMatch match, ObjectNode.Builder builder) {
+                    SourceLocation source = match.getShape().getSourceLocation();
+                    // Only shapes with a real source location add a file.
+                    if (!source.getFilename().equals(SourceLocation.NONE.getFilename())) {
+                        builder.withMember("file", source.getFilename()
+                                                   + ':' + source.getLine()
+                                                   + ':' + source.getColumn());
+                    }
+                }
+            },
+
+            VARS("vars") {
+                @Override
+                protected void inject(Selector.ShapeMatch match, ObjectNode.Builder builder) {
+                    if (!match.isEmpty()) {
+                        ObjectNode.Builder varBuilder = Node.objectNodeBuilder();
+                        for (Map.Entry<String, Set<Shape>> varEntry : match.entrySet()) {
+                            varBuilder.withMember(
+                                varEntry.getKey(),
+                                sortShapeIds(varEntry.getValue()).map(Node::from).collect(ArrayNode.collect())
+                            );
+                        }
+                        ObjectNode collectedVars = varBuilder.build();
+                        builder.withMember("vars", collectedVars);
+                    }
+                }
+            };
+
+            private final String value;
+
+            Show(String value) {
+                this.value = value;
+            }
+
+            protected abstract void inject(Selector.ShapeMatch match, ObjectNode.Builder builder);
+
+            private static Show from(String value) {
+                for (Show variant : values()) {
+                    if (variant.value.equals(value)) {
+                        return variant;
+                    }
+                }
+                throw new CliError("Invalid value given to --show: `" + value + "`");
+            }
+        }
 
         @Override
         public boolean testOption(String name) {
             switch (name) {
                 case "--vars":
-                    LOGGER.warning("--vars is deprecated. Use --show-vars instead.");
-                    // fall-through
                 case "--show-vars":
-                    showVars = true;
-                    return true;
+                    return deprecatedVars(name);
                 default:
                     return false;
             }
         }
 
+        private boolean deprecatedVars(String name) {
+            LOGGER.warning(name + " is deprecated. Use `--show vars` instead.");
+            show.add(Show.VARS);
+            return true;
+        }
+
         @Override
         public Consumer<String> testParameter(String name) {
             switch (name) {
@@ -128,6 +187,13 @@ final class SelectCommand implements Command {
                             throw new CliError("--show-traits must contain traits");
                         }
                     };
+                case "--show":
+                    return value -> {
+                        String[] parts = value.split("\\s*,\\s*");
+                        for (String part : parts) {
+                            show.add(Show.from(part));
+                        }
+                    };
                 default:
                     return null;
             }
@@ -137,16 +203,17 @@ final class SelectCommand implements Command {
         public void registerHelp(HelpPrinter printer) {
             printer.param("--selector", null, "SELECTOR",
                           "The Smithy selector to execute. Reads from STDIN when not provided.");
+            printer.param("--show", null, "DATA",
+                          "Displays additional top-level members in each match and forces JSON output. This parameter "
+                          + "accepts a comma-separated list of values, including 'type', 'file', and 'vars'. 'type' "
+                          + "adds a string member containing the shape type of each match. 'file' adds a string "
+                          + "member containing the absolute path to where the shape is defined followed by the line "
+                          + "number then column (e.g., '/path/example.smithy:10:1'). 'vars' adds an object containing "
+                          + "the variables that were captured when a shape was matched.");
             printer.param("--show-traits", null, "TRAITS",
                           "Returns JSON output that includes the values of specific traits applied to matched shapes, "
                           + "stored in a 'traits' property. Provide a comma-separated list of trait shape IDs. "
                           + "Prelude traits may omit a namespace (e.g., 'required' or 'smithy.api#required').");
-            printer.option("--show-vars", null, "Returns JSON output that includes the variables that were captured "
-                                                + "when a shape was matched, stored in a 'vars' property.");
-        }
-
-        public boolean showVars() {
-            return showVars;
         }
 
         public Selector selector() {
@@ -199,8 +266,8 @@ final class SelectCommand implements Command {
                     ObjectNode.Builder builder = Node.objectNodeBuilder()
                             .withMember("shape", Node.from(match.getShape().getId().toString()));
 
-                    if (!match.isEmpty()) {
-                        builder.withMember("vars", collectVars(match));
+                    for (Options.Show showData : options.show) {
+                        showData.inject(match, builder);
                     }
 
                     if (!options.showTraits.isEmpty()) {
@@ -223,21 +290,11 @@ final class SelectCommand implements Command {
         abstract void dumpResults(Selector selector, Model model, Options options, CliPrinter stdout);
 
         static OutputFormat determineFormat(Options options) {
-            // If --show-vars isn't provided and --show-traits is empty, then use the SHAPE_ID_LINES output.
-            return !options.showVars() && options.showTraits.isEmpty() ? SHAPE_ID_LINES : JSON;
-        }
-
-        private static Stream<String> sortShapeIds(Collection<Shape> shapes) {
-            return shapes.stream().map(Shape::getId).map(ShapeId::toString).sorted();
+            return options.showTraits.isEmpty() && options.show.isEmpty() ? SHAPE_ID_LINES : JSON;
         }
+    }
 
-        private static ObjectNode collectVars(Map<String, Set<Shape>> vars) {
-            ObjectNode.Builder varBuilder = Node.objectNodeBuilder();
-            for (Map.Entry<String, Set<Shape>> varEntry : vars.entrySet()) {
-                ArrayNode value = sortShapeIds(varEntry.getValue()).map(Node::from).collect(ArrayNode.collect());
-                varBuilder.withMember(varEntry.getKey(), value);
-            }
-            return varBuilder.build();
-        }
+    private static Stream<String> sortShapeIds(Collection<Shape> shapes) {
+        return shapes.stream().map(Shape::getId).map(ShapeId::toString).sorted();
     }
 }
diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/node/NumberNode.java b/smithy-model/src/main/java/software/amazon/smithy/model/node/NumberNode.java
index 10346dbd6..a52349cb5 100644
--- a/smithy-model/src/main/java/software/amazon/smithy/model/node/NumberNode.java
+++ b/smithy-model/src/main/java/software/amazon/smithy/model/node/NumberNode.java
@@ -33,11 +33,16 @@ public final class NumberNode extends Node {
 
     private final Number value;
     private final String stringCache;
+    private volatile BigDecimal equality;
 
     public NumberNode(Number value, SourceLocation sourceLocation) {
         super(sourceLocation);
         this.value = Objects.requireNonNull(value);
         stringCache = value.toString();
+
+        if (value instanceof BigDecimal) {
+            equality = (BigDecimal) value;
+        }
     }
 
     /**
@@ -129,7 +134,47 @@ public final class NumberNode extends Node {
 
     @Override
     public boolean equals(Object other) {
-        return other instanceof NumberNode && stringCache.equals(((NumberNode) other).stringCache);
+        if (!(other instanceof NumberNode)) {
+            return false;
+        } else if (other == this) {
+            return true;
+        } else {
+            NumberNode otherNode = (NumberNode) other;
+
+            // This only works if both values are the same type.
+            if (value.equals(otherNode.value)) {
+                return true;
+            }
+
+            // Attempt a cheap check based on the string cache.
+            if (stringCache.equals(otherNode.stringCache)) {
+                return true;
+            }
+
+            // Convert both to BigDecimal and compare equality.
+            return getEquality().equals(otherNode.getEquality());
+        }
+    }
+
+    private BigDecimal getEquality() {
+        BigDecimal e = equality;
+
+        if (e == null) {
+            if (value instanceof Integer) {
+                e = BigDecimal.valueOf(value.intValue());
+            } else if (value instanceof Short) {
+                e = BigDecimal.valueOf(value.shortValue());
+            } else if (value instanceof Byte) {
+                e = BigDecimal.valueOf(value.byteValue());
+            } else if (value instanceof Long) {
+                e = BigDecimal.valueOf(value.longValue());
+            } else {
+                e = new BigDecimal(stringCache);
+            }
+            equality = e;
+        }
+
+        return e;
     }
 
     @Override


Test patch:
diff --git a/smithy-cli/src/it/java/software/amazon/smithy/cli/SelectTest.java b/smithy-cli/src/it/java/software/amazon/smithy/cli/SelectTest.java
index 14dc8562a..8b6673fe6 100644
--- a/smithy-cli/src/it/java/software/amazon/smithy/cli/SelectTest.java
+++ b/smithy-cli/src/it/java/software/amazon/smithy/cli/SelectTest.java
@@ -31,7 +31,7 @@ public class SelectTest {
 
     @Test
     public void selectsVariables() {
-        List<String> args = Arrays.asList("select", "--show-vars", "--selector", "list $list(*) > member > string");
+        List<String> args = Arrays.asList("select", "--show", "vars", "--selector", "list $list(*) > member > string");
         IntegUtils.run("simple-config-sources", args, result -> {
             assertThat(result.getExitCode(), equalTo(0));
             String content = result.getOutput().trim();
@@ -79,4 +79,80 @@ public class SelectTest {
             assertThat(result.getExitCode(), not(equalTo(0)));
         });
     }
+
+    @Test
+    public void showCannotBeEmpty() {
+        List<String> args = Arrays.asList("select", "--show", "", "--selector", "string");
+        IntegUtils.run("simple-config-sources", args, result -> {
+            assertThat(result.getExitCode(), equalTo(1));
+        });
+    }
+
+    @Test
+    public void showCannotContainInvalidValues() {
+        List<String> args = Arrays.asList("select", "--show", "foo", "--selector", "string");
+        IntegUtils.run("simple-config-sources", args, result -> {
+            assertThat(result.getExitCode(), equalTo(1));
+        });
+    }
+
+    @Test
+    public void showCannotContainInvalidValuesInCsv() {
+        List<String> args = Arrays.asList("select", "--show", "vars,foo", "--selector", "string");
+        IntegUtils.run("simple-config-sources", args, result -> {
+            assertThat(result.getExitCode(), equalTo(1));
+        });
+    }
+
+    @Test
+    public void includesType() {
+        List<String> args = Arrays.asList("select", "--show", "type", "--selector", "string");
+        IntegUtils.run("simple-config-sources", args, result -> {
+            assertThat(result.getExitCode(), equalTo(0));
+            String content = result.getOutput().trim();
+            // Ensure it's valid JSON
+            Node.parse(content);
+            assertThat(content, containsString("\"type\": \"string\""));
+        });
+    }
+
+    @Test
+    public void includesFile() {
+        List<String> args = Arrays.asList("select", "--show", "file", "--selector", "string");
+        IntegUtils.run("simple-config-sources", args, result -> {
+            assertThat(result.getExitCode(), equalTo(0));
+            String content = result.getOutput().trim();
+            // Ensure it's valid JSON
+            Node.parse(content);
+            assertThat(content, containsString("\"file\": "));
+        });
+    }
+
+    @Test
+    public void includesFileAndType() {
+        List<String> args = Arrays.asList("select", "--show", "file, type", "--selector", "string");
+        IntegUtils.run("simple-config-sources", args, result -> {
+            assertThat(result.getExitCode(), equalTo(0));
+            String content = result.getOutput().trim();
+            // Ensure it's valid JSON
+            Node.parse(content);
+            assertThat(content, containsString("\"type\": \"string\""));
+            assertThat(content, containsString("\"file\": "));
+        });
+    }
+
+    @Test
+    public void includesFileAndTypeAndVars() {
+        List<String> args = Arrays.asList("select", "--show", "file, type,vars", "--selector", "string $hi(*)");
+        IntegUtils.run("simple-config-sources", args, result -> {
+            assertThat(result.getExitCode(), equalTo(0));
+            String content = result.getOutput().trim();
+            // Ensure it's valid JSON
+            Node.parse(content);
+            assertThat(content, containsString("\"type\": \"string\""));
+            assertThat(content, containsString("\"file\": "));
+            assertThat(content, containsString("\"vars\": "));
+            assertThat(content, containsString("\"hi\": "));
+        });
+    }
 }
diff --git a/smithy-cli/src/test/java/software/amazon/smithy/cli/commands/SelectCommandTest.java b/smithy-cli/src/test/java/software/amazon/smithy/cli/commands/SelectCommandTest.java
index 11fb44566..69bbf930d 100644
--- a/smithy-cli/src/test/java/software/amazon/smithy/cli/commands/SelectCommandTest.java
+++ b/smithy-cli/src/test/java/software/amazon/smithy/cli/commands/SelectCommandTest.java
@@ -57,7 +57,7 @@ public class SelectCommandTest {
     public void printsJsonVarsToStdout() throws Exception {
         String model = Paths.get(getClass().getResource("valid-model.smithy").toURI()).toString();
         CliUtils.Result result = CliUtils.runSmithy("select", "--selector", "string $referenceMe(<)",
-                                                    "--show-vars", model);
+                                                    "--show", "vars", model);
 
         assertThat(result.code(), equalTo(0));
         validateSelectorOutput(result.stdout());
@@ -81,7 +81,7 @@ public class SelectCommandTest {
         try {
             // Send the selector through input stream.
             System.setIn(new ByteArrayInputStream("string $referenceMe(<)".getBytes()));
-            CliUtils.Result result = CliUtils.runSmithy("select", "--show-vars", model);
+            CliUtils.Result result = CliUtils.runSmithy("select", "--show", "vars", model);
 
             assertThat(result.code(), equalTo(0));
             validateSelectorOutput(result.stdout());
diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/node/NumberNodeTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/node/NumberNodeTest.java
index 25d9e2ea1..910860574 100644
--- a/smithy-model/src/test/java/software/amazon/smithy/model/node/NumberNodeTest.java
+++ b/smithy-model/src/test/java/software/amazon/smithy/model/node/NumberNodeTest.java
@@ -15,7 +15,9 @@
 
 package software.amazon.smithy.model.node;
 
+import static org.hamcrest.CoreMatchers.equalTo;
 import static org.hamcrest.CoreMatchers.instanceOf;
+import static org.hamcrest.CoreMatchers.not;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
@@ -241,4 +243,44 @@ public class NumberNodeTest {
             assertEquals(v, result, "Expected " + k + " to be " + v);
         });
     }
+
+    @Test
+    public void compareIntAndShort() {
+        Node left = Node.from(1);
+        Node right = Node.from((short) 1);
+
+        assertThat(left, equalTo(right));
+    }
+
+    @Test
+    public void compareShortAndFloat() {
+        Node left = Node.from((float) 1.0);
+        Node right = Node.from((short) 1);
+
+        assertThat(left, not(equalTo(right)));
+    }
+
+    @Test
+    public void compareByteAndFloat() {
+        Node left = Node.from((float) 1.0);
+        Node right = Node.from((byte) 1);
+
+        assertThat(left, not(equalTo(right)));
+    }
+
+    @Test
+    public void compareDoubleAndBigDecimal() {
+        Node left = Node.from(new BigDecimal("1.0e+10"));
+        Node right = Node.from(1.0e+10);
+
+        assertThat(left, equalTo(right));
+    }
+
+    @Test
+    public void compareDoubleAndFloat() {
+        Node left = Node.from((float) 1.0e+10);
+        Node right = Node.from((double) 1.0e+10);
+
+        assertThat(left, equalTo(right));
+    }
 }