Refactor rules engine functions

This commit refactors the configuration and inheritance hierarchy
of functions in the rules engine. Two intermediate abstract classes
have been eliminated, SingleArgFunction and Function.

All functions are now supplied through an SPI on FunctionDefinition,
and each implementation now supplies a FunctionDefinition. Special
cases for methods when generating from FunctionNode instances have
also been removed, aligning all implementations to the same
construction. No behavior has changed in the ExpressionVisitor to
reduce the impact of migration.

Unused, or minimally test-case used, convenience functions have been
removed.

Several bugs were fixed, including dropping context information from
certain sets of RulesError generation.

smithy-rules-engine/src/main/java/software/amazon/smithy/rulesengine/analysis/CoverageChecker.java

@@ -23,11 +23,11 @@
 import java.util.stream.Stream;
 import software.amazon.smithy.model.node.Node;
 import software.amazon.smithy.rulesengine.language.EndpointRuleSet;
+import software.amazon.smithy.rulesengine.language.TraversingVisitor;
 import software.amazon.smithy.rulesengine.language.evaluation.RuleEvaluator;
 import software.amazon.smithy.rulesengine.language.evaluation.value.Value;
 import software.amazon.smithy.rulesengine.language.syntax.Identifier;
 import software.amazon.smithy.rulesengine.language.syntax.rule.Condition;
-import software.amazon.smithy.rulesengine.language.visitors.TraversingVisitor;
 import software.amazon.smithy.rulesengine.traits.EndpointTestCase;
 import software.amazon.smithy.utils.SetUtils;
 import software.amazon.smithy.utils.SmithyUnstableApi;

smithy-rules-engine/src/main/java/software/amazon/smithy/rulesengine/language/visitors/TraversingVisitor.java → smithy-rules-engine/src/main/java/software/amazon/smithy/rulesengine/language/TraversingVisitor.java

@@ -13,15 +13,15 @@
  * permissions and limitations under the License.
  */
 
-package software.amazon.smithy.rulesengine.language.visitors;
+package software.amazon.smithy.rulesengine.language;
 
 import java.util.List;
 import java.util.stream.Stream;
-import software.amazon.smithy.rulesengine.language.Endpoint;
-import software.amazon.smithy.rulesengine.language.EndpointRuleSet;
 import software.amazon.smithy.rulesengine.language.syntax.expressions.Expression;
+import software.amazon.smithy.rulesengine.language.syntax.expressions.ExpressionVisitor;
 import software.amazon.smithy.rulesengine.language.syntax.rule.Condition;
 import software.amazon.smithy.rulesengine.language.syntax.rule.Rule;
+import software.amazon.smithy.rulesengine.language.syntax.rule.RuleValueVisitor;
 import software.amazon.smithy.utils.SmithyUnstableApi;
 
 /**
@@ -30,7 +30,9 @@
  * @param <R> the return type.
  */
 @SmithyUnstableApi
