From cc07b2d4102c6891eadfdbd009f744233b3925ec Mon Sep 17 00:00:00 2001 From: Vladimir Moskva Date: Thu, 23 Mar 2017 16:54:28 +0000 Subject: 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 --- .../build/lib/rules/SkylarkRuleContext.java | 196 +++++++++++++++------ 1 file changed, 147 insertions(+), 49 deletions(-) (limited to 'src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleContext.java') 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 implementation 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 makeVariables; - private final SkylarkRuleAttributesCollection attributesCollection; - private final SkylarkRuleAttributesCollection ruleAttributesCollection; - private final SkylarkClassObject splitAttributes; + private SkylarkDict 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 artifactsLabelMap; - private final SkylarkClassObject outputsObject; + private ImmutableMap 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 attributes, RuleContext ruleContext, Function 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 attrs, ImmutableMap executables, ImmutableMap singleFiles, ImmutableMap files, ImmutableMap 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 { + "

" + "This is intended to help write tests for rule-implementation helper functions, which " + "may take in a ctx 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 { *

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 " + "configuration 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 " + "configuration 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 getFeatures() { + public ImmutableList 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 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 var() { + public SkylarkDict 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 tokenize(String optionString) throws FuncallException { + public SkylarkList tokenize(String optionString) throws FuncallException, EvalException { + checkMutable("tokenize"); List options = new ArrayList<>(); try { ShellUtils.tokenize(options, optionString); @@ -751,6 +838,7 @@ public final class SkylarkRuleContext { public String expand( @Nullable String expression, SkylarkList artifacts, Label labelResolver) throws EvalException, FuncallException { + checkMutable("expand"); try { Map> 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 middleMan(String attribute) { + public NestedSet middleMan(String attribute) throws EvalException { + checkMutable("middle_man"); return AnalysisUtils.getMiddlemanFor(ruleContext, attribute); } @SkylarkCallable(documented = false) public boolean checkPlaceholders(String template, SkylarkList allowedPlaceholders) throws EvalException { + checkMutable("check_placeholders"); List actualPlaceHolders = new LinkedList<>(); Set 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 additionalSubstitutions) { + final Map 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()); -- cgit v1.2.3