Remove special handling of visibility dependencies in DependencyResolver.
This is done by allowing the analysis of a configured target with a null configuration to proceed a little further than before: we start the ConfiguredTargetFunction, but we short-circuit it as soon as we know that it won't work out.
RELNOTES: None.
PiperOrigin-RevId: 234465687
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
index f6d2dbf..8d1df1f 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
@@ -54,7 +54,6 @@
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
-import java.util.Set;
import javax.annotation.Nullable;
/**
@@ -135,6 +134,10 @@
}
}
+ /**
+ * What we know about a dependency edge after factoring in the properties of the configured target
+ * that the edge originates from, but not the properties of target it points to.
+ */
@AutoValue
abstract static class PartiallyResolvedDependency {
public abstract Label getLabel();
@@ -276,8 +279,6 @@
OrderedSetMultimap<DependencyKind, PartiallyResolvedDependency> partiallyResolvedDeps =
partiallyResolveDependencies(outgoingLabels, fromRule, attributeMap, aspects);
- filterIllegalVisibilityDependencies(node, targetMap, partiallyResolvedDeps);
-
OrderedSetMultimap<DependencyKind, Dependency> outgoingEdges =
fullyResolveDependencies(
partiallyResolvedDeps, targetMap, node.getConfiguration(), trimmingTransitionFactory);
@@ -322,12 +323,9 @@
}
if (entry.getKey() == VISIBILITY_DEPENDENCY) {
- // Package groups should be analyzed with the null configuration, but we don't know if this
- // is actually a package group. So just use no transition until it's known whether that
- // assumption is correct.
partiallyResolvedDeps.put(
VISIBILITY_DEPENDENCY,
- PartiallyResolvedDependency.of(toLabel, NoTransition.INSTANCE, ImmutableList.of()));
+ PartiallyResolvedDependency.of(toLabel, NullTransition.INSTANCE, ImmutableList.of()));
continue;
}
@@ -356,49 +354,14 @@
}
/**
- * Filter out visibility dependencies that don't point to package groups.
- *
- * <p>This should really be done where we filter other illegal kinds of dependencies (e.g. on the
- * wrong rule class), but we'll either have to figure out a way to report the common mistake of
- * the visibility attribute of a rule referring to the rule that depends on it (instead of its
- * package) or decide to live without nice reporting in that case.
- */
- private void filterIllegalVisibilityDependencies(
- TargetAndConfiguration node,
- Map<Label, Target> targetMap,
- OrderedSetMultimap<DependencyKind, PartiallyResolvedDependency> partiallyResolvedDeps) {
- Set<PartiallyResolvedDependency> illegalVisibilityDeps = new LinkedHashSet<>();
- for (PartiallyResolvedDependency dep : partiallyResolvedDeps.get(VISIBILITY_DEPENDENCY)) {
- Target toTarget = targetMap.get(dep.getLabel());
- if (toTarget == null) {
- // Dependency pointing to non-existent target. This error was reported above, so we can just
- // ignore this dependency.
- }
-
- if (!(toTarget instanceof PackageGroup)) {
- // Note that this error could also be caught in
- // AbstractConfiguredTarget.convertVisibility(), but we have an
- // opportunity here to avoid dependency cycles that result from
- // the visibility attribute of a rule referring to a rule that
- // depends on it (instead of its package)
- invalidPackageGroupReferenceHook(node, dep.getLabel());
- illegalVisibilityDeps.add(dep);
- }
- }
-
- for (PartiallyResolvedDependency illegalVisibilityDep : illegalVisibilityDeps) {
- partiallyResolvedDeps.remove(VISIBILITY_DEPENDENCY, illegalVisibilityDep);
- }
- }
-
- /**
* Factor in the properties of the target where the dependency points to in the dependency edge
* calculation.
*
* <p>The target of the dependency edges depends on two things: the rule that depends on them and
* the type of target they depend on. This function takes the rule into account. Accordingly, it
* should <b>NOT</b> get the {@link Rule} instance representing the rule whose dependencies are
- * being calculated as an argument or its attributes.
+ * being calculated as an argument or its attributes and it should <b>NOT</b> do anything with the
+ * keys of {@code partiallyResolvedDeps} other than passing them on to the output map.
*/
private OrderedSetMultimap<DependencyKind, Dependency> fullyResolveDependencies(
OrderedSetMultimap<DependencyKind, PartiallyResolvedDependency> partiallyResolvedDeps,
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/EmptyConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/analysis/EmptyConfiguredTarget.java
new file mode 100644
index 0000000..90ef920
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/analysis/EmptyConfiguredTarget.java
@@ -0,0 +1,42 @@
+// Copyright 2019 The Bazel Authors. 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.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License 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 com.google.devtools.build.lib.analysis;
+
+import com.google.devtools.build.lib.analysis.configuredtargets.AbstractConfiguredTarget;
+import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
+import com.google.devtools.build.lib.collect.nestedset.Order;
+import com.google.devtools.build.lib.packages.InfoInterface;
+import com.google.devtools.build.lib.packages.Provider;
+import com.google.devtools.build.lib.skyframe.BuildConfigurationValue;
+import javax.annotation.Nullable;
+
+/** A configured target that is empty. */
+public class EmptyConfiguredTarget extends AbstractConfiguredTarget {
+ public EmptyConfiguredTarget(Label label, BuildConfigurationValue.Key configurationKey) {
+ super(label, configurationKey, NestedSetBuilder.emptySet(Order.STABLE_ORDER));
+ }
+
+ @Nullable
+ @Override
+ protected InfoInterface rawGetSkylarkProvider(Provider.Key providerKey) {
+ return null;
+ }
+
+ @Override
+ protected Object rawGetSkylarkProvider(String providerKey) {
+ return null;
+ }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/TransitionResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/config/TransitionResolver.java
index 4fb189e..c057826a 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/TransitionResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/TransitionResolver.java
@@ -59,12 +59,18 @@
Target toTarget,
@Nullable RuleTransitionFactory trimmingTransitionFactory) {
- // I. Input files and package groups have no configurations. We don't want to duplicate them.
+ // I. The null configuration always remains the null configuration. We could fold this into
+ // (III), but NoTransition doesn't work if the source is the null configuration.
+ if (fromConfig == null) {
+ return NullTransition.INSTANCE;
+ }
+
+ // II. Input files and package groups have no configurations. We don't want to duplicate them.
if (usesNullConfiguration(toTarget)) {
return NullTransition.INSTANCE;
}
- // II. Host configurations never switch to another. All prerequisites of host targets have the
+ // III. Host configurations never switch to another. All prerequisites of host targets have the
// same host configuration.
if (fromConfig.isHostConfiguration()) {
return NoTransition.INSTANCE;
@@ -90,14 +96,14 @@
// The current transition to apply. When multiple transitions are requested, this is a
// ComposingTransition, which encapsulates them into a single object so calling code
// doesn't need special logic for combinations.
- // III. Apply whatever transition the attribute requires.
+ // IV. Apply whatever transition the attribute requires.
ConfigurationTransition currentTransition = attributeTransition;
- // IV. Applies any rule transitions associated with the dep target and composes their
+ // V. Applies any rule transitions associated with the dep target and composes their
// transitions with a passed-in existing transition.
currentTransition = applyRuleTransition(currentTransition, toTarget);
- // V. Applies a transition to trim the result and returns it. (note: this is a temporary
+ // VI. Applies a transition to trim the result and returns it. (note: this is a temporary
// feature; see the corresponding methods in ConfiguredRuleClassProvider)
return applyTransitionFromFactory(currentTransition, toTarget, trimmingTransitionFactory);
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
index ad2091f..508d56b 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
@@ -33,6 +33,7 @@
import com.google.devtools.build.lib.analysis.DependencyResolver.AttributeDependencyKind;
import com.google.devtools.build.lib.analysis.DependencyResolver.DependencyKind;
import com.google.devtools.build.lib.analysis.DependencyResolver.InconsistentAspectOrderException;
+import com.google.devtools.build.lib.analysis.EmptyConfiguredTarget;
import com.google.devtools.build.lib.analysis.PlatformSemantics;
import com.google.devtools.build.lib.analysis.TargetAndConfiguration;
import com.google.devtools.build.lib.analysis.ToolchainContext;
@@ -235,6 +236,21 @@
configuration = null;
}
+ if (target.isConfigurable() && configuredTargetKey.getConfigurationKey() == null) {
+ // We somehow ended up in a target that requires a non-null configuration as a dependency of
+ // one that requires a null configuration. This is always an error, but we need to analyze
+ // the dependencies of the latter target to realize that. Short-circuit the evaluation to
+ // avoid doing useless work and running code with a null configuration that's not prepared for
+ // it.
+ return new NonRuleConfiguredTargetValue(
+ new EmptyConfiguredTarget(target.getLabel(), null),
+ GeneratingActions.EMPTY,
+ transitivePackagesForPackageRootResolution == null
+ ? null
+ : transitivePackagesForPackageRootResolution.build(),
+ nonceVersion.get());
+ }
+
// This line is only needed for accurate error messaging. Say this target has a circular
// dependency with one of its deps. With this line, loading this target fails so Bazel
// associates the corresponding error with this target, as expected. Without this line,