-public class TraversingVisitor<R> extends DefaultVisitor<Stream<R>> {
+public class TraversingVisitor<R> extends ExpressionVisitor.Default<Stream<R>>
+        implements RuleValueVisitor<Stream<R>> {
+
     /**
      * Given an {@link EndpointRuleSet} will invoke the visitor methods for each rule.
      *

smithy-rules-engine/src/main/java/software/amazon/smithy/rulesengine/language/error/RuleError.java

@@ -80,7 +80,7 @@ public RuleError withContext(String context, SourceLocation loc) {
     public String toString() {
         StringBuilder message = new StringBuilder();
         SourceLocation lastLoc = SourceLocation.none();
-        for (int i = contexts.size() - 1; i > 0; i--) {
+        for (int i = contexts.size() - 1; i >= 0; i--) {
             Pair<String, SourceLocation> context = contexts.get(i);
             message.append(context.left);
             message.append("\n");
@@ -94,14 +94,12 @@ public String toString() {
             }
         }
 
+        message.append(root.getMessageWithoutLocation());
         if (root.getSourceLocation() != SourceLocation.none() && root.getSourceLocation() != lastLoc) {
-            message.append("  at ")
+            message.append("\n").append("  at ")
                     .append(root.getSourceLocation().getFilename())
-                    .append(":").append(root.getSourceLocation().getLine())
-                    .append("\n");
+                    .append(":").append(root.getSourceLocation().getLine());
         }
-
-        message.append(root.getMessageWithoutLocation());
         return message.toString();
     }
 

smithy-rules-engine/src/main/java/software/amazon/smithy/rulesengine/language/evaluation/RuleEvaluator.java

@@ -24,15 +24,15 @@
 import software.amazon.smithy.rulesengine.language.evaluation.value.Value;
 import software.amazon.smithy.rulesengine.language.syntax.Identifier;
 import software.amazon.smithy.rulesengine.language.syntax.expressions.Expression;
+import software.amazon.smithy.rulesengine.language.syntax.expressions.ExpressionVisitor;
 import software.amazon.smithy.rulesengine.language.syntax.expressions.Reference;
+import software.amazon.smithy.rulesengine.language.syntax.expressions.functions.FunctionDefinition;
+import software.amazon.smithy.rulesengine.language.syntax.expressions.functions.GetAttr;
 import software.amazon.smithy.rulesengine.language.syntax.expressions.literal.Literal;
-import software.amazon.smithy.rulesengine.language.syntax.functions.FunctionDefinition;
-import software.amazon.smithy.rulesengine.language.syntax.functions.GetAttr;
 import software.amazon.smithy.rulesengine.language.syntax.parameters.Parameter;
 import software.amazon.smithy.rulesengine.language.syntax.rule.Condition;
 import software.amazon.smithy.rulesengine.language.syntax.rule.Rule;
 import software.amazon.smithy.rulesengine.language.syntax.rule.RuleValueVisitor;
-import software.amazon.smithy.rulesengine.language.visitors.ExpressionVisitor;
 import software.amazon.smithy.utils.SmithyUnstableApi;
 
 /**

smithy-rules-engine/src/main/java/software/amazon/smithy/rulesengine/language/evaluation/type/OptionalType.java

@@ -20,7 +20,8 @@ public boolean isA(Type type) {
         if (!(type instanceof OptionalType)) {
             return false;
         }
-        return ((OptionalType) type).inner.isA(inner);
+        OptionalType other = ((OptionalType) type);
+        return other.inner.isA(this.inner);
     }
 
     @Override

smithy-rules-engine/src/main/java/software/amazon/smithy/rulesengine/language/stdlib/AwsIsVirtualHostableS3Bucket.java

@@ -19,50 +19,63 @@
 import java.util.List;
 import software.amazon.smithy.rulesengine.language.evaluation.type.Type;
 import software.amazon.smithy.rulesengine.language.evaluation.value.Value;
-import software.amazon.smithy.rulesengine.language.syntax.expressions.Expression;
-import software.amazon.smithy.rulesengine.language.syntax.functions.Function;
-import software.amazon.smithy.rulesengine.language.syntax.functions.FunctionDefinition;
-import software.amazon.smithy.rulesengine.language.syntax.functions.LibraryFunction;
+import software.amazon.smithy.rulesengine.language.syntax.expressions.ExpressionVisitor;
+import software.amazon.smithy.rulesengine.language.syntax.expressions.functions.FunctionDefinition;
+import software.amazon.smithy.rulesengine.language.syntax.expressions.functions.FunctionNode;
+import software.amazon.smithy.rulesengine.language.syntax.expressions.functions.LibraryFunction;
 import software.amazon.smithy.utils.SmithyUnstableApi;
 
 /**
  * An AWS rule-set function for determining whether a given string can be promoted to an S3 virtual bucket host label.
  */
 @SmithyUnstableApi
-public class AwsIsVirtualHostableS3Bucket implements FunctionDefinition {
+public class AwsIsVirtualHostableS3Bucket extends LibraryFunction {
     public static final String ID = "aws.isVirtualHostableS3Bucket";
+    private static final Definition DEFINITION = new Definition();
 
-    @Override
-    public String getId() {
-        return ID;
+    public AwsIsVirtualHostableS3Bucket(FunctionNode functionNode) {
+        super(DEFINITION, functionNode);
     }
 
     @Override
-    public List<Type> getArguments() {
-        return Arrays.asList(Type.stringType(), Type.booleanType());
+    public <T> T accept(ExpressionVisitor<T> visitor) {
+        return visitor.visitLibraryFunction(DEFINITION, getArguments());
     }
 
-    @Override
-    public Type getReturnType() {
-        return Type.booleanType();
-    }
+    public static final class Definition implements FunctionDefinition {
+        @Override
+        public String getId() {
+            return ID;
+        }
 
-    @Override
-    public Value evaluate(List<Value> arguments) {
-        String hostLabel = arguments.get(0).expectStringValue().getValue();
-        boolean allowDots = arguments.get(1).expectBooleanValue().getValue();
-        if (allowDots) {
-            return Value.booleanValue(
-                    hostLabel.matches("[a-z\\d][a-z\\d\\-.]{1,61}[a-z\\d]")
-                    && !hostLabel.matches("(\\d+\\.){3}\\d+") // don't allow ip address
-                    && !hostLabel.matches(".*[.-]{2}.*") // don't allow names like bucket-.name or bucket.-name
-            );
-        } else {
-            return Value.booleanValue(hostLabel.matches("[a-z\\d][a-z\\d\\-]{1,61}[a-z\\d]"));
+        @Override
+        public List<Type> getArguments() {
+            return Arrays.asList(Type.stringType(), Type.booleanType());
         }
-    }
 
-    public static Function ofExpression(Expression input, boolean allowDots) {
-        return LibraryFunction.ofExpressions(new AwsIsVirtualHostableS3Bucket(), input, Expression.of(allowDots));
+        @Override
+        public Type getReturnType() {
+            return Type.booleanType();
+        }
+
+        @Override
+        public Value evaluate(List<Value> arguments) {
+            String hostLabel = arguments.get(0).expectStringValue().getValue();
+            boolean allowDots = arguments.get(1).expectBooleanValue().getValue();
+            if (allowDots) {
+                return Value.booleanValue(
+                        hostLabel.matches("[a-z\\d][a-z\\d\\-.]{1,61}[a-z\\d]")
+                        && !hostLabel.matches("(\\d+\\.){3}\\d+") // don't allow ip address
+                        && !hostLabel.matches(".*[.-]{2}.*") // don't allow names like bucket-.name or bucket.-name
+                );
+            } else {
+                return Value.booleanValue(hostLabel.matches("[a-z\\d][a-z\\d\\-]{1,61}[a-z\\d]"));
+            }
+        }
+
+        @Override
+        public AwsIsVirtualHostableS3Bucket createFunction(FunctionNode functionNode) {
+            return new AwsIsVirtualHostableS3Bucket(functionNode);
+        }
     }
 }

smithy-rules-engine/src/main/java/software/amazon/smithy/rulesengine/language/stdlib/AwsPartition.java

@@ -32,103 +32,36 @@
 import software.amazon.smithy.rulesengine.language.model.Partitions;
 import software.amazon.smithy.rulesengine.language.stdlib.partition.PartitionDataProvider;
 import software.amazon.smithy.rulesengine.language.syntax.Identifier;
-import software.amazon.smithy.rulesengine.language.syntax.expressions.Expression;
-import software.amazon.smithy.rulesengine.language.syntax.functions.Function;
-import software.amazon.smithy.rulesengine.language.syntax.functions.FunctionDefinition;
-import software.amazon.smithy.rulesengine.language.syntax.functions.FunctionNode;
-import software.amazon.smithy.rulesengine.language.syntax.functions.LibraryFunction;
+import software.amazon.smithy.rulesengine.language.syntax.expressions.ExpressionVisitor;
+import software.amazon.smithy.rulesengine.language.syntax.expressions.functions.FunctionDefinition;
+import software.amazon.smithy.rulesengine.language.syntax.expressions.functions.FunctionNode;
+import software.amazon.smithy.rulesengine.language.syntax.expressions.functions.LibraryFunction;
 import software.amazon.smithy.utils.MapUtils;
 import software.amazon.smithy.utils.SmithyUnstableApi;
 
 /**
  * An AWS rule-set function for mapping a region string to a partition.
  */
 @SmithyUnstableApi
-public final class AwsPartition implements FunctionDefinition {
+public final class AwsPartition extends LibraryFunction {
     public static final String ID = "aws.partition";
-
     public static final Identifier NAME = Identifier.of("name");
     public static final Identifier DNS_SUFFIX = Identifier.of("dnsSuffix");
     public static final Identifier DUAL_STACK_DNS_SUFFIX = Identifier.of("dualStackDnsSuffix");
     public static final Identifier SUPPORTS_FIPS = Identifier.of("supportsFIPS");
     public static final Identifier SUPPORTS_DUAL_STACK = Identifier.of("supportsDualStack");
     public static final Identifier INFERRED = Identifier.of("inferred");
 
+    private static final Definition DEFINITION = new Definition();
     private static final PartitionData PARTITION_DATA = loadPartitionData();
 
-    @Override
-    public String getId() {
-        return ID;
-    }
-
-    @Override
-    public List<Type> getArguments() {
-        return Collections.singletonList(Type.stringType());
-    }
-
-    @Override
-    public Type getReturnType() {
-        Map<Identifier, Type> type = new LinkedHashMap<>();
-        type.put(NAME, Type.stringType());
-        type.put(DNS_SUFFIX, Type.stringType());
-        type.put(DUAL_STACK_DNS_SUFFIX, Type.stringType());
-        type.put(SUPPORTS_DUAL_STACK, Type.booleanType());
-        type.put(SUPPORTS_FIPS, Type.booleanType());
-        return Type.optionalType(new RecordType(type));
+    public AwsPartition(FunctionNode functionNode) {
+        super(DEFINITION, functionNode);
     }
 
     @Override
-    public Value evaluate(List<Value> arguments) {
-        String regionName = arguments.get(0).expectStringValue().getValue();
-        Partition matchedPartition;
-        boolean inferred = false;
-
-        // Known region
-        matchedPartition = PARTITION_DATA.regionMap.get(regionName);
-        if (matchedPartition == null) {
-            // Try matching on region name pattern
-            for (Partition p : PARTITION_DATA.partitions) {
-                Pattern regex = Pattern.compile(p.getRegionRegex());
-                if (regex.matcher(regionName).matches()) {
-                    matchedPartition = p;
-                    inferred = true;
-                    break;
-                }
-            }
-        }
-
-        if (matchedPartition == null) {
-            for (Partition partition : PARTITION_DATA.partitions) {
-                if (partition.getId().equals("aws")) {
-                    matchedPartition = partition;
-                    break;
-                }
-            }
-        }
-
-        if (matchedPartition == null) {
-            // TODO
-            throw new RuntimeException("Unable to match a partition for region " + regionName);
-        }
-
-        PartitionOutputs matchedPartitionOutputs = matchedPartition.getOutputs();
-        return Value.recordValue(MapUtils.of(
-                NAME, Value.stringValue(matchedPartition.getId()),
-                DNS_SUFFIX, Value.stringValue(matchedPartitionOutputs.getDnsSuffix()),
-                DUAL_STACK_DNS_SUFFIX, Value.stringValue(matchedPartitionOutputs.getDualStackDnsSuffix()),
-                SUPPORTS_FIPS, Value.booleanValue(matchedPartitionOutputs.supportsFips()),
-                SUPPORTS_DUAL_STACK, Value.booleanValue(matchedPartitionOutputs.supportsDualStack()),
-                INFERRED, Value.booleanValue(inferred)));
-    }
-
-    /**
-     * Constructs a function definition for resolving a string expression to a partition.
-     *
-     * @param expression expression to evaluate to a partition.
-     * @return the function representing the partition lookup.
-     */
-    public static Function ofExpression(Expression expression) {
-        return new LibraryFunction(new AwsPartition(), FunctionNode.ofExpressions(ID, expression));
+    public <T> T accept(ExpressionVisitor<T> visitor) {
+        return visitor.visitLibraryFunction(DEFINITION, getArguments());
     }
 
     private static PartitionData loadPartitionData() {
@@ -157,4 +90,75 @@ private static class PartitionData {
         private final List<Partition> partitions = new ArrayList<>();
         private final Map<String, Partition> regionMap = new HashMap<>();
     }
+
+    public static final class Definition implements FunctionDefinition {
+        @Override
+        public String getId() {
+            return ID;
+        }
+
+        @Override
+        public List<Type> getArguments() {
+            return Collections.singletonList(Type.stringType());
+        }
+
+        @Override
+        public Type getReturnType() {
+            Map<Identifier, Type> type = new LinkedHashMap<>();
+            type.put(NAME, Type.stringType());
+            type.put(DNS_SUFFIX, Type.stringType());
+            type.put(DUAL_STACK_DNS_SUFFIX, Type.stringType());
+            type.put(SUPPORTS_DUAL_STACK, Type.booleanType());
+            type.put(SUPPORTS_FIPS, Type.booleanType());
+            return Type.optionalType(new RecordType(type));
+        }
+
+        @Override
+        public Value evaluate(List<Value> arguments) {
+            String regionName = arguments.get(0).expectStringValue().getValue();
+            Partition matchedPartition;
+            boolean inferred = false;
+
+            // Known region
+            matchedPartition = PARTITION_DATA.regionMap.get(regionName);
+            if (matchedPartition == null) {
+                // Try matching on region name pattern
+                for (Partition p : PARTITION_DATA.partitions) {
+                    Pattern regex = Pattern.compile(p.getRegionRegex());
+                    if (regex.matcher(regionName).matches()) {
+                        matchedPartition = p;
+                        inferred = true;
+                        break;
+                    }
+                }
+            }
+
+            if (matchedPartition == null) {
+                for (Partition partition : PARTITION_DATA.partitions) {
+                    if (partition.getId().equals("aws")) {
+                        matchedPartition = partition;
+                        break;
+                    }
+                }
+            }
+
+            if (matchedPartition == null) {
+                throw new RuntimeException("Unable to match a partition for region " + regionName);
+            }
+
+            PartitionOutputs matchedPartitionOutputs = matchedPartition.getOutputs();
+            return Value.recordValue(MapUtils.of(
+                    NAME, Value.stringValue(matchedPartition.getId()),
+                    DNS_SUFFIX, Value.stringValue(matchedPartitionOutputs.getDnsSuffix()),
+                    DUAL_STACK_DNS_SUFFIX, Value.stringValue(matchedPartitionOutputs.getDualStackDnsSuffix()),
+                    SUPPORTS_FIPS, Value.booleanValue(matchedPartitionOutputs.supportsFips()),
+                    SUPPORTS_DUAL_STACK, Value.booleanValue(matchedPartitionOutputs.supportsDualStack()),
+                    INFERRED, Value.booleanValue(inferred)));
+        }
+
+        @Override
+        public AwsPartition createFunction(FunctionNode functionNode) {
+            return new AwsPartition(functionNode);
+        }
+    }
 }