From 2b4f78ee0fec03b8cd1ede6bb5c9bd23f6cf5958 Mon Sep 17 00:00:00 2001 From: Michael Dowling Date: Sat, 18 Mar 2023 23:07:49 -0700 Subject: [PATCH] Add :root and :in selectors, fix variable bug This commit adds support for the `:in` and `:root` functions. The `:in` function is a simpler way to check if a shape is in an expression, typically used to test if a variable contains a shape or if a root expression contains a shape. `:root` is used to create rooted common subexpressions that are evaluated once against every shape in the model. `:root` expressions are evaluate in an isolated context, so any variables used or stored by them are not accessible outside the root selector. `:root` selectors allows selection to be broken into multiple steps and evaluate globally. Let's say you want all number shapes that are used in operation inputs, but not used in operation outputs. This can be done today using the following expression: ``` service $outputs(~> operation -[output]-> ~> number) ~> operation -[input]-> ~> number :not([@: @{id} = @{var|outputs|id}]) ``` With the addition of the ``:in` selector, this gets easier because we can avoid using a scoped attribute selector: ``` service $outputs(~> operation -[output]-> ~> number) ~> operation -[input]-> ~> number :not(:in(${outputs})) ``` (Note: to make this work, I had to uncover and fix a bug in the implementation of how we store variables. We previously used `Collection#add` as a `Receiver`, but that method will return false if it's already seen a shape, which is wrong.) With the addition of `:root`, you can use a much simpler expression: ``` number :in(:root(service ~> operation -[input]-> ~> number)) :not(:in(:root(service ~> operation -[output]-> ~> number))) ``` (Note: the result of root expressions are run once and cached. No need to store them in a variable) These expressions _seem_ to be exactly the same, however, the `:root` expression gives a different result when working with models that contain multiple services. In the first two expressions, if any service uses shape X in input and not output, then X is a result. However, in the `:root` expression, X is only part of the result if no service uses it in their output shape closures. --- docs/source-2.0/spec/selectors.rst | 82 ++++++++++++ .../smithy/cli/commands/SelectCommand.java | 6 + .../software/amazon/smithy/model/Model.java | 10 ++ .../smithy/model/selector/AndSelector.java | 120 +++--------------- .../model/selector/AttributeSelector.java | 21 +-- .../amazon/smithy/model/selector/Context.java | 42 +++--- .../smithy/model/selector/InSelector.java | 71 +++++++++++ .../model/selector/InternalSelector.java | 61 +++++++-- .../smithy/model/selector/RootSelector.java | 53 ++++++++ .../smithy/model/selector/SelectorParser.java | 21 ++- .../selector/ShapeTypeCategorySelector.java | 12 ++ .../model/selector/ShapeTypeSelector.java | 12 +- .../model/selector/VariableGetSelector.java | 12 +- .../model/selector/VariableStoreSelector.java | 5 +- .../model/selector/WrappedSelector.java | 78 +++++++----- .../smithy/model/selector/SelectorTest.java | 49 ++++++- .../ShapeTypeCategorySelectorTest.java | 46 +++++++ .../model/selector/ShapeTypeSelectorTest.java | 7 + .../model/selector/cases/in-function.smithy | 73 +++++++++++ .../model/selector/cases/root-function.smithy | 61 +++++++++ 20 files changed, 660 insertions(+), 182 deletions(-) create mode 100644 smithy-model/src/main/java/software/amazon/smithy/model/selector/InSelector.java create mode 100644 smithy-model/src/main/java/software/amazon/smithy/model/selector/RootSelector.java create mode 100644 smithy-model/src/test/java/software/amazon/smithy/model/selector/ShapeTypeCategorySelectorTest.java create mode 100644 smithy-model/src/test/resources/software/amazon/smithy/model/selector/cases/in-function.smithy create mode 100644 smithy-model/src/test/resources/software/amazon/smithy/model/selector/cases/root-function.smithy diff --git a/docs/source-2.0/spec/selectors.rst b/docs/source-2.0/spec/selectors.rst index 86913164a66..fe090d606b1 100644 --- a/docs/source-2.0/spec/selectors.rst +++ b/docs/source-2.0/spec/selectors.rst @@ -1401,6 +1401,88 @@ trait applied to it: service :not(-[trait]-> [trait|protocolDefinition]) +.. _selector-in-function: + +``:in`` +------- + +The ``:in`` function is used to test if a shape is contained within the +result of an expression. This function is most useful when testing if a +:ref:`variable ` or the result of a +:ref:`root ` function contains a shape. The ``:in`` +function requires exactly one selector. If a shape is contained in the +result of evaluating the selector, the shape is yielded from the function. + +The following example finds all numbers that are used in service operation +inputs and not used in service operation outputs: + +.. code-block:: none + :caption: :in example using variables + :name: in-variable-input-output-example + + service + $outputs(~> operation -[output]-> ~> number) + ~> operation -[input]-> ~> number + :not(:in(${outputs})) + +.. note:: + + The above example returns the aggregate results of applying the selector + to every shape: if a model contains multiple services, and one of the + services uses a number 'X' in input and not output, but another service + uses 'X' in both input and output, 'X' is part of the matched shapes. + Use the :ref:`:root function ` to match shapes + globally. + + +.. _selector-root-function: + +``:root`` +--------- + +The ``:root`` function evaluates a subexpression against *all* shapes in the +model and yields all matches. The ``:root`` function is useful for breaking +a selector down into smaller operations, and it works best when used with +:ref:`variables ` or the :ref:`:in function `. +The ``:root`` function requires exactly one selector. + +The following example finds all numbers that are used in any operation inputs +and not used in any operation outputs: + +.. code-block:: none + + number + :in(:root(service ~> operation -[input]-> ~> number)) + :not(:in(:root(service ~> operation -[output]-> ~> number))) + +.. note:: + + The above example is similar to ":ref:`in-variable-input-output-example`" + but works independent of services. That is, if a model contains multiple + services, and one of the services uses a number 'X' in input and not + output, but another service uses 'X' in both input and output, 'X' + *is not* part of the matched shapes. + + +:root functions are isolated subexpressions +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +The expression evaluated by a ``:root`` expression is evaluated in an isolated +context from the rest of the expression. The selector provided to a ``:root`` +function cannot access variables defined outside the function, and variables +defined in the selector do not persist outside the selector. + + +:root functions are evaluated at most once +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +There is no need to store the result of a ``:root`` function in a variable +because ``:root`` selector functions are considered global common +subexpressions and are evaluated at most once during the selection process. +Implementations MAY choose to evaluate ``:root`` expressions eagerly or +lazily, though they MUST evaluate ``:root`` expressions no more than once. + + ``:topdown`` ------------ 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 44630058b38..caeaa02e120 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.Map; import java.util.Set; import java.util.function.Consumer; +import java.util.logging.Logger; import java.util.stream.Stream; import software.amazon.smithy.build.model.SmithyBuildConfig; import software.amazon.smithy.cli.ArgumentReceiver; @@ -41,6 +42,8 @@ final class SelectCommand extends ClasspathCommand { + private static final Logger LOGGER = Logger.getLogger(SelectCommand.class.getName()); + SelectCommand(String parentCommandName, DependencyResolver.Factory dependencyResolverFactory) { super(parentCommandName, dependencyResolverFactory); } @@ -117,6 +120,7 @@ int runWithClassLoader(SmithyBuildConfig config, Arguments arguments, Env env, L Model model = CommandUtils.buildModel(arguments, models, env, env.stderr(), true, config); Selector selector = options.selector(); + long startTime = System.nanoTime(); if (!options.vars()) { sortShapeIds(selector.select(model)).forEach(stdout::println); } else { @@ -130,6 +134,8 @@ int runWithClassLoader(SmithyBuildConfig config, Arguments arguments, Env env, L }); stdout.println(Node.prettyPrintJson(new ArrayNode(result, SourceLocation.NONE))); } + long endTime = System.nanoTime(); + LOGGER.fine(() -> "Select time: " + ((endTime - startTime) / 1000000) + "ms"); return 0; } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/Model.java b/smithy-model/src/main/java/software/amazon/smithy/model/Model.java index 112b33fa8ff..c317a67b346 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/Model.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/Model.java @@ -802,6 +802,16 @@ public boolean contains(Object o) { public Iterator iterator() { return shapeMap.values().iterator(); } + + @Override + public Stream stream() { + return shapes(); + } + + @Override + public Stream parallelStream() { + return shapes().parallel(); + } }; } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/selector/AndSelector.java b/smithy-model/src/main/java/software/amazon/smithy/model/selector/AndSelector.java index fd7cda1c459..c904bc99356 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/selector/AndSelector.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/selector/AndSelector.java @@ -15,7 +15,9 @@ package software.amazon.smithy.model.selector; +import java.util.Collection; import java.util.List; +import software.amazon.smithy.model.Model; import software.amazon.smithy.model.shapes.Shape; /** @@ -33,122 +35,38 @@ private AndSelector() {} static InternalSelector of(List selectors) { switch (selectors.size()) { case 0: - // This happens when selectors are optimized (i.e., the first internal - // selector is a shape type and it gets applied in Model.shape() before - // pushing shapes through the selector. return InternalSelector.IDENTITY; case 1: // If there's only a single selector, then no need to wrap. return selectors.get(0); case 2: - // Cases 2-7 are optimizations that make selectors about - // 40% faster based on JMH benchmarks (at least on my machine, - // JDK 11.0.5, Java HotSpot(TM) 64-Bit Server VM, 11.0.5+10-LTS). - // I stopped at 7 because, it needs to stop somewhere, and it's lucky. - return (c, s, n) -> { - return selectors.get(0).push(c, s, (c2, s2) -> { - return selectors.get(1).push(c2, s2, n); - }); - }; - case 3: - return (c, s, n) -> { - return selectors.get(0).push(c, s, (c2, s2) -> { - return selectors.get(1).push(c2, s2, (c3, s3) -> { - return selectors.get(2).push(c3, s3, n); - }); - }); - }; - case 4: - return (c, s, n) -> { - return selectors.get(0).push(c, s, (c2, s2) -> { - return selectors.get(1).push(c2, s2, (c3, s3) -> { - return selectors.get(2).push(c3, s3, (c4, s4) -> { - return selectors.get(3).push(c4, s4, n); - }); - }); - }); - }; - case 5: - return (c, s, n) -> { - return selectors.get(0).push(c, s, (c2, s2) -> { - return selectors.get(1).push(c2, s2, (c3, s3) -> { - return selectors.get(2).push(c3, s3, (c4, s4) -> { - return selectors.get(3).push(c4, s4, (c5, s5) -> { - return selectors.get(4).push(c5, s5, n); - }); - }); - }); - }); - }; - case 6: - return (c, s, n) -> { - return selectors.get(0).push(c, s, (c2, s2) -> { - return selectors.get(1).push(c2, s2, (c3, s3) -> { - return selectors.get(2).push(c3, s3, (c4, s4) -> { - return selectors.get(3).push(c4, s4, (c5, s5) -> { - return selectors.get(4).push(c5, s5, (c6, s6) -> { - return selectors.get(5).push(c6, s6, n); - }); - }); - }); - }); - }); - }; - case 7: - return (c, s, n) -> { - return selectors.get(0).push(c, s, (c2, s2) -> { - return selectors.get(1).push(c2, s2, (c3, s3) -> { - return selectors.get(2).push(c3, s3, (c4, s4) -> { - return selectors.get(3).push(c4, s4, (c5, s5) -> { - return selectors.get(4).push(c5, s5, (c6, s6) -> { - return selectors.get(5).push(c6, s6, (c7, s7) -> { - return selectors.get(6).push(c7, s7, n); - }); - }); - }); - }); - }); - }); - }; + return new IntermediateAndSelector(selectors.get(0), selectors.get(1)); default: - return new RecursiveAndSelector(selectors); + InternalSelector result = selectors.get(selectors.size() - 1); + for (int i = selectors.size() - 2; i >= 0; i--) { + result = new IntermediateAndSelector(selectors.get(i), result); + } + return result; } } - static final class RecursiveAndSelector implements InternalSelector { - - private final List selectors; - private final int terminalSelectorIndex; + static final class IntermediateAndSelector implements InternalSelector { + private final InternalSelector leftSelector; + private final InternalSelector rightSelector; - private RecursiveAndSelector(List selectors) { - this.selectors = selectors; - this.terminalSelectorIndex = this.selectors.size() - 1; + IntermediateAndSelector(InternalSelector leftSelector, InternalSelector rightSelector) { + this.leftSelector = leftSelector; + this.rightSelector = rightSelector; } @Override - public boolean push(Context context, Shape shape, Receiver next) { - // This is safe since the number of selectors is always >= 2. - return selectors.get(0).push(context, shape, new State(1, next)); + public boolean push(Context ctx, Shape shape, Receiver next) { + return leftSelector.push(ctx, shape, (c, s) -> rightSelector.push(c, s, next)); } - private final class State implements Receiver { - - private final int position; - private final Receiver downstream; - - private State(int position, Receiver downstream) { - this.position = position; - this.downstream = downstream; - } - - @Override - public boolean apply(Context context, Shape shape) { - if (position == terminalSelectorIndex) { - return selectors.get(position).push(context, shape, downstream); - } else { - return selectors.get(position).push(context, shape, new State(position + 1, downstream)); - } - } + @Override + public Collection getStartingShapes(Model model) { + return leftSelector.getStartingShapes(model); } } } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/selector/AttributeSelector.java b/smithy-model/src/main/java/software/amazon/smithy/model/selector/AttributeSelector.java index ba8248ea3cf..765546bba4b 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/selector/AttributeSelector.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/selector/AttributeSelector.java @@ -34,6 +34,7 @@ final class AttributeSelector implements InternalSelector { private final List expected; private final AttributeComparator comparator; private final boolean caseInsensitive; + private final Function> optimizer; AttributeSelector( List path, @@ -54,14 +55,7 @@ final class AttributeSelector implements InternalSelector { this.expected.add(AttributeValue.literal(validValue)); } } - } - static AttributeSelector existence(List path) { - return new AttributeSelector(path, null, null, false); - } - - @Override - public Function> optimize() { // Optimization for loading shapes with a specific trait. // This optimization can only be applied when there's no comparator, // and it doesn't matter how deep into the trait the selector descends. @@ -69,17 +63,26 @@ public Function> optimize() { && path.size() >= 2 && path.get(0).equals("trait") // only match on traits && !path.get(1).startsWith("(")) { // don't match projections - return model -> { + optimizer = model -> { // The trait name might be relative to the prelude, so ensure it's absolute. String absoluteShapeId = Trait.makeAbsoluteName(path.get(1)); ShapeId trait = ShapeId.from(absoluteShapeId); return model.getShapesWithTrait(trait); }; } else { - return null; + optimizer = Model::toSet; } } + static AttributeSelector existence(List path) { + return new AttributeSelector(path, null, null, false); + } + + @Override + public Collection getStartingShapes(Model model) { + return optimizer.apply(model); + } + @Override public boolean push(Context context, Shape shape, Receiver next) { if (matchesAttribute(shape, context)) { diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/selector/Context.java b/smithy-model/src/main/java/software/amazon/smithy/model/selector/Context.java index c1eddd8abb1..6f17bf590ee 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/selector/Context.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/selector/Context.java @@ -16,8 +16,10 @@ package software.amazon.smithy.model.selector; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.Set; +import software.amazon.smithy.model.Model; import software.amazon.smithy.model.knowledge.NeighborProviderIndex; import software.amazon.smithy.model.shapes.Shape; @@ -27,43 +29,31 @@ final class Context { NeighborProviderIndex neighborIndex; - private final Map> variables; + private final Model model; + private final Map> variables = new HashMap<>(); + private final List> roots; - Context(NeighborProviderIndex neighborIndex) { + Context(Model model, NeighborProviderIndex neighborIndex, List> roots) { + this.model = model; this.neighborIndex = neighborIndex; - this.variables = new HashMap<>(); + this.roots = roots; } /** - * Clears the variables stored in the context. + * Gets the mutable map of captured variables. * - * @return Returns the current context. - */ - Context clearVars() { - variables.clear(); - return this; - } - - /** - * Gets the currently set variables. - * - *

Note that this is a mutable array and needs to be copied to - * get a persistent snapshot of the variables. - * - * @return Returns the currently set variables. + * @return Returns the captured variables. */ Map> getVars() { return variables; } - /** - * Puts a variable into the context using a variable name. - * - * @param variable Variable to set. - * @param shapes Shapes to associate with the variable. - */ - void putVar(String variable, Set shapes) { - variables.put(variable, shapes); + Set getRootResult(int index) { + return roots.get(index); + } + + Model getModel() { + return model; } /** diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/selector/InSelector.java b/smithy-model/src/main/java/software/amazon/smithy/model/selector/InSelector.java new file mode 100644 index 00000000000..3bff703005b --- /dev/null +++ b/smithy-model/src/main/java/software/amazon/smithy/model/selector/InSelector.java @@ -0,0 +1,71 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://aws.amazon.com/apache2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +package software.amazon.smithy.model.selector; + +import software.amazon.smithy.model.shapes.Shape; + +/** + * Checks if the given value is in the result of a selector. + */ +final class InSelector implements InternalSelector { + + private final InternalSelector selector; + + InSelector(InternalSelector selector) { + this.selector = selector; + } + + @Override + public boolean push(Context context, Shape shape, Receiver next) { + // Some internal selectors provide optimizations for quickly checking if they contain a shape. + switch (selector.containsShapeOptimization(context, shape)) { + case YES: + return next.apply(context, shape); + case NO: + return true; + case MAYBE: + default: + // Unable to use the optimization, so emit each shape until a match is found. + FilteredHolder holder = new FilteredHolder(shape); + selector.push(context, shape, holder); + + if (holder.matched) { + return next.apply(context, shape); + } + + return true; + } + } + + private static final class FilteredHolder implements InternalSelector.Receiver { + private final Shape shapeToMatch; + private boolean matched; + + FilteredHolder(Shape shapeToMatch) { + this.shapeToMatch = shapeToMatch; + } + + @Override + public boolean apply(Context context, Shape shape) { + if (shape.equals(shapeToMatch)) { + matched = true; + return false; + } + + return true; + } + } +} diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/selector/InternalSelector.java b/smithy-model/src/main/java/software/amazon/smithy/model/selector/InternalSelector.java index a0f12b9bf01..8a6e1ffb76d 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/selector/InternalSelector.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/selector/InternalSelector.java @@ -16,7 +16,6 @@ package software.amazon.smithy.model.selector; import java.util.Collection; -import java.util.function.Function; import software.amazon.smithy.model.Model; import software.amazon.smithy.model.shapes.Shape; @@ -56,18 +55,64 @@ interface InternalSelector { boolean push(Context ctx, Shape shape, Receiver next); /** - * Returns a function that is used to optimize which shapes in a model - * need to be evaluated. + * Pushes {@code shape} through the selector and adds all results to {@code captures}. * - *

For example, when selecting "structure", it is far less work + *

This method exists because we've messed this up multiple times. When buffering values sent to a receiver, + * you have to return true to keep getting results. It's easy to make a closure that just uses + * {@link Collection#add(Object)}, but that will return false if the shape was already in the collection, which + * isn't the desired behavior. + * + * @param context Context being evaluated. + * @param shape Shape being pushed through the selector. + * @param captures Where to buffer all results. + * @param Collection type that is given and returned. + * @return Returns the given {@code captures} collection. + */ + default > C pushResultsToCollection(Context context, Shape shape, C captures) { + push(context, shape, (c, s) -> { + captures.add(s); + return true; + }); + return captures; + } + + /** + * Returns the set of shapes to pump through the selector. + * + *

This method returns all shapes in the model by default. Some selectors can return a subset of shapes if + * the selector can filter shapes more efficiently. For example, when selecting "structure", it is far less work * to leverage {@link Model#toSet(Class)} than it is to send every shape * through every selector. * - * @return Returns a function that returns null if no optimization can - * be made, or a Collection of Shapes if an optimization was made. + * @return Returns the starting shapes to push through the selector. + */ + default Collection getStartingShapes(Model model) { + return model.toSet(); + } + + /** + * The result of determining if a presence optimization can be made to find a shape. + */ + enum ContainsShape { + /** The shape is definitely in the selector. */ + YES, + + /** The shape is definitely not in the selector. */ + NO, + + /** No optimization could be made, so send every shape through to determine if the shape is present. */ + MAYBE + } + + /** + * Checks if the internal selector can quickly detect if it contains the given shape. + * + * @param context Evaluation context. + * @param shape Shape to check. + * @return Returns YES if the selector knows the shape is in the selector, NO if it isn't, and MAYBE if unknown. */ - default Function> optimize() { - return null; + default ContainsShape containsShapeOptimization(Context context, Shape shape) { + return ContainsShape.MAYBE; } /** diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/selector/RootSelector.java b/smithy-model/src/main/java/software/amazon/smithy/model/selector/RootSelector.java new file mode 100644 index 00000000000..62e5c929b06 --- /dev/null +++ b/smithy-model/src/main/java/software/amazon/smithy/model/selector/RootSelector.java @@ -0,0 +1,53 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://aws.amazon.com/apache2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +package software.amazon.smithy.model.selector; + +import software.amazon.smithy.model.shapes.Shape; + +/** + * Root expressions are rooted common subexpressions. + * + *

Roots are evaluated eagerly and then the result is retrieved by ID. This prevents needing to evaluate a root + * expression over and over for each shape given the result does not vary based on the current shape. + * Roots are evaluated in an isolated context, meaning it can't use variables defined outside the root, nor can it + * set variables that can be used outside the root. + */ +final class RootSelector implements InternalSelector { + + private final InternalSelector selector; + private final int id; + + RootSelector(InternalSelector selector, int id) { + this.selector = selector; + this.id = id; + } + + @Override + public boolean push(Context context, Shape shape, Receiver next) { + for (Shape v : context.getRootResult(id)) { + if (!next.apply(context, v)) { + return false; + } + } + + return true; + } + + @Override + public ContainsShape containsShapeOptimization(Context context, Shape shape) { + return context.getRootResult(id).contains(shape) ? ContainsShape.YES : ContainsShape.NO; + } +} diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/selector/SelectorParser.java b/smithy-model/src/main/java/software/amazon/smithy/model/selector/SelectorParser.java index 174b5e1ff21..da322c1afc0 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/selector/SelectorParser.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/selector/SelectorParser.java @@ -39,6 +39,7 @@ final class SelectorParser extends SimpleParser { private static final Logger LOGGER = Logger.getLogger(SelectorParser.class.getName()); private static final Set BREAK_TOKENS = SetUtils.of(',', ']', ')'); private static final Set REL_TYPES = new HashSet<>(); + private final List roots = new ArrayList<>(); static { // Adds selector relationship labels for warnings when unknown relationship names are used. @@ -52,7 +53,9 @@ private SelectorParser(String selector) { } static Selector parse(String selector) { - return new WrappedSelector(selector, new SelectorParser(selector).parse()); + SelectorParser parser = new SelectorParser(selector); + List result = parser.parse(); + return new WrappedSelector(selector, result, parser.roots); } List parse() { @@ -227,6 +230,22 @@ private InternalSelector parseSelectorFunction() { return new TestSelector(selectors); case "is": return IsSelector.of(selectors); + case "in": + if (selectors.size() != 1) { + throw new SelectorSyntaxException( + "The :in function requires a single selector argument", + expression(), functionPosition, line(), column()); + } + return new InSelector(selectors.get(0)); + case "root": + if (selectors.size() != 1) { + throw new SelectorSyntaxException( + "The :root function requires a single selector argument", + expression(), functionPosition, line(), column()); + } + InternalSelector root = new RootSelector(selectors.get(0), roots.size()); + roots.add(selectors.get(0)); + return root; case "topdown": if (selectors.size() > 2) { throw new SelectorSyntaxException( diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/selector/ShapeTypeCategorySelector.java b/smithy-model/src/main/java/software/amazon/smithy/model/selector/ShapeTypeCategorySelector.java index 0b252fef60b..e5c59732afd 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/selector/ShapeTypeCategorySelector.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/selector/ShapeTypeCategorySelector.java @@ -15,6 +15,8 @@ package software.amazon.smithy.model.selector; +import java.util.Collection; +import software.amazon.smithy.model.Model; import software.amazon.smithy.model.shapes.Shape; final class ShapeTypeCategorySelector implements InternalSelector { @@ -32,4 +34,14 @@ public boolean push(Context ctx, Shape shape, Receiver next) { return true; } + + @Override + public Collection getStartingShapes(Model model) { + return model.toSet(shapeCategory); + } + + @Override + public ContainsShape containsShapeOptimization(Context context, Shape shape) { + return getStartingShapes(context.getModel()).contains(shape) ? ContainsShape.YES : ContainsShape.NO; + } } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/selector/ShapeTypeSelector.java b/smithy-model/src/main/java/software/amazon/smithy/model/selector/ShapeTypeSelector.java index 7f6b34e8c96..4dfe285934a 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/selector/ShapeTypeSelector.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/selector/ShapeTypeSelector.java @@ -16,7 +16,6 @@ package software.amazon.smithy.model.selector; import java.util.Collection; -import java.util.function.Function; import software.amazon.smithy.model.Model; import software.amazon.smithy.model.shapes.Shape; import software.amazon.smithy.model.shapes.ShapeType; @@ -39,7 +38,14 @@ public boolean push(Context ctx, Shape shape, Receiver next) { } @Override - public Function> optimize() { - return model -> model.toSet(shapeType.getShapeClass()); + public Collection getStartingShapes(Model model) { + return model.toSet(shapeType.getShapeClass()); + } + + @Override + public ContainsShape containsShapeOptimization(Context context, Shape shape) { + return context.getModel().toSet(shapeType.getShapeClass()).contains(shape) + ? ContainsShape.YES + : ContainsShape.NO; } } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/selector/VariableGetSelector.java b/smithy-model/src/main/java/software/amazon/smithy/model/selector/VariableGetSelector.java index e8f466f7d88..3096e4fe99e 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/selector/VariableGetSelector.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/selector/VariableGetSelector.java @@ -16,6 +16,7 @@ package software.amazon.smithy.model.selector; import java.util.Collections; +import java.util.Set; import software.amazon.smithy.model.shapes.Shape; /** @@ -31,7 +32,7 @@ final class VariableGetSelector implements InternalSelector { @Override public boolean push(Context context, Shape shape, Receiver next) { // Do not fail on an invalid variable access. - for (Shape v : context.getVars().getOrDefault(variableName, Collections.emptySet())) { + for (Shape v : getShapes(context)) { if (!next.apply(context, v)) { // Propagate the signal to stop upstream. return false; @@ -40,4 +41,13 @@ public boolean push(Context context, Shape shape, Receiver next) { return true; } + + private Set getShapes(Context context) { + return context.getVars().getOrDefault(variableName, Collections.emptySet()); + } + + @Override + public ContainsShape containsShapeOptimization(Context context, Shape shape) { + return getShapes(context).contains(shape) ? ContainsShape.YES : ContainsShape.NO; + } } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/selector/VariableStoreSelector.java b/smithy-model/src/main/java/software/amazon/smithy/model/selector/VariableStoreSelector.java index e29906e691f..ba096144ecc 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/selector/VariableStoreSelector.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/selector/VariableStoreSelector.java @@ -41,9 +41,8 @@ final class VariableStoreSelector implements InternalSelector { public boolean push(Context context, Shape shape, Receiver next) { // Buffer the result of piping the shape through the selector // so that it can be retrieved through context vars. - Set captures = new HashSet<>(); - selector.push(context, shape, (c, s) -> captures.add(s)); - context.putVar(variableName, captures); + Set captures = selector.pushResultsToCollection(context, shape, new HashSet<>()); + context.getVars().put(variableName, captures); // Now send the received shape to the next receiver. return next.apply(context, shape); diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/selector/WrappedSelector.java b/smithy-model/src/main/java/software/amazon/smithy/model/selector/WrappedSelector.java index f6579952b64..cc7e26c5ef0 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/selector/WrappedSelector.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/selector/WrappedSelector.java @@ -21,7 +21,6 @@ import java.util.List; import java.util.Set; import java.util.function.Consumer; -import java.util.function.Function; import java.util.stream.Collectors; import java.util.stream.Stream; import software.amazon.smithy.model.Model; @@ -38,12 +37,12 @@ final class WrappedSelector implements Selector { private final String expression; private final InternalSelector delegate; - private final Function> optimizer; + private final List roots; - WrappedSelector(String expression, List selectors) { + WrappedSelector(String expression, List selectors, List roots) { this.expression = expression; - delegate = AndSelector.of(selectors); - optimizer = selectors.get(0).optimize(); + this.roots = roots; + this.delegate = AndSelector.of(selectors); } @Override @@ -90,21 +89,21 @@ public void consumeMatches(Model model, Consumer shapeMatchConsumer) @Override public Stream shapes(Model model) { + NeighborProviderIndex index = NeighborProviderIndex.of(model); + List> computedRoots = computeRoots(model); return streamStartingShape(model).flatMap(shape -> { - List result = new ArrayList<>(); - delegate.push(createContext(model), shape, (ctx, s) -> { - result.add(s); - return true; - }); - return result.stream(); + Context context = new Context(model, index, computedRoots); + return delegate.pushResultsToCollection(context, shape, new ArrayList<>()).stream(); }); } @Override public Stream matches(Model model) { + NeighborProviderIndex index = NeighborProviderIndex.of(model); + List> computedRoots = computeRoots(model); return streamStartingShape(model).flatMap(shape -> { List result = new ArrayList<>(); - delegate.push(createContext(model), shape, (ctx, s) -> { + delegate.push(new Context(model, index, computedRoots), shape, (ctx, s) -> { result.add(new ShapeMatch(s, ctx.getVars())); return true; }); @@ -112,31 +111,52 @@ public Stream matches(Model model) { }); } - private Context createContext(Model model) { - return new Context(NeighborProviderIndex.of(model)); + // Eagerly compute roots over all model shapes before evaluating shapes one at a time. + private List> computeRoots(Model model) { + NeighborProviderIndex index = NeighborProviderIndex.of(model); + List> rootResults = new ArrayList<>(roots.size()); + for (InternalSelector selector : roots) { + Set result = evalRoot(model, index, selector, rootResults); + rootResults.add(result); + } + return rootResults; + } + + // Eagerly compute a root subexpression. + private Set evalRoot( + Model model, + NeighborProviderIndex index, + InternalSelector selector, + List> results + ) { + Collection shapesToEmit = selector.getStartingShapes(model); + Context isolatedContext = new Context(model, index, results); + Set captures = new HashSet<>(); + for (Shape rootShape : shapesToEmit) { + isolatedContext.getVars().clear(); + selector.push(isolatedContext, rootShape, (c, s) -> { + captures.add(s); + return true; + }); + } + + return captures; } private void pushShapes(Model model, InternalSelector.Receiver acceptor) { - Context context = createContext(model); - Collection shapes = optimizer == null - ? model.toSet() - : optimizer.apply(model); + Context context = new Context(model, NeighborProviderIndex.of(model), computeRoots(model)); + Collection shapes = delegate.getStartingShapes(model); for (Shape shape : shapes) { - delegate.push(context.clearVars(), shape, acceptor); + context.getVars().clear(); + delegate.push(context, shape, acceptor); } } private Stream streamStartingShape(Model model) { - Stream stream = optimizer != null - ? optimizer.apply(model).stream() - : model.shapes(); - - // Use a parallel stream for larger models. - if (isParallel(model)) { - stream = stream.parallel(); - } - - return stream; + Collection startingShapes = delegate.getStartingShapes(model); + return startingShapes.size() > PARALLEL_THRESHOLD + ? startingShapes.parallelStream() + : startingShapes.stream(); } private boolean isParallel(Model model) { diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/selector/SelectorTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/selector/SelectorTest.java index ecd999bdb51..130114fe7ff 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/selector/SelectorTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/selector/SelectorTest.java @@ -45,7 +45,6 @@ import software.amazon.smithy.model.shapes.MemberShape; import software.amazon.smithy.model.shapes.ResourceShape; import software.amazon.smithy.model.shapes.ServiceShape; -import software.amazon.smithy.model.shapes.SetShape; import software.amazon.smithy.model.shapes.Shape; import software.amazon.smithy.model.shapes.ShapeId; import software.amazon.smithy.model.shapes.StringShape; @@ -1116,4 +1115,52 @@ public void supportsResourceProperties() { assertThat(shapesTargettedByCityOnly.size(), equalTo(2)); assertThat(shapesTargettedByCityOnly, containsInAnyOrder(coordinatesShape, stringShape)); } + + @Test + public void rootFunctionReturnsAllShapes() { + Selector selector = Selector.parse("string" + + ":in(:root(-[input]-> ~> *))" + + ":not(:in(:root(-[output]-> ~> *)))"); + Set result = selector.select(resourceModel); + + // This is the only string used in input but not output. + assertThat(result, contains(resourceModel.expectShape(ShapeId.from("example.weather#CityId")))); + } + + @Test + public void inefficientIfNotCached() { + Selector selector = Selector.parse(":in(:root(service ~> number))"); + Set result = selector.select(resourceModel); + + // This is the only number used in any service. + assertThat(result, contains(resourceModel.expectShape(ShapeId.from("smithy.api#Float")))); + } + + @Test + public void allowsNestedRoots() { + Selector selector = Selector.parse(":root(:root(:root(*)))"); + Set result = selector.select(resourceModel); + + assertThat(result, equalTo(resourceModel.toSet())); + } + + @Test + public void inDoesNotSupportMoreThanOneSelector() { + Assertions.assertThrows(SelectorSyntaxException.class, () -> Selector.parse(":in(*, *)")); + } + + @Test + public void inRequiresOneSelector() { + Assertions.assertThrows(SelectorSyntaxException.class, () -> Selector.parse(":in()")); + } + + @Test + public void rootDoesNotSupportMoreThanOneSelector() { + Assertions.assertThrows(SelectorSyntaxException.class, () -> Selector.parse(":root(*, *)")); + } + + @Test + public void rootRequiresOneSelector() { + Assertions.assertThrows(SelectorSyntaxException.class, () -> Selector.parse(":root()")); + } } diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/selector/ShapeTypeCategorySelectorTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/selector/ShapeTypeCategorySelectorTest.java new file mode 100644 index 00000000000..dfebd6adacf --- /dev/null +++ b/smithy-model/src/test/java/software/amazon/smithy/model/selector/ShapeTypeCategorySelectorTest.java @@ -0,0 +1,46 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://aws.amazon.com/apache2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +package software.amazon.smithy.model.selector; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.in; + +import java.util.Set; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; +import software.amazon.smithy.model.Model; + +public class ShapeTypeCategorySelectorTest { + + private static Model model; + + @BeforeAll + public static void before() { + model = Model.assembler() + .addImport(SelectorTest.class.getResource("shape-type-test.smithy")) + .assemble() + .unwrap(); + } + + @Test + public void hasContainsOptimization() { + // "number" is a category. intEnum is considered a number, so it is returned. This example triggers the + // :in function optimization of the selector. + Set ids = SelectorTest.ids(model, ":in(number) [id|namespace = smithy.example]"); + + assertThat("smithy.example#IntEnum", in(ids)); + } +} diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/selector/ShapeTypeSelectorTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/selector/ShapeTypeSelectorTest.java index 01637972259..42888a161ba 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/selector/ShapeTypeSelectorTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/selector/ShapeTypeSelectorTest.java @@ -50,4 +50,11 @@ public void integerSelectsIntEnum() { assertThat("smithy.example#Integer", in(ids)); assertThat("smithy.example#IntEnum", in(ids)); } + + @Test + public void hasContainsOptimization() { + Set ids = SelectorTest.ids(model, ":in(enum) [id|namespace = smithy.example]"); + + assertThat("smithy.example#Enum", in(ids)); + } } diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/selector/cases/in-function.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/selector/cases/in-function.smithy new file mode 100644 index 00000000000..921584ca911 --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/selector/cases/in-function.smithy @@ -0,0 +1,73 @@ +$version: "2.0" + +metadata selectorTests = [ + // Find numbers that are used in input but are not used in output. + { + selector: """ + service + $output(~> operation -[output]-> ~> number) + ~> operation -[input]-> ~> number + :not(:in(${output}))""" + matches: [ + smithy.api#Integer + smithy.api#Double + ] + } + // This is a pointless example, but does test that :in can be used with variables. This should be written as: + // operation ~> number + { + selector: """ + $usedNumbers(operation ~> number) + operation ~> * + :in(${usedNumbers})""" + matches: [ + smithy.api#Integer + smithy.api#Float + smithy.api#Double + smithy.api#Short + smithy.api#Long + smithy.api#Byte + ] + } + // Another pointless usage of in, but valid. This should be written more directly as: + // member [id|namespace = smithy.example] > number + { + selector: ":in(number :test(< member [id|namespace = smithy.example]))" + matches: [ + smithy.api#Integer + smithy.api#Float + smithy.api#Double + smithy.api#Short + smithy.api#Long + smithy.api#Byte + ] + } +] + +namespace smithy.example + +service MyService { + operations: [A, B] +} + +operation A { + input:= { + a: Integer + b: Float + c: Double + } + output:= { + a: Short + b: Long + c: Float + } +} + +operation B { + input:= { + a: Byte + } + output:= { + b: Byte + } +} diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/selector/cases/root-function.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/selector/cases/root-function.smithy new file mode 100644 index 00000000000..c087e774c77 --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/selector/cases/root-function.smithy @@ -0,0 +1,61 @@ +$version: "2.0" + +metadata selectorTests = [ + // Find shapes used in service operation inputs but not operation outputs. + { + selector: """ + number + :in(:root(service ~> operation -[input]-> * ~> number)) + :not(:in(:root(service ~> operation -[output]-> * ~> number)))""" + matches: [ + smithy.api#Integer + ] + } + // This is similar to the above :root example, but also returns smithy.api#Double because each capture + // of service operations is isolate to a single service and not global across all services. When MyService1 + // is evaluated, Double is only used in input and not output. The usage of Double in the output of MyService2 + // is not taken into account when evaluating MyService1. + { + selector: """ + service + $outputs(~> operation -[output]-> ~> number) + ~> operation -[input]-> ~> number + :not(:in(${outputs}))""" + matches: [ + smithy.api#Integer + smithy.api#Double + ] + } +] + +namespace smithy.example + +service MyService1 { + operations: [A] +} + +operation A { + input:= { + a: Integer + b: Float + c: Double + } + output:= { + d: Float + e: Short + f: Long + } +} + +service MyService2 { + operations: [B] +} + +operation B { + input:= { + a: Double + } + output:= { + a: Double + } +}