Remove provider safety check and make all old ctx objects featureless
It's now allowed to return anything from a rule implementation function.
If a ctx object is returned it becomes featureless and cannot be used from
anywhere else (i.e. from an implementation function of another rule).
Fixes #2319
--
PiperOrigin-RevId: 151015919
MOS_MIGRATED_REVID=151015919
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAspect.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAspect.java
index 48fdc0c..f649728 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAspect.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAspect.java
@@ -186,14 +186,12 @@
}
public Builder addSkylarkTransitiveInfo(String name, Object value) {
- SkylarkProviderValidationUtil.checkSkylarkObjectSafe(value);
skylarkProviderBuilder.put(name, value);
return this;
}
public Builder addSkylarkTransitiveInfo(String name, Object value, Location loc)
throws EvalException {
- SkylarkProviderValidationUtil.validateAndThrowEvalException(name, value, loc);
skylarkProviderBuilder.put(name, value);
return this;
}
@@ -201,8 +199,6 @@
public Builder addSkylarkDeclaredProvider(SkylarkClassObject declaredProvider, Location loc)
throws EvalException {
ClassObjectConstructor constructor = declaredProvider.getConstructor();
- SkylarkProviderValidationUtil.validateAndThrowEvalException(
- constructor.getPrintableName(), declaredProvider, loc);
if (!constructor.isExported()) {
throw new EvalException(
constructor.getLocation(), "All providers must be top level values");
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
index fd5bca0..9bab046 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
@@ -287,7 +287,6 @@
*/
public RuleConfiguredTargetBuilder addSkylarkTransitiveInfo(
String name, Object value, Location loc) throws EvalException {
- SkylarkProviderValidationUtil.validateAndThrowEvalException(name, value, loc);
skylarkProviders.put(name, value);
return this;
}
@@ -302,8 +301,6 @@
public RuleConfiguredTargetBuilder addSkylarkDeclaredProvider(
SkylarkClassObject provider, Location loc) throws EvalException {
ClassObjectConstructor constructor = provider.getConstructor();
- SkylarkProviderValidationUtil.validateAndThrowEvalException(
- constructor.getPrintableName(), provider, loc);
if (!constructor.isExported()) {
throw new EvalException(constructor.getLocation(),
"All providers must be top level values");
@@ -346,7 +343,6 @@
*/
public RuleConfiguredTargetBuilder addSkylarkTransitiveInfo(
String name, Object value) {
- SkylarkProviderValidationUtil.checkSkylarkObjectSafe(value);
skylarkProviders.put(name, value);
return this;
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/SkylarkProviderValidationUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/SkylarkProviderValidationUtil.java
index 74291c3..95e48a3 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/SkylarkProviderValidationUtil.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/SkylarkProviderValidationUtil.java
@@ -17,100 +17,14 @@
import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
-import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.events.Location;
-import com.google.devtools.build.lib.rules.SkylarkApiProvider;
-import com.google.devtools.build.lib.syntax.ClassObject;
import com.google.devtools.build.lib.syntax.EvalException;
-import com.google.devtools.build.lib.syntax.EvalUtils;
-import com.google.devtools.build.lib.syntax.SkylarkList;
-import com.google.devtools.build.lib.syntax.SkylarkNestedSet;
-import java.util.Map;
/**
* Utility class to validate results of executing Skylark rules and aspects.
*/
public class SkylarkProviderValidationUtil {
- /**
- * Check if the value provided by a Skylark provider is safe (i.e. can be a
- * TransitiveInfoProvider value).
- */
- public static void checkSkylarkObjectSafe(Object value) {
- if (!isSimpleSkylarkObjectSafe(value.getClass())
- // Java transitive Info Providers are accessible from Skylark.
- && !(value instanceof TransitiveInfoProvider)) {
- checkCompositeSkylarkObjectSafe(value);
- }
- }
-
- /**
- * Check if the value provided by a Skylark provider is safe (i.e. can be a
- * TransitiveInfoProvider value).
- * Throws {@link EvalException} if not.
- */
- public static void validateAndThrowEvalException(String providerName, Object value, Location loc)
- throws EvalException {
- try {
- checkSkylarkObjectSafe(value);
- } catch (IllegalArgumentException e) {
- throw new EvalException(
- loc,
- String.format(
- "Value of provider '%s' is of an illegal type: %s", providerName, e.getMessage()));
- }
- }
-
-
- private static void checkCompositeSkylarkObjectSafe(Object object) {
- if (object instanceof SkylarkApiProvider) {
- return;
- } else if (object instanceof SkylarkList) {
- SkylarkList list = (SkylarkList) object;
- if (list.isEmpty()) {
- // Try not to iterate over the list if avoidable.
- return;
- }
- // The list can be a tuple or a list of composite items.
- for (Object listItem : list) {
- checkSkylarkObjectSafe(listItem);
- }
- return;
- } else if (object instanceof SkylarkNestedSet) {
- // SkylarkNestedSets cannot have composite items.
- Class<?> contentType = ((SkylarkNestedSet) object).getContentType().getType();
- if (!contentType.equals(Object.class) && !isSimpleSkylarkObjectSafe(contentType)) {
- throw new IllegalArgumentException(EvalUtils.getDataTypeNameFromClass(contentType));
- }
- return;
- } else if (object instanceof Map<?, ?>) {
- for (Map.Entry<?, ?> entry : ((Map<?, ?>) object).entrySet()) {
- checkSkylarkObjectSafe(entry.getKey());
- checkSkylarkObjectSafe(entry.getValue());
- }
- return;
- } else if (object instanceof ClassObject) {
- ClassObject struct = (ClassObject) object;
- for (String key : struct.getKeys()) {
- checkSkylarkObjectSafe(struct.getValue(key));
- }
- return;
- }
- throw new IllegalArgumentException(EvalUtils.getDataTypeName(object));
- }
-
- private static boolean isSimpleSkylarkObjectSafe(Class<?> type) {
- return type.equals(String.class)
- || type.equals(Integer.class)
- || type.equals(Boolean.class)
- || Artifact.class.isAssignableFrom(type)
- || ActionAnalysisMetadata.class.isAssignableFrom(type)
- || type.equals(Label.class)
- || type.equals(com.google.devtools.build.lib.syntax.Runtime.NoneType.class);
- }
-
public static void checkOrphanArtifacts(RuleContext ruleContext) throws EvalException {
ImmutableSet<Artifact> orphanArtifacts =
ruleContext.getAnalysisEnvironment().getOrphanArtifacts();
diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleConfiguredTargetBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleConfiguredTargetBuilder.java
index 809265e..c3f6954 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleConfiguredTargetBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleConfiguredTargetBuilder.java
@@ -71,9 +71,9 @@
Map<String, Class<? extends TransitiveInfoProvider>> registeredProviderTypes)
throws InterruptedException {
String expectFailure = ruleContext.attributes().get("expect_failure", Type.STRING);
+ SkylarkRuleContext skylarkRuleContext = null;
try (Mutability mutability = Mutability.create("configured target")) {
- SkylarkRuleContext skylarkRuleContext = new SkylarkRuleContext(ruleContext,
- null);
+ skylarkRuleContext = new SkylarkRuleContext(ruleContext, null);
Environment env = Environment.builder(mutability)
.setCallerLabel(ruleContext.getLabel())
.setGlobals(
@@ -114,6 +114,10 @@
}
ruleContext.ruleError("\n" + e.print());
return null;
+ } finally {
+ if (skylarkRuleContext != null) {
+ skylarkRuleContext.nullify();
+ }
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleContext.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleContext.java
index 3e763a2..3eee895 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleContext.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleContext.java
@@ -58,8 +58,10 @@
import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkValue;
import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.syntax.FuncallExpression.FuncallException;
+import com.google.devtools.build.lib.syntax.Printer;
import com.google.devtools.build.lib.syntax.Runtime;
import com.google.devtools.build.lib.syntax.SkylarkDict;
import com.google.devtools.build.lib.syntax.SkylarkList;
@@ -80,7 +82,14 @@
import java.util.Set;
import javax.annotation.Nullable;
-/** A Skylark API for the ruleContext. */
+/** A Skylark API for the ruleContext.
+ *
+ * "This object becomes featureless once the rule implementation function that it was created for
+ * has completed. To achieve this, the {@link #nullify()} should be called once the evaluation of
+ * the function is completed. The method both frees memory by deleting all significant fields of the
+ * object and makes it impossible to accidentally use this object where it's not supposed to be used
+ * (such attempts will result in {@link EvalException}s).
+ */
@SkylarkModule(
name = "ctx",
category = SkylarkModuleCategory.BUILTIN,
@@ -90,7 +99,7 @@
+ "You get a ctx object as an argument to the <code>implementation</code> function when "
+ "you create a rule."
)
-public final class SkylarkRuleContext {
+public final class SkylarkRuleContext implements SkylarkValue {
private static final String DOC_NEW_FILE_TAIL = "Does not actually create a file on the file "
+ "system, just declares that some action will do so. You must create an action that "
@@ -154,21 +163,25 @@
}
};
- private final RuleContext ruleContext;
+ // This field is a copy of the info from ruleContext, stored separately so it can be accessed
+ // after this object has been nullified.
+ private final String ruleLabelCanonicalName;
- private final FragmentCollection fragments;
+ // The fields below intended to be final except that they can be cleared by calling `nullify()`
+ // when the object becomes featureless.
+ private RuleContext ruleContext;
+ private FragmentCollection fragments;
+ private FragmentCollection hostFragments;
+ private AspectDescriptor aspectDescriptor;
- private final FragmentCollection hostFragments;
- private final AspectDescriptor aspectDescriptor;
-
- private final SkylarkDict<String, String> makeVariables;
- private final SkylarkRuleAttributesCollection attributesCollection;
- private final SkylarkRuleAttributesCollection ruleAttributesCollection;
- private final SkylarkClassObject splitAttributes;
+ private SkylarkDict<String, String> makeVariables;
+ private SkylarkRuleAttributesCollection attributesCollection;
+ private SkylarkRuleAttributesCollection ruleAttributesCollection;
+ private SkylarkClassObject splitAttributes;
// TODO(bazel-team): we only need this because of the css_binary rule.
- private final ImmutableMap<Artifact, Label> artifactsLabelMap;
- private final SkylarkClassObject outputsObject;
+ private ImmutableMap<Artifact, Label> artifactsLabelMap;
+ private SkylarkClassObject outputsObject;
/**
* Creates a new SkylarkRuleContext using ruleContext.
@@ -176,9 +189,11 @@
* if it is for a rule.
* @throws InterruptedException
*/
- public SkylarkRuleContext(RuleContext ruleContext, @Nullable AspectDescriptor aspectDescriptor)
+ public SkylarkRuleContext(RuleContext ruleContext,
+ @Nullable AspectDescriptor aspectDescriptor)
throws EvalException, InterruptedException {
this.ruleContext = Preconditions.checkNotNull(ruleContext);
+ this.ruleLabelCanonicalName = ruleContext.getLabel().getCanonicalForm();
this.fragments = new FragmentCollection(ruleContext, ConfigurationTransition.NONE);
this.hostFragments = new FragmentCollection(ruleContext, ConfigurationTransition.HOST);
this.aspectDescriptor = aspectDescriptor;
@@ -241,7 +256,7 @@
this.attributesCollection =
buildAttributesCollection(
- attributes, ruleContext, attributeValueExtractorForRule(ruleContext));
+ this, attributes, ruleContext, attributeValueExtractorForRule(ruleContext));
this.splitAttributes = buildSplitAttributeInfo(attributes, ruleContext);
this.ruleAttributesCollection = null;
} else { // ASPECT
@@ -249,12 +264,14 @@
this.outputsObject = null;
this.attributesCollection =
buildAttributesCollection(
+ this,
ruleContext.getAspectAttributes().values(),
ruleContext,
ATTRIBUTE_VALUE_EXTRACTOR_FOR_ASPECT);
this.splitAttributes = null;
this.ruleAttributesCollection =
buildAttributesCollection(
+ this,
ruleContext.getRule().getAttributes(),
ruleContext,
attributeValueExtractorForRule(ruleContext));
@@ -263,6 +280,32 @@
makeVariables = ruleContext.getConfigurationMakeVariableContext().collectMakeVariables();
}
+ /**
+ * Nullifies fields of the object when it's not supposed to be used anymore to free unused memory
+ * and to make sure this object is not accessed when it's not supposed to (after the corresponding
+ * rule implementation function has exited).
+ */
+ public void nullify() {
+ ruleContext = null;
+ fragments = null;
+ hostFragments = null;
+ aspectDescriptor = null;
+ makeVariables = null;
+ attributesCollection = null;
+ ruleAttributesCollection = null;
+ splitAttributes = null;
+ artifactsLabelMap = null;
+ outputsObject = null;
+ }
+
+ public void checkMutable(String attrName) throws EvalException {
+ if (isImmutable()) {
+ throw new EvalException(null, String.format(
+ "cannot access field or method '%s' of rule context for '%s' outside of its own rule "
+ + "implementation function", attrName, ruleLabelCanonicalName));
+ }
+ }
+
@Nullable
public AspectDescriptor getAspectDescriptor() {
return aspectDescriptor;
@@ -280,6 +323,7 @@
}
private static SkylarkRuleAttributesCollection buildAttributesCollection(
+ SkylarkRuleContext skylarkRuleContext,
Collection<Attribute> attributes,
RuleContext ruleContext,
Function<Attribute, Object> attributeValueExtractor) {
@@ -375,6 +419,7 @@
}
return new SkylarkRuleAttributesCollection(
+ skylarkRuleContext,
ruleContext.getRule().getRuleClass(),
attrBuilder.build(),
executableBuilder.build(),
@@ -435,6 +480,7 @@
doc = "Information about attributes of a rule an aspect is applied to."
)
private static class SkylarkRuleAttributesCollection {
+ private final SkylarkRuleContext skylarkRuleContext;
private final SkylarkClassObject attrObject;
private final SkylarkClassObject executableObject;
private final SkylarkClassObject fileObject;
@@ -443,11 +489,13 @@
private final String ruleClassName;
private SkylarkRuleAttributesCollection(
+ SkylarkRuleContext skylarkRuleContext,
String ruleClassName, ImmutableMap<String, Object> attrs,
ImmutableMap<String, Object> executables,
ImmutableMap<String, Object> singleFiles,
ImmutableMap<String, Object> files,
ImmutableMap<Artifact, FilesToRunProvider> executableRunfilesMap) {
+ this.skylarkRuleContext = skylarkRuleContext;
this.ruleClassName = ruleClassName;
attrObject =
NativeClassObjectConstructor.STRUCT.create(
@@ -471,29 +519,38 @@
this.executableRunfilesMap = executableRunfilesMap;
}
+ private void checkMutable(String attrName) throws EvalException {
+ skylarkRuleContext.checkMutable("rule." + attrName);
+ }
+
@SkylarkCallable(name = "attr", structField = true, doc = ATTR_DOC)
- public SkylarkClassObject getAttr() {
+ public SkylarkClassObject getAttr() throws EvalException {
+ checkMutable("attr");
return attrObject;
}
@SkylarkCallable(name = "executable", structField = true, doc = EXECUTABLE_DOC)
- public SkylarkClassObject getExecutable() {
+ public SkylarkClassObject getExecutable() throws EvalException {
+ checkMutable("executable");
return executableObject;
}
@SkylarkCallable(name = "file", structField = true, doc = FILE_DOC)
- public SkylarkClassObject getFile() {
+ public SkylarkClassObject getFile() throws EvalException {
+ checkMutable("file");
return fileObject;
}
@SkylarkCallable(name = "files", structField = true, doc = FILES_DOC)
- public SkylarkClassObject getFiles() {
+ public SkylarkClassObject getFiles() throws EvalException {
+ checkMutable("files");
return filesObject;
}
@SkylarkCallable(name = "kind", structField = true, doc =
"The kind of a rule, such as 'cc_library'")
- public String getRuleClassName() {
+ public String getRuleClassName() throws EvalException {
+ checkMutable("kind");
return ruleClassName;
}
@@ -510,6 +567,16 @@
outputsBuilder.put(key, value);
}
+ @Override
+ public boolean isImmutable() {
+ return ruleContext == null;
+ }
+
+ @Override
+ public void write(Appendable buffer, char quotationMark) {
+ Printer.append(buffer, ruleLabelCanonicalName);
+ }
+
/**
* Returns the original ruleContext.
*/
@@ -548,7 +615,8 @@
+ "<br/><br/>"
+ "This is intended to help write tests for rule-implementation helper functions, which "
+ "may take in a <code>ctx</code> object and create actions on it.")
- public Object createdActions() {
+ public Object createdActions() throws EvalException {
+ checkMutable("created_actions");
if (ruleContext.getRule().getRuleClassObject().isSkylarkTestable()) {
return ActionsProvider.create(
ruleContext.getAnalysisEnvironment().getRegisteredActions());
@@ -558,12 +626,14 @@
}
@SkylarkCallable(name = "attr", structField = true, doc = ATTR_DOC)
- public SkylarkClassObject getAttr() {
+ public SkylarkClassObject getAttr() throws EvalException {
+ checkMutable("attr");
return attributesCollection.getAttr();
}
@SkylarkCallable(name = "split_attr", structField = true, doc = SPLIT_ATTR_DOC)
public SkylarkClassObject getSplitAttr() throws EvalException {
+ checkMutable("split_attr");
if (splitAttributes == null) {
throw new EvalException(
Location.BUILTIN, "'split_attr' is available only in rule implementations");
@@ -575,7 +645,8 @@
* <p>See {@link RuleContext#getExecutablePrerequisite(String, Mode)}.
*/
@SkylarkCallable(name = "executable", structField = true, doc = EXECUTABLE_DOC)
- public SkylarkClassObject getExecutable() {
+ public SkylarkClassObject getExecutable() throws EvalException {
+ checkMutable("executable");
return attributesCollection.getExecutable();
}
@@ -583,7 +654,8 @@
* See {@link RuleContext#getPrerequisiteArtifact(String, Mode)}.
*/
@SkylarkCallable(name = "file", structField = true, doc = FILE_DOC)
- public SkylarkClassObject getFile() {
+ public SkylarkClassObject getFile() throws EvalException {
+ checkMutable("file");
return attributesCollection.getFile();
}
@@ -591,44 +663,51 @@
* See {@link RuleContext#getPrerequisiteArtifacts(String, Mode)}.
*/
@SkylarkCallable(name = "files", structField = true, doc = FILES_DOC)
- public SkylarkClassObject getFiles() {
+ public SkylarkClassObject getFiles() throws EvalException {
+ checkMutable("files");
return attributesCollection.getFiles();
}
@SkylarkCallable(name = "workspace_name", structField = true,
doc = "Returns the workspace name as defined in the WORKSPACE file.")
- public String getWorkspaceName() {
+ public String getWorkspaceName() throws EvalException {
+ checkMutable("workspace_name");
return ruleContext.getWorkspaceName();
}
@SkylarkCallable(name = "label", structField = true, doc = "The label of this rule.")
- public Label getLabel() {
+ public Label getLabel() throws EvalException {
+ checkMutable("label");
return ruleContext.getLabel();
}
@SkylarkCallable(name = "fragments", structField = true,
doc = "Allows access to configuration fragments in target configuration.")
- public FragmentCollection getFragments() {
+ public FragmentCollection getFragments() throws EvalException {
+ checkMutable("fragments");
return fragments;
}
@SkylarkCallable(name = "host_fragments", structField = true,
doc = "Allows access to configuration fragments in host configuration.")
- public FragmentCollection getHostFragments() {
+ public FragmentCollection getHostFragments() throws EvalException {
+ checkMutable("host_fragments");
return hostFragments;
}
@SkylarkCallable(name = "configuration", structField = true,
doc = "Returns the default configuration. See the <a href=\"configuration.html\">"
+ "configuration</a> type for more details.")
- public BuildConfiguration getConfiguration() {
+ public BuildConfiguration getConfiguration() throws EvalException {
+ checkMutable("configuration");
return ruleContext.getConfiguration();
}
@SkylarkCallable(name = "host_configuration", structField = true,
doc = "Returns the host configuration. See the <a href=\"configuration.html\">"
+ "configuration</a> type for more details.")
- public BuildConfiguration getHostConfiguration() {
+ public BuildConfiguration getHostConfiguration() throws EvalException {
+ checkMutable("host_configuration");
return ruleContext.getHostConfiguration();
}
@@ -648,7 +727,8 @@
named = true,
doc = "A Target specifying a rule. If not provided, defaults to the current rule.")
})
- public boolean instrumentCoverage(Object targetUnchecked) {
+ public boolean instrumentCoverage(Object targetUnchecked) throws EvalException {
+ checkMutable("coverage_instrumented");
BuildConfiguration config = ruleContext.getConfiguration();
if (!config.isCodeCoverageEnabled()) {
return false;
@@ -664,24 +744,28 @@
@SkylarkCallable(name = "features", structField = true,
doc = "Returns the set of features that are enabled for this rule."
)
- public ImmutableList<String> getFeatures() {
+ public ImmutableList<String> getFeatures() throws EvalException {
+ checkMutable("features");
return ImmutableList.copyOf(ruleContext.getFeatures());
}
@SkylarkCallable(name = "bin_dir", structField = true,
doc = "The root corresponding to bin directory.")
- public Root getBinDirectory() {
+ public Root getBinDirectory() throws EvalException {
+ checkMutable("bin_dir");
return getConfiguration().getBinDirectory(ruleContext.getRule().getRepository());
}
@SkylarkCallable(name = "genfiles_dir", structField = true,
doc = "The root corresponding to genfiles directory.")
- public Root getGenfilesDirectory() {
+ public Root getGenfilesDirectory() throws EvalException {
+ checkMutable("genfiles_dir");
return getConfiguration().getGenfilesDirectory(ruleContext.getRule().getRepository());
}
@SkylarkCallable(structField = true, doc = OUTPUTS_DOC)
public SkylarkClassObject outputs() throws EvalException {
+ checkMutable("outputs");
if (outputsObject == null) {
throw new EvalException(Location.BUILTIN, "'outputs' is not defined");
}
@@ -692,6 +776,7 @@
doc = "Returns rule attributes descriptor for the rule that aspect is applied to."
+ " Only available in aspect implementation functions.")
public SkylarkRuleAttributesCollection rule() throws EvalException {
+ checkMutable("rule");
if (ruleAttributesCollection == null) {
throw new EvalException(
Location.BUILTIN, "'rule' is only available in aspect implementations");
@@ -704,6 +789,7 @@
doc = "Returns a list ids for all aspects applied to the target."
+ " Only available in aspect implementation functions.")
public ImmutableList<String> aspectIds() throws EvalException {
+ checkMutable("aspect_ids");
if (ruleAttributesCollection == null) {
throw new EvalException(
Location.BUILTIN, "'aspect_ids' is only available in aspect implementations");
@@ -716,22 +802,23 @@
return result.build();
}
-
@SkylarkCallable(
structField = true,
doc = "Dictionary (String to String) of configuration variables."
)
- public SkylarkDict<String, String> var() {
+ public SkylarkDict<String, String> var() throws EvalException {
+ checkMutable("var");
return makeVariables;
}
@Override
public String toString() {
- return ruleContext.getLabel().toString();
+ return ruleLabelCanonicalName;
}
@SkylarkCallable(doc = "Splits a shell command to a list of tokens.", documented = false)
- public SkylarkList<String> tokenize(String optionString) throws FuncallException {
+ public SkylarkList<String> tokenize(String optionString) throws FuncallException, EvalException {
+ checkMutable("tokenize");
List<String> options = new ArrayList<>();
try {
ShellUtils.tokenize(options, optionString);
@@ -751,6 +838,7 @@
public String expand(
@Nullable String expression, SkylarkList<Object> artifacts, Label labelResolver)
throws EvalException, FuncallException {
+ checkMutable("expand");
try {
Map<Label, Iterable<Artifact>> labelMap = new HashMap<>();
for (Artifact artifact : artifacts.getContents(Artifact.class, "artifacts")) {
@@ -771,11 +859,12 @@
"Creates a file object with the given filename, in the current package. "
+ DOC_NEW_FILE_TAIL
)
- public Artifact newFile(String filename) {
+ public Artifact newFile(String filename) throws EvalException {
+ checkMutable("new_file");
return newFile(newFileRoot(), filename);
}
- private Root newFileRoot() {
+ private Root newFileRoot() throws EvalException {
return isForAspect()
? getConfiguration().getBinDirectory(ruleContext.getRule().getRepository())
: ruleContext.getBinOrGenfilesDirectory();
@@ -783,14 +872,16 @@
// Kept for compatibility with old code.
@SkylarkCallable(documented = false)
- public Artifact newFile(Root root, String filename) {
+ public Artifact newFile(Root root, String filename) throws EvalException {
+ checkMutable("new_file");
return ruleContext.getPackageRelativeArtifact(filename, root);
}
@SkylarkCallable(doc =
"Creates a new file object in the same directory as the original file. "
+ DOC_NEW_FILE_TAIL)
- public Artifact newFile(Artifact baseArtifact, String newBaseName) {
+ public Artifact newFile(Artifact baseArtifact, String newBaseName) throws EvalException {
+ checkMutable("new_file");
PathFragment original = baseArtifact.getRootRelativePath();
PathFragment fragment = original.replaceName(newBaseName);
return ruleContext.getDerivedArtifact(fragment, newFileRoot());
@@ -798,20 +889,23 @@
// Kept for compatibility with old code.
@SkylarkCallable(documented = false)
- public Artifact newFile(Root root, Artifact baseArtifact, String suffix) {
+ public Artifact newFile(Root root, Artifact baseArtifact, String suffix) throws EvalException {
+ checkMutable("new_file");
PathFragment original = baseArtifact.getRootRelativePath();
PathFragment fragment = original.replaceName(original.getBaseName() + suffix);
return ruleContext.getDerivedArtifact(fragment, root);
}
@SkylarkCallable(documented = false)
- public NestedSet<Artifact> middleMan(String attribute) {
+ public NestedSet<Artifact> middleMan(String attribute) throws EvalException {
+ checkMutable("middle_man");
return AnalysisUtils.getMiddlemanFor(ruleContext, attribute);
}
@SkylarkCallable(documented = false)
public boolean checkPlaceholders(String template, SkylarkList<Object> allowedPlaceholders)
throws EvalException {
+ checkMutable("check_placeholders");
List<String> actualPlaceHolders = new LinkedList<>();
Set<String> allowedPlaceholderSet =
ImmutableSet.copyOf(allowedPlaceholders.getContents(String.class, "allowed_placeholders"));
@@ -842,7 +936,8 @@
+ "Additional variables may come from other places, such as configurations. Note that "
+ "this function is experimental.")
public String expandMakeVariables(String attributeName, String command,
- final Map<String, String> additionalSubstitutions) {
+ final Map<String, String> additionalSubstitutions) throws EvalException {
+ checkMutable("expand_make_variables");
return ruleContext.expandMakeVariables(attributeName,
command, new ConfigurationMakeVariableContext(ruleContext.getRule().getPackage(),
ruleContext.getConfiguration()) {
@@ -870,7 +965,8 @@
"Returns the file that is used to hold the non-volatile workspace status for the "
+ "current build request."
)
- public Artifact getStableWorkspaceStatus() throws InterruptedException {
+ public Artifact getStableWorkspaceStatus() throws InterruptedException, EvalException {
+ checkMutable("info_file");
return ruleContext.getAnalysisEnvironment().getStableWorkspaceStatusArtifact();
}
@@ -882,7 +978,8 @@
"Returns the file that is used to hold the volatile workspace status for the "
+ "current build request."
)
- public Artifact getVolatileWorkspaceStatus() throws InterruptedException {
+ public Artifact getVolatileWorkspaceStatus() throws InterruptedException, EvalException {
+ checkMutable("version_file");
return ruleContext.getAnalysisEnvironment().getVolatileWorkspaceStatusArtifact();
}
@@ -892,7 +989,8 @@
documented = true,
doc = "Returns path to the BUILD file for this rule, relative to the source root."
)
- public String getBuildFileRelativePath() {
+ public String getBuildFileRelativePath() throws EvalException {
+ checkMutable("build_file_path");
Package pkg = ruleContext.getRule().getPackage();
Root root = Root.asSourceRoot(pkg.getSourceRoot(),
pkg.getPackageIdentifier().getRepository().isMain());
diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleImplementationFunctions.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleImplementationFunctions.java
index 6792e4e..cd6182a 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleImplementationFunctions.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleImplementationFunctions.java
@@ -229,6 +229,7 @@
Object inputManifestsUnchecked,
Location loc)
throws EvalException, ConversionException {
+ ctx.checkMutable("action");
SpawnAction.Builder builder = new SpawnAction.Builder();
// TODO(bazel-team): builder still makes unnecessary copies of inputs, outputs and args.
boolean hasCommand = commandUnchecked != Runtime.NONE;
@@ -369,6 +370,7 @@
Location loc,
Environment env)
throws EvalException {
+ ctx.checkMutable("expand_location");
try {
return new LocationExpander(
ctx.getRuleContext(),
@@ -422,6 +424,7 @@
public Runtime.NoneType invoke(
SkylarkRuleContext ctx, Artifact output, String content, Boolean executable)
throws EvalException, ConversionException {
+ ctx.checkMutable("file_action");
FileWriteAction action =
FileWriteAction.create(ctx.getRuleContext(), output, content, executable);
ctx.getRuleContext().registerAction(action);
@@ -461,6 +464,7 @@
@SuppressWarnings("unused")
public Runtime.NoneType invoke(SkylarkRuleContext ctx, String mnemonic, SkylarkList inputs)
throws EvalException, ConversionException {
+ ctx.checkMutable("empty_action");
RuleContext ruleContext = ctx.getRuleContext();
Action action =
new PseudoAction<>(
@@ -544,6 +548,7 @@
SkylarkDict<?, ?> substitutionsUnchecked,
Boolean executable)
throws EvalException, ConversionException {
+ ctx.checkMutable("template_action");
ImmutableList.Builder<Substitution> substitutionsBuilder = ImmutableList.builder();
for (Map.Entry<String, String> substitution :
substitutionsUnchecked
@@ -609,6 +614,7 @@
Boolean collectData, Boolean collectDefault,
SkylarkDict<?, ?> symlinks, SkylarkDict<?, ?> rootSymlinks,
Location loc) throws EvalException, ConversionException {
+ ctx.checkMutable("runfiles");
Runfiles.Builder builder = new Runfiles.Builder(
ctx.getRuleContext().getWorkspaceName(),
ctx.getConfiguration().legacyExternalRunfiles());
@@ -790,6 +796,7 @@
Location loc,
Environment env)
throws ConversionException, EvalException {
+ ctx.checkMutable("resolve_command");
Label ruleLabel = ctx.getLabel();
Map<Label, Iterable<Artifact>> labelDict = checkLabelDict(labelDictUnchecked, loc);
// The best way to fix this probably is to convert CommandHelper to Skylark.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaSkylarkCommon.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaSkylarkCommon.java
index 434b405..85ef296 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaSkylarkCommon.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaSkylarkCommon.java
@@ -31,6 +31,7 @@
import com.google.devtools.build.lib.skylarkinterface.Param;
import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
+import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.syntax.SkylarkList;
import java.util.List;
@@ -243,7 +244,7 @@
}
)
public static List<String> getDefaultJavacOpts(
- SkylarkRuleContext skylarkRuleContext, String javaToolchainAttr) {
+ SkylarkRuleContext skylarkRuleContext, String javaToolchainAttr) throws EvalException {
RuleContext ruleContext = skylarkRuleContext.getRuleContext();
ConfiguredTarget javaToolchainConfigTarget =
(ConfiguredTarget) checkNotNull(skylarkRuleContext.getAttr().getValue(javaToolchainAttr));
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaProtoSkylarkCommon.java b/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaProtoSkylarkCommon.java
index a2b077e..f4cbfb9 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaProtoSkylarkCommon.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaProtoSkylarkCommon.java
@@ -32,6 +32,7 @@
import com.google.devtools.build.lib.skylarkinterface.Param;
import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
+import com.google.devtools.build.lib.syntax.EvalException;
import java.util.List;
/**
@@ -72,7 +73,7 @@
ConfiguredTarget target,
Artifact sourceJar,
String protoToolchainAttr,
- String flavour) {
+ String flavour) throws EvalException {
SupportData supportData =
checkNotNull(target.getProvider(ProtoSupportDataProvider.class).getSupportData());
ProtoCompileActionBuilder.registerActions(
@@ -114,7 +115,7 @@
}
)
public static JavaProvider getRuntimeToolchainProvider(
- SkylarkRuleContext skylarkRuleContext, String protoToolchainAttr) {
+ SkylarkRuleContext skylarkRuleContext, String protoToolchainAttr) throws EvalException {
TransitiveInfoCollection runtime =
getProtoToolchainProvider(skylarkRuleContext, protoToolchainAttr).runtime();
return
@@ -137,7 +138,7 @@
)
// TODO(elenairina): Consider a nicer way of returning this, taking in a JavaToolchainProvider.
public static List<String> getJavacOpts(
- SkylarkRuleContext skylarkRuleContext, String javaToolchainAttr) {
+ SkylarkRuleContext skylarkRuleContext, String javaToolchainAttr) throws EvalException {
ConfiguredTarget javaToolchainConfigTarget =
(ConfiguredTarget) checkNotNull(skylarkRuleContext.getAttr().getValue(javaToolchainAttr));
JavaToolchainProvider toolchain =
@@ -150,7 +151,7 @@
}
private static ProtoLangToolchainProvider getProtoToolchainProvider(
- SkylarkRuleContext skylarkRuleContext, String protoToolchainAttr) {
+ SkylarkRuleContext skylarkRuleContext, String protoToolchainAttr) throws EvalException {
ConfiguredTarget javaliteToolchain = (ConfiguredTarget) checkNotNull(
skylarkRuleContext.getAttr().getValue(protoToolchainAttr));
return checkNotNull(javaliteToolchain.getProvider(ProtoLangToolchainProvider.class));
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java
index bd0900b..0c01e20 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java
@@ -51,10 +51,10 @@
public ConfiguredAspect create(
ConfiguredTarget base, RuleContext ruleContext, AspectParameters parameters)
throws InterruptedException {
+ SkylarkRuleContext skylarkRuleContext = null;
try (Mutability mutability = Mutability.create("aspect")) {
AspectDescriptor aspectDescriptor = new AspectDescriptor(
skylarkAspect.getAspectClass(), parameters);
- SkylarkRuleContext skylarkRuleContext;
try {
skylarkRuleContext = new SkylarkRuleContext(ruleContext, aspectDescriptor);
} catch (EvalException e) {
@@ -94,6 +94,10 @@
ruleContext.ruleError("\n" + e.print());
return null;
}
+ } finally {
+ if (skylarkRuleContext != null) {
+ skylarkRuleContext.nullify();
+ }
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Mutability.java b/src/main/java/com/google/devtools/build/lib/syntax/Mutability.java
index 21d5c28..78879af 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Mutability.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Mutability.java
@@ -118,7 +118,7 @@
/**
* For a locked {@link Freezable} that belongs to this Mutability, return a List of the
- * {@link Locations} corresponding to its current locks.
+ * {@link Location}s corresponding to its current locks.
*/
public List<Location> getLockLocations(Freezable object) {
if (!isLocked(object)) {
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkMutable.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkMutable.java
index 6903108..a880173 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkMutable.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkMutable.java
@@ -29,7 +29,7 @@
/**
* Base class for data structures that are only mutable with a proper Mutability.
*/
-abstract class SkylarkMutable implements Freezable, SkylarkValue {
+public abstract class SkylarkMutable implements Freezable, SkylarkValue {
protected SkylarkMutable() {}
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
index 976c268..528fe99 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
@@ -701,34 +701,6 @@
}
@Test
- public void aspectFailingReturnsUnsafeObject() throws Exception {
- scratch.file(
- "test/aspect.bzl",
- "def foo():",
- " return 0",
- "def _impl(target, ctx):",
- " return struct(x = foo)",
- "",
- "MyAspect = aspect(implementation=_impl)");
- scratch.file("test/BUILD", "java_library(name = 'xxx',)");
-
- reporter.removeHandler(failFastHandler);
- try {
- AnalysisResult result = update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx");
- assertThat(keepGoing()).isTrue();
- assertThat(result.hasError()).isTrue();
- } catch (ViewCreationFailedException e) {
- // expect to fail.
- }
- assertContainsEvent(
- "ERROR /workspace/test/BUILD:1:1: in //test:aspect.bzl%MyAspect aspect on java_library rule"
- + " //test:xxx: \n"
- + "\n"
- + "\n"
- + "/workspace/test/aspect.bzl:4:11: Value of provider 'x' is of an illegal type: function");
- }
-
- @Test
public void aspectFailingOrphanArtifacts() throws Exception {
scratch.file(
"test/aspect.bzl",
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
index 14f783e..79231d6 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
@@ -972,26 +972,6 @@
}
@Test
- public void testProviderValidation() throws Exception {
- // This does not have full coverage for SkylarkProviderValidationUtil, which eventually
- // should be factored into a more general way for validating that an object is deeply
- // Skylark-permissible. It's just a regression test for specific issues.
- reporter.removeHandler(failFastHandler);
- scratch.file(
- "test/skylark/extension.bzl",
- "def _impl(ctx):",
- " return struct(bad=depset([depset([])]))",
- "my_rule = rule(implementation = _impl)");
- scratch.file(
- "test/skylark/BUILD",
- "load('/test/skylark/extension', 'my_rule')",
- "my_rule(name = 'r')");
-
- getConfiguredTarget("//test/skylark:r");
- assertContainsEvent("Value of provider 'bad' is of an illegal type: depset\n");
- }
-
- @Test
public void testRecursionDetection() throws Exception {
reporter.removeHandler(failFastHandler);
scratch.file(
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
index b5d7108..ef609db 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
@@ -42,6 +42,7 @@
import com.google.devtools.build.lib.syntax.SkylarkNestedSet;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.PathFragment;
+import java.util.ArrayList;
import java.util.List;
import org.junit.Before;
import org.junit.Test;
@@ -1733,4 +1734,118 @@
(Label) evalRuleContextCode(ruleContext, "ruleContext.attr.runtimes.values()[0]");
assertThat(valueLabel).isEqualTo(Label.parseAbsolute("//jvm:runtime"));
}
+
+ // A list of attributes and methods ctx objects have
+ private final List<String> ctxAttributes = ImmutableList.of(
+ "attr",
+ "split_attr",
+ "executable",
+ "file",
+ "files",
+ "workspace_name",
+ "label",
+ "fragments",
+ "host_fragments",
+ "configuration",
+ "host_configuration",
+ "coverage_instrumented(dep)",
+ "features",
+ "bin_dir",
+ "genfiles_dir",
+ "outputs",
+ "rule",
+ "aspect_ids",
+ "var",
+ "tokenize('foo')",
+ "expand('foo', [], Label('//test:main'))",
+ "new_file('foo.txt')",
+ "new_file(file, 'foo.txt')",
+ "check_placeholders('foo', [])",
+ "action(command = 'foo', outputs = [file])",
+ "file_action(file, 'foo')",
+ "empty_action(mnemonic = 'foo', inputs = [file])",
+ "template_action(template = file, output = file, substitutions = {})",
+ "runfiles()",
+ "resolve_command(command = 'foo')");
+
+ @Test
+ public void testFrozenRuleContextHasInaccessibleAttributes() throws Exception {
+ scratch.file("test/BUILD",
+ "load('/test/rules', 'main_rule', 'dep_rule')",
+ "dep_rule(name = 'dep')",
+ "main_rule(name = 'main', deps = [':dep'])");
+ scratch.file("test/rules.bzl");
+
+ for (String attribute : ctxAttributes) {
+ scratch.overwriteFile("test/rules.bzl",
+ "def _main_impl(ctx):",
+ " dep = ctx.attr.deps[0]",
+ " file = ctx.outputs.file",
+ " foo = dep.dep_ctx." + attribute,
+ "main_rule = rule(",
+ " implementation = _main_impl,",
+ " attrs = {",
+ " 'deps': attr.label_list()",
+ " },",
+ " outputs = {'file': 'output.txt'},",
+ ")",
+ "def _dep_impl(ctx):",
+ " return struct(dep_ctx = ctx)",
+ "dep_rule = rule(implementation = _dep_impl)");
+ invalidatePackages();
+ try {
+ getConfiguredTarget("//test:main");
+ fail("Should have been unable to access dep_ctx." + attribute);
+ } catch (AssertionError e) {
+ assertThat(e.getMessage()).contains("cannot access field or method '"
+ + attribute.split("\\(")[0]
+ + "' of rule context for '//test:dep' outside of its own rule implementation function");
+ }
+ }
+ }
+
+ @Test
+ public void testFrozenRuleContextForAspectsHasInaccessibleAttributes() throws Exception {
+ List<String> attributes = new ArrayList<>();
+ attributes.addAll(ctxAttributes);
+ attributes.addAll(ImmutableList.of(
+ "rule.attr",
+ "rule.executable",
+ "rule.file",
+ "rule.files",
+ "rule.kind"));
+ scratch.file("test/BUILD",
+ "load('/test/rules', 'my_rule')",
+ "my_rule(name = 'dep')",
+ "my_rule(name = 'mid', deps = [':dep'])",
+ "my_rule(name = 'main', deps = [':mid'])");
+ scratch.file("test/rules.bzl");
+ for (String attribute : attributes) {
+ scratch.overwriteFile("test/rules.bzl",
+ "def _rule_impl(ctx):",
+ " pass",
+ "def _aspect_impl(target, ctx):",
+ " if ctx.rule.attr.deps:",
+ " dep = ctx.rule.attr.deps[0]",
+ " file = ctx.new_file('file.txt')",
+ " foo = dep." + (attribute.startsWith("rule.") ? "" : "ctx.") + attribute,
+ " return struct(ctx = ctx, rule=ctx.rule)",
+ "MyAspect = aspect(implementation=_aspect_impl)",
+ "my_rule = rule(",
+ " implementation = _rule_impl,",
+ " attrs = {",
+ " 'deps': attr.label_list(aspects = [MyAspect])",
+ " },",
+ ")");
+ invalidatePackages();
+ try {
+ getConfiguredTarget("//test:main");
+ fail("Should have been unable to access dep." + attribute);
+ } catch (AssertionError e) {
+ assertThat(e.getMessage()).contains("cannot access field or method '"
+ + attribute.split("\\(")[0]
+ + "' of rule context for '//test:dep' outside of its own rule implementation function");
+ }
+ }
+ }
}
