Make SkyQuery's rdeps->allrdeps transformations apply when `--universe_scope` uses a "all targets" pattern and the universe of the `rdeps` expression uses a "rules only" pattern.
PiperOrigin-RevId: 361640819
diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java b/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java
index 21eb22f..c3fc185 100644
--- a/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java
+++ b/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java
@@ -224,11 +224,21 @@
}
/**
- * For patterns of type {@link Type#SINGLE_TARGET} and {@link Type#TARGETS_IN_PACKAGE}, returns
- * the {@link PackageIdentifier} corresponding to the package that would contain the target(s)
- * matched by this {@link TargetPattern}.
+ * For patterns of type {@link Type#SINGLE_TARGET}, {@link Type#TARGETS_IN_PACKAGE}, and {@link
+ * Type#TARGETS_BELOW_DIRECTORY}, returns the {@link PackageIdentifier} of the pattern.
+ *
+ * <p>Note that we are using the {@link PackageIdentifier} type as a convenience; there may not
+ * actually be a package corresponding to this directory!
+ *
+ * <p>Examples:
+ *
+ * <ul>
+ * <li>For pattern {@code //foo:bar}, returns package identifier {@code //foo}.
+ * <li>For pattern {@code //foo:all}, returns package identifier {@code //foo}.
+ * <li>For pattern {@code //foo/...}, returns package identifier {@code //foo}.
+ * </ul>
*/
- public PackageIdentifier getDirectoryForTargetOrTargetsInPackage() {
+ public PackageIdentifier getDirectory() {
throw new IllegalStateException();
}
@@ -269,7 +279,7 @@
}
@Override
- public PackageIdentifier getDirectoryForTargetOrTargetsInPackage() {
+ public PackageIdentifier getDirectory() {
return directory;
}
@@ -439,7 +449,7 @@
}
@Override
- public PackageIdentifier getDirectoryForTargetOrTargetsInPackage() {
+ public PackageIdentifier getDirectory() {
return packageIdentifier;
}
@@ -762,16 +772,7 @@
NOT_CONTAINED,
}
- /**
- * Returns a {@link PackageIdentifier} identifying the most specific containing directory of the
- * patterns that could be matched by this pattern.
- *
- * <p>Note that we are using the {@link PackageIdentifier} type as a convenience; there may not
- * actually be a package corresponding to this directory!
- *
- * <p>This returns a {@link PackageIdentifier} that identifies the referred-to directory. For
- * example, for "//foo/bar/...", this method returns a {@link PackageIdentifier} for "foo/bar".
- */
+ @Override
public PackageIdentifier getDirectory() {
return directory;
}
diff --git a/src/main/java/com/google/devtools/build/lib/query2/FilteredDirectRdepsInUniverseExpressionMapper.java b/src/main/java/com/google/devtools/build/lib/query2/FilteredDirectRdepsInUniverseExpressionMapper.java
index b652a82..b4bcd71 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/FilteredDirectRdepsInUniverseExpressionMapper.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/FilteredDirectRdepsInUniverseExpressionMapper.java
@@ -17,6 +17,7 @@
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.cmdline.TargetPattern;
+import com.google.devtools.build.lib.query2.RdepsToAllRdepsQueryExpressionMapper.Eligibility;
import com.google.devtools.build.lib.query2.engine.BinaryOperatorExpression;
import com.google.devtools.build.lib.query2.engine.FunctionExpression;
import com.google.devtools.build.lib.query2.engine.LetExpression;
@@ -31,6 +32,7 @@
import com.google.devtools.build.lib.query2.engine.TargetLiteral;
import java.util.ArrayList;
import java.util.List;
+import javax.annotation.Nullable;
/**
* A {@link QueryExpressionMapper} that transforms each occurrence of an expression of the form
@@ -64,13 +66,13 @@
*/
public class FilteredDirectRdepsInUniverseExpressionMapper extends QueryExpressionMapper<Void> {
private final TargetPattern.Parser targetPatternParser;
- private final String absoluteUniverseScopePattern;
+ private final TargetPattern absoluteUniverseScopePattern;
@VisibleForTesting
public FilteredDirectRdepsInUniverseExpressionMapper(
- TargetPattern.Parser targetPatternParser, String universeScopePattern) {
+ TargetPattern.Parser targetPatternParser, TargetPattern absoluteUniverseScopePattern) {
this.targetPatternParser = targetPatternParser;
- this.absoluteUniverseScopePattern = targetPatternParser.absolutize(universeScopePattern);
+ this.absoluteUniverseScopePattern = absoluteUniverseScopePattern;
}
@Override
@@ -79,13 +81,18 @@
List<Argument> args = functionExpression.getArgs();
// This transformation only applies to the 3-arg form of rdeps, with depth == 1.
if (args.size() == 3 && args.get(2).getInteger() == 1) {
- List<FunctionExpression> universeFilteringFunctions =
- args.get(0).getExpression().accept(new ExtractFilteringFunctionsFromUniverseVisitor());
- // If we get back a non-empty list of FunctionExpressions, these are filtering functions
- // that can be safely factored out.
- if (!universeFilteringFunctions.isEmpty()) {
- QueryExpression curFunction = makeUnfilteredRdepsWithDepthOne(args.get(1));
- for (FunctionExpression filterExpr : universeFilteringFunctions) {
+ Argument rdepsUniverseArgument = args.get(0);
+ QueryExpression rdepsUniverseExpression = rdepsUniverseArgument.getExpression();
+ ExtractFilteringFunctionsResult result =
+ rdepsUniverseExpression.accept(new ExtractFilteringFunctionsFromUniverseVisitor());
+ // If we get back a non-empty result, then there are filtering functions that can be safely
+ // factored out.
+ if (result.universeArgument != null) {
+ QueryExpression curFunction =
+ makeUnfilteredRdepsWithDepthOne(
+ /*rdepsUniverseArgument=*/ result.universeArgument,
+ /*sourceArgument=*/ args.get(1));
+ for (FunctionExpression filterExpr : result.filteringExpressions) {
curFunction = wrapExprWithFilter(curFunction, filterExpr);
}
return curFunction;
@@ -95,11 +102,11 @@
return super.visit(functionExpression, context);
}
- private FunctionExpression makeUnfilteredRdepsWithDepthOne(Argument target) {
+ private FunctionExpression makeUnfilteredRdepsWithDepthOne(
+ Argument rdepsUniverseArgument, Argument sourceArgument) {
return new FunctionExpression(
new RdepsFunction(),
- ImmutableList.of(
- Argument.of(new TargetLiteral(absoluteUniverseScopePattern)), target, Argument.of(1)));
+ ImmutableList.of(rdepsUniverseArgument, sourceArgument, Argument.of(1)));
}
private static QueryExpression wrapExprWithFilter(
@@ -117,63 +124,86 @@
return new FunctionExpression(filteringFunction, rewrittenArgs);
}
+ private static class ExtractFilteringFunctionsResult {
+ private static final ExtractFilteringFunctionsResult EMPTY =
+ new ExtractFilteringFunctionsResult(ImmutableList.of(), null);
+
+ private final ImmutableList<FunctionExpression> filteringExpressions;
+ @Nullable private final Argument universeArgument;
+
+ private ExtractFilteringFunctionsResult(
+ ImmutableList<FunctionExpression> filteringExpressions,
+ @Nullable Argument universeArgument) {
+ this.filteringExpressions = filteringExpressions;
+ this.universeArgument = universeArgument;
+ }
+ }
+
/**
* Internal visitor applied to the universe argument of all QueryExpressions of the form {@literal
* rdeps(u, x, 1)}.
*/
private class ExtractFilteringFunctionsFromUniverseVisitor
- implements QueryExpressionVisitor<List<FunctionExpression>, Void> {
+ implements QueryExpressionVisitor<ExtractFilteringFunctionsResult, Void> {
@Override
- public List<FunctionExpression> visit(TargetLiteral targetLiteral, Void context) {
- return ImmutableList.of();
+ public ExtractFilteringFunctionsResult visit(TargetLiteral targetLiteral, Void context) {
+ return ExtractFilteringFunctionsResult.EMPTY;
}
@Override
- public List<FunctionExpression> visit(
+ public ExtractFilteringFunctionsResult visit(
BinaryOperatorExpression binaryOperatorExpression, Void context) {
- return ImmutableList.of();
+ return ExtractFilteringFunctionsResult.EMPTY;
}
@SuppressWarnings("MixedMutabilityReturnType")
@Override
- public List<FunctionExpression> visit(FunctionExpression functionExpression, Void context) {
+ public ExtractFilteringFunctionsResult visit(
+ FunctionExpression functionExpression, Void context) {
FilteringQueryFunction filteringFunction =
functionExpression.getFunction().asFilteringFunction();
if (filteringFunction == null) {
- return ImmutableList.of();
+ return ExtractFilteringFunctionsResult.EMPTY;
}
Argument filteredArgument =
functionExpression.getArgs().get(filteringFunction.getExpressionToFilterIndex());
Preconditions.checkArgument(filteredArgument.getType() == ArgumentType.EXPRESSION);
QueryExpression filteredExpression = filteredArgument.getExpression();
- ArrayList<FunctionExpression> results = new ArrayList<>();
if (filteredExpression instanceof TargetLiteral) {
- TargetLiteral literalUniverseExpression = (TargetLiteral) filteredExpression;
- String absolutizedUniverseExpression =
- targetPatternParser.absolutize(literalUniverseExpression.getPattern());
- if (absolutizedUniverseExpression.equals(absoluteUniverseScopePattern)) {
- results.add(functionExpression);
+ Eligibility eligibility =
+ RdepsToAllRdepsQueryExpressionMapper.determineEligibility(
+ targetPatternParser,
+ absoluteUniverseScopePattern,
+ ((TargetLiteral) filteredExpression).getPattern());
+ if (eligibility == Eligibility.ELIGIBLE_AS_IS
+ || eligibility == Eligibility.ELIGIBLE_WITH_FILTERING) {
+ return new ExtractFilteringFunctionsResult(
+ ImmutableList.of(functionExpression), filteredArgument);
}
} else if (filteredExpression instanceof FunctionExpression) {
- List<FunctionExpression> nestedFilters = filteredExpression.accept(this);
- if (!nestedFilters.isEmpty()) {
- results.addAll(nestedFilters);
- results.add(functionExpression);
+ ExtractFilteringFunctionsResult recursiveResult = filteredExpression.accept(this);
+ if (recursiveResult.universeArgument != null) {
+ return new ExtractFilteringFunctionsResult(
+ /*filteringExpressions=*/ ImmutableList.<FunctionExpression>builder()
+ .addAll(recursiveResult.filteringExpressions)
+ .add(functionExpression)
+ .build(),
+ recursiveResult.universeArgument);
}
}
- return results;
+ return ExtractFilteringFunctionsResult.EMPTY;
}
@Override
- public List<FunctionExpression> visit(LetExpression letExpression, Void context) {
- return ImmutableList.of();
+ public ExtractFilteringFunctionsResult visit(LetExpression letExpression, Void context) {
+ return ExtractFilteringFunctionsResult.EMPTY;
}
@Override
- public List<FunctionExpression> visit(SetExpression setExpression, Void context) {
- return ImmutableList.of();
+ public ExtractFilteringFunctionsResult visit(SetExpression setExpression, Void context) {
+ return ExtractFilteringFunctionsResult.EMPTY;
}
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/query2/RdepsToAllRdepsQueryExpressionMapper.java b/src/main/java/com/google/devtools/build/lib/query2/RdepsToAllRdepsQueryExpressionMapper.java
index 3146ef6..5d834da 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/RdepsToAllRdepsQueryExpressionMapper.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/RdepsToAllRdepsQueryExpressionMapper.java
@@ -13,9 +13,12 @@
// limitations under the License.
package com.google.devtools.build.lib.query2;
+import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.cmdline.TargetParsingException;
import com.google.devtools.build.lib.cmdline.TargetPattern;
import com.google.devtools.build.lib.query2.engine.AllRdepsFunction;
import com.google.devtools.build.lib.query2.engine.FunctionExpression;
+import com.google.devtools.build.lib.query2.engine.KindFunction;
import com.google.devtools.build.lib.query2.engine.QueryEnvironment.Argument;
import com.google.devtools.build.lib.query2.engine.QueryExpression;
import com.google.devtools.build.lib.query2.engine.QueryExpressionMapper;
@@ -30,29 +33,106 @@
*/
class RdepsToAllRdepsQueryExpressionMapper extends QueryExpressionMapper<Void> {
private final TargetPattern.Parser targetPatternParser;
- private final String absoluteUniverseScopePattern;
+ private final TargetPattern absoluteUniverseScopePattern;
RdepsToAllRdepsQueryExpressionMapper(
- TargetPattern.Parser targetPatternParser, String universeScopePattern) {
+ TargetPattern.Parser targetPatternParser, TargetPattern absoluteUniverseScopePattern) {
this.targetPatternParser = targetPatternParser;
- this.absoluteUniverseScopePattern = targetPatternParser.absolutize(universeScopePattern);
+ this.absoluteUniverseScopePattern = absoluteUniverseScopePattern;
}
@Override
public QueryExpression visit(FunctionExpression functionExpression, Void context) {
if (functionExpression.getFunction().getName().equals(new RdepsFunction().getName())) {
List<Argument> args = functionExpression.getArgs();
- QueryExpression universeExpression = args.get(0).getExpression();
- if (universeExpression instanceof TargetLiteral) {
- TargetLiteral literalUniverseExpression = (TargetLiteral) universeExpression;
- String absolutizedUniverseExpression =
- targetPatternParser.absolutize(literalUniverseExpression.getPattern());
- if (absolutizedUniverseExpression.equals(absoluteUniverseScopePattern)) {
- List<Argument> argsTail = args.subList(1, functionExpression.getArgs().size());
- return new FunctionExpression(new AllRdepsFunction(), argsTail);
+ QueryExpression rdepsUniverseExpression = args.get(0).getExpression();
+ if (rdepsUniverseExpression instanceof TargetLiteral) {
+ Eligibility eligibility =
+ determineEligibility(
+ targetPatternParser,
+ absoluteUniverseScopePattern,
+ ((TargetLiteral) rdepsUniverseExpression).getPattern());
+ switch (eligibility) {
+ case ELIGIBLE_AS_IS:
+ return new FunctionExpression(
+ new AllRdepsFunction(), args.subList(1, functionExpression.getArgs().size()));
+ case ELIGIBLE_WITH_FILTERING:
+ return new FunctionExpression(
+ new KindFunction(),
+ ImmutableList.of(
+ Argument.of(" rule$"),
+ Argument.of(
+ new FunctionExpression(
+ new AllRdepsFunction(),
+ args.subList(1, functionExpression.getArgs().size())))));
+ default:
+ // Do nothing. The return statement at the bottom of the method is what we want.
}
}
}
return super.visit(functionExpression, context);
}
+
+ /**
+ * Describes how eligible, if at all, a `rdeps(pattern, E, d)` expression is for being transformed
+ * to one that uses `allrdeps`.
+ */
+ enum Eligibility {
+ NOT_ELIGIBLE,
+
+ ELIGIBLE_WITH_FILTERING,
+
+ ELIGIBLE_AS_IS,
+ }
+
+ static Eligibility determineEligibility(
+ TargetPattern.Parser targetPatternParser,
+ TargetPattern absoluteUniverseScopePattern,
+ String rdepsUniversePatternString) {
+ TargetPattern absoluteRdepsUniverseTargetPattern;
+ try {
+ absoluteRdepsUniverseTargetPattern =
+ targetPatternParser.parse(targetPatternParser.absolutize(rdepsUniversePatternString));
+ } catch (TargetParsingException e) {
+ return Eligibility.NOT_ELIGIBLE;
+ }
+
+ if (absoluteUniverseScopePattern.getType() != absoluteRdepsUniverseTargetPattern.getType()) {
+ return Eligibility.NOT_ELIGIBLE;
+ }
+
+ switch (absoluteUniverseScopePattern.getType()) {
+ case PATH_AS_TARGET:
+ case SINGLE_TARGET:
+ return absoluteUniverseScopePattern
+ .getOriginalPattern()
+ .equals(absoluteRdepsUniverseTargetPattern.getOriginalPattern())
+ ? Eligibility.ELIGIBLE_AS_IS
+ : Eligibility.NOT_ELIGIBLE;
+ case TARGETS_IN_PACKAGE:
+ case TARGETS_BELOW_DIRECTORY:
+ if (!absoluteUniverseScopePattern
+ .getDirectory()
+ .equals(absoluteRdepsUniverseTargetPattern.getDirectory())) {
+ return Eligibility.NOT_ELIGIBLE;
+ }
+
+ // Note: If we're here, both patterns are either TARGETS_IN_PACKAGE or
+ // TARGETS_BELOW_DIRECTORY, and are for the same directory.
+
+ if (absoluteUniverseScopePattern.getRulesOnly()
+ == absoluteRdepsUniverseTargetPattern.getRulesOnly()) {
+ return Eligibility.ELIGIBLE_AS_IS;
+ }
+
+ return absoluteUniverseScopePattern.getRulesOnly()
+ // If the actual universe is narrower, then allrdeps would be unsound because it may
+ // produce narrower results.
+ ? Eligibility.NOT_ELIGIBLE
+ // If the actual universe is wider, then allrdeps would produce wider results.
+ // Therefore, we'd want to filter those results.
+ : Eligibility.ELIGIBLE_WITH_FILTERING;
+ }
+ throw new IllegalStateException(absoluteUniverseScopePattern.getType().toString());
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
index 3c90bc5..3e561a2 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
@@ -376,12 +376,20 @@
return QueryExpressionMapper.identity();
}
TargetPattern.Parser targetPatternParser = new TargetPattern.Parser(parserPrefix);
- String universeScopePattern = Iterables.getOnlyElement(constantUniverseScopeList);
+ String universeScopePatternString = Iterables.getOnlyElement(constantUniverseScopeList);
+ TargetPattern absoluteUniverseScopePattern = null;
+ try {
+ absoluteUniverseScopePattern =
+ targetPatternParser.parse(targetPatternParser.absolutize(universeScopePatternString));
+ } catch (TargetParsingException e) {
+ return QueryExpressionMapper.identity();
+ }
return QueryExpressionMapper.compose(
ImmutableList.of(
- new RdepsToAllRdepsQueryExpressionMapper(targetPatternParser, universeScopePattern),
+ new RdepsToAllRdepsQueryExpressionMapper(
+ targetPatternParser, absoluteUniverseScopePattern),
new FilteredDirectRdepsInUniverseExpressionMapper(
- targetPatternParser, universeScopePattern)));
+ targetPatternParser, absoluteUniverseScopePattern)));
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/query2/engine/KindFunction.java b/src/main/java/com/google/devtools/build/lib/query2/engine/KindFunction.java
index 4030ceb..4d3f82a 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/engine/KindFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/engine/KindFunction.java
@@ -20,12 +20,13 @@
import java.util.List;
/**
- * A kind(pattern, argument) filter expression, which computes the set of subset
- * of nodes in 'argument' whose kind matches the unanchored regexp 'pattern'.
+ * A kind(pattern, argument) filter expression, which computes the set of subset of nodes in
+ * 'argument' whose kind matches the unanchored regexp 'pattern'.
*
* <pre>expr ::= KIND '(' WORD ',' expr ')'</pre>
*
* Example patterns:
+ *
* <pre>
* ' file' Match all file targets.
* 'source file' Match all test source file targets.
@@ -35,11 +36,11 @@
* 'test' Match all test (rule) targets.
* </pre>
*
- * Note, the space before "file" is needed to prevent unwanted matches against
- * (e.g.) "filegroup rule".
+ * Note, the space before "file" is needed to prevent unwanted matches against (e.g.) "filegroup
+ * rule".
*/
-class KindFunction extends RegexFilterExpression {
- KindFunction() {
+public class KindFunction extends RegexFilterExpression {
+ public KindFunction() {
super(/*invert=*/ false);
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java
index 623044d..4b1094a 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java
@@ -228,9 +228,7 @@
private SimpleLookup(String pattern, TargetPatternKey key) {
this(
- pattern,
- PackageValue.key(key.getParsedPattern().getDirectoryForTargetOrTargetsInPackage()),
- key.getParsedPattern());
+ pattern, PackageValue.key(key.getParsedPattern().getDirectory()), key.getParsedPattern());
}
private SimpleLookup(String pattern, PackageValue.Key key, TargetPattern targetPattern) {
