diff options
14 files changed, 289 insertions, 201 deletions
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 48fdc0c610..f649728cf8 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 final class ConfiguredAspect implements Iterable<TransitiveInfoProvider> } 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 final class ConfiguredAspect implements Iterable<TransitiveInfoProvider> 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 fd5bca0873..9bab0463af 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 final class RuleConfiguredTargetBuilder { */ 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 final class RuleConfiguredTargetBuilder { 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 final class RuleConfiguredTargetBuilder { */ 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 74291c39e0..95e48a390a 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.Function; 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 809265e5ec..c3f6954312 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 @@ public final class SkylarkRuleConfiguredTargetBuilder { 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 @@ public final class SkylarkRuleConfiguredTargetBuilder { } 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 3e763a2008..3eee895d48 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.Param; 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.Map.Entry; 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 @@ import javax.annotation.Nullable; + "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 @@ public final class SkylarkRuleContext { } }; - 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 @@ public final class SkylarkRuleContext { * 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 @@ public final class SkylarkRuleContext { 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 @@ public final class SkylarkRuleContext { 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 @@ public final class SkylarkRuleContext { 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 @@ public final class SkylarkRuleContext { } private static SkylarkRuleAttributesCollection buildAttributesCollection( + SkylarkRuleContext skylarkRuleContext, Collection<Attribute> attributes, RuleContext ruleContext, Function<Attribute, Object> attributeValueExtractor) { @@ -375,6 +419,7 @@ public final class SkylarkRuleContext { } return new SkylarkRuleAttributesCollection( + skylarkRuleContext, ruleContext.getRule().getRuleClass(), attrBuilder.build(), executableBuilder.build(), @@ -435,6 +480,7 @@ public final class SkylarkRuleContext { 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 @@ public final class SkylarkRuleContext { 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 @@ public final class SkylarkRuleContext { 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 @@ public final class SkylarkRuleContext { 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 @@ public final class SkylarkRuleContext { + "<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 @@ public final class SkylarkRuleContext { } @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 @@ public final class SkylarkRuleContext { * <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 @@ public final class SkylarkRuleContext { * 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 @@ public final class SkylarkRuleContext { * 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 @@ public final class SkylarkRuleContext { 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 @@ public final class SkylarkRuleContext { @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 @@ public final class SkylarkRuleContext { 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 @@ public final class SkylarkRuleContext { 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 @@ public final class SkylarkRuleContext { 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 final class SkylarkRuleContext { 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 @@ public final class SkylarkRuleContext { "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 @@ public final class SkylarkRuleContext { // 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 @@ public final class SkylarkRuleContext { // 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 @@ public final class SkylarkRuleContext { + "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 @@ public final class SkylarkRuleContext { "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 @@ public final class SkylarkRuleContext { "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 @@ public final class SkylarkRuleContext { 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 6792e4e3f7..cd6182a7f9 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 @@ public class SkylarkRuleImplementationFunctions { 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 @@ public class SkylarkRuleImplementationFunctions { Location loc, Environment env) throws EvalException { + ctx.checkMutable("expand_location"); try { return new LocationExpander( ctx.getRuleContext(), @@ -422,6 +424,7 @@ public class SkylarkRuleImplementationFunctions { 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 @@ public class SkylarkRuleImplementationFunctions { @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 @@ public class SkylarkRuleImplementationFunctions { 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 @@ public class SkylarkRuleImplementationFunctions { 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 @@ public class SkylarkRuleImplementationFunctions { 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 434b40574d..85ef296bdd 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.rules.java.proto.StrictDepsUtils; 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 class JavaSkylarkCommon { } ) 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 a2b077e8e0..f4cbfb9b5e 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.rules.proto.SupportData; 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 @@ public class JavaProtoSkylarkCommon { 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 class JavaProtoSkylarkCommon { } ) 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 @@ public class JavaProtoSkylarkCommon { ) // 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 @@ public class JavaProtoSkylarkCommon { } 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 bd0900b8a5..0c01e20c64 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 class SkylarkAspectFactory implements ConfiguredAspectFactory { 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 @@ public class SkylarkAspectFactory implements ConfiguredAspectFactory { 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 21d5c28de3..78879afa21 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 @@ public final class Mutability implements AutoCloseable, Serializable { /** * 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 6903108cce..a88017318a 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 @@ import javax.annotation.Nullable; /** * 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 976c268e97..528fe99064 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 @@ public class SkylarkAspectsTest extends AnalysisTestCase { } @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 14f783e915..79231d663d 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 @@ public class SkylarkIntegrationTest extends BuildViewTestCase { } @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 b5d7108dd7..ef609dbb83 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.SkylarkList; 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 @@ public class SkylarkRuleContextTest extends SkylarkTestCase { (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"); + } + } + } } |