Skip to content

Commit

Permalink
Disallow field syntax to access a target's providers
Browse files Browse the repository at this point in the history
    The provider-key syntax should be used instead.

    This is an incompatible change attached to new flag --incompatible_disable_target_provider_fields.

    See bazelbuild/bazel#9014 for details.

    RELNOTES: New incompatible flag --incompatible_disable_target_provider_fields removes the ability (in Starlark) to access a target's providers via the field syntax (for example, `ctx.attr.dep.my_provider`). The provider-key syntax should be used instead (for example, `ctx.attr.dep[MyProvider]`). See bazelbuild/bazel#9014 for details.
    PiperOrigin-RevId: 260920114
  • Loading branch information
Luca Di Grazia committed Sep 4, 2022
1 parent cc240b8 commit 8836311
Show file tree
Hide file tree
Showing 8 changed files with 42 additions and 92 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import com.google.devtools.build.lib.packages.Provider;
import com.google.devtools.build.lib.skyframe.BuildConfigurationValue;
import com.google.devtools.build.lib.skylarkinterface.SkylarkPrinter;
import com.google.devtools.build.lib.skylarkinterface.StarlarkContext;
import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.syntax.EvalUtils;
import com.google.devtools.build.lib.syntax.Mutability;
Expand Down Expand Up @@ -154,7 +155,8 @@ public Object getValue(String name) {
}

@Override
public final Object getIndex(Object key, Location loc) throws EvalException {
public final Object getIndex(Object key, Location loc, StarlarkContext context)
throws EvalException {
if (!(key instanceof Provider)) {
throw new EvalException(loc, String.format(
"Type Target only supports indexing by object constructors, got %s instead",
Expand All @@ -175,7 +177,8 @@ public final Object getIndex(Object key, Location loc) throws EvalException {
}

@Override
public boolean containsKey(Object key, Location loc) throws EvalException {
public boolean containsKey(Object key, Location loc, StarlarkContext context)
throws EvalException {
if (!(key instanceof Provider)) {
throw new EvalException(loc, String.format(
"Type Target only supports querying by object constructors, got %s instead",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,11 @@
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import com.google.devtools.build.lib.syntax.Concatable;
import com.google.devtools.build.lib.syntax.Environment;
import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.syntax.EvalUtils;
import com.google.devtools.build.lib.syntax.SkylarkClassObject;
import com.google.devtools.build.lib.syntax.Starlark;
import com.google.devtools.build.lib.syntax.SkylarkType;
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
import java.util.Arrays;
import java.util.List;
Expand Down Expand Up @@ -336,10 +337,7 @@ private static final class CompactSkylarkInfo extends SkylarkInfo implements Con
this.values = new Object[values.length];
for (int i = 0; i < values.length; i++) {
// TODO(b/74396075): Phase out this unnecessary conversion.
// NB: fromJava treats null as None, but we need nulls to indicate a field is not present.
if (values[i] != null) {
this.values[i] = Starlark.fromJava(values[i], null);
}
this.values[i] = SkylarkType.convertToSkylark(values[i], (Environment) null);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -348,18 +348,6 @@ public class StarlarkSemanticsOptions extends OptionsBase implements Serializabl
help = "If set to true, the default value of the `allow_empty` argument of glob() is False.")
public boolean incompatibleDisallowEmptyGlob;

@Option(
name = "incompatible_disallow_hashing_frozen_mutables",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS},
metadataTags = {
OptionMetadataTag.INCOMPATIBLE_CHANGE,
OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
},
help = "If set to true, freezing a mutable object will not make it hashable.")
public boolean incompatibleDisallowHashingFrozenMutables;

@Option(
name = "incompatible_disallow_legacy_java_provider",
defaultValue = "false",
Expand Down Expand Up @@ -704,15 +692,6 @@ public class StarlarkSemanticsOptions extends OptionsBase implements Serializabl
help = "If set to true, unknown string escapes like `\\a` become rejected.")
public boolean incompatibleRestrictStringEscapes;

@Option(
name = "experimental_function_equality_by_location",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
effectTags = OptionEffectTag.BUILD_FILE_SEMANTICS,
help =
"If set to true, two Starlark functions defined at the same place are considered equal.")
public boolean experimentalFunctionEqualityByLocation;

/**
* An interner to reduce the number of StarlarkSemantics instances. A single Blaze instance should
* never accumulate a large number of these and being able to shortcut on object identity makes a
Expand Down Expand Up @@ -782,8 +761,6 @@ public StarlarkSemantics toSkylarkSemantics() {
.incompatibleAllowTagsPropagation(incompatibleAllowTagsPropagation)
.incompatibleAssignmentIdentifiersHaveLocalScope(
incompatibleAssignmentIdentifiersHaveLocalScope)
.incompatibleDisallowHashingFrozenMutables(incompatibleDisallowHashingFrozenMutables)
.experimentalFunctionEqualityByLocation(experimentalFunctionEqualityByLocation)
.build();
return INTERNER.intern(semantics);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public final class DotExpression extends Expression {

private final Identifier field;

DotExpression(Expression object, Identifier field) {
public DotExpression(Expression object, Identifier field) {
this.object = object;
this.field = field;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,30 @@
import com.google.devtools.build.lib.events.Location;
import javax.annotation.Nullable;

/** A variant of ClassObject for implementations that require a StarlarkSemantics. */
/**
* A marker interface for a {@link ClassObject} whose {@link #getValue} always returns a
* Skylark-friendly value, with no defensive conversion required.
*
* <p>An value is Skylark-friendly if its class (or a supertype) implements
* {@link com.google.devtools.build.lib.skylarkinterface.SkylarkValue},
* is annotated with {@link com.google.devtools.build.lib.skylarkinterface.SkylarkModule}, or is a
* Skylark primitive like {@link String}.
*
* <p>Ideally, this class should not be needed, and all {@link ClassObject}s should only expose
* Skylark-friendly values.
*/
public interface SkylarkClassObject extends ClassObject {

/**
* Returns the value of the field with the given name, or null if the field does not exist.
*
* @param loc the location of the field access operation
* @param semantics the Starlark semantics, which determine the available fields
* @param name the name of the field to retrieve
* @throws EvalException if the field exists could not be retrieved
* @param loc the location in the current Starlark evaluation context
* @param starlarkSemantics the current starlark semantics, which may affect which fields are
* available, or the semantics of the available fields
* @param name the name of the field to return the value of
* @throws EvalException if a user-visible error occurs (other than non-existent field).
*/
@Nullable
Object getValue(Location loc, StarlarkSemantics semantics, String name) throws EvalException;
Object getValue(Location loc, StarlarkSemantics starlarkSemantics, String name)
throws EvalException;
}
Original file line number Diff line number Diff line change
Expand Up @@ -220,10 +220,6 @@ public boolean flagValue(FlagIdentifier flagIdentifier) {

public abstract boolean incompatibleAllowTagsPropagation();

public abstract boolean incompatibleDisallowHashingFrozenMutables();

public abstract boolean experimentalFunctionEqualityByLocation();

@Memoized
@Override
public abstract int hashCode();
Expand Down Expand Up @@ -305,8 +301,6 @@ public static Builder builderWithDefaults() {
.incompatibleDisablePartitionDefaultParameter(false)
.incompatibleAllowTagsPropagation(false)
.incompatibleAssignmentIdentifiersHaveLocalScope(false)
.incompatibleDisallowHashingFrozenMutables(false)
.experimentalFunctionEqualityByLocation(false)
.build();

/** Builder for {@link StarlarkSemantics}. All fields are mandatory. */
Expand Down Expand Up @@ -411,10 +405,6 @@ public abstract Builder incompatibleDisallowRuleExecutionPlatformConstraintsAllo

public abstract Builder incompatibleAssignmentIdentifiersHaveLocalScope(boolean value);

public abstract Builder incompatibleDisallowHashingFrozenMutables(boolean value);

public abstract Builder experimentalFunctionEqualityByLocation(boolean value);

public abstract StarlarkSemantics build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -171,9 +171,7 @@ private static StarlarkSemanticsOptions buildRandomOptions(Random rand) throws E
"--incompatible_disallow_dict_lookup_unhashable_keys=" + rand.nextBoolean(),
"--incompatible_disable_partition_default_parameter=" + rand.nextBoolean(),
"--incompatible_assignment_identifiers_have_local_scope=" + rand.nextBoolean(),
"--incompatible_disallow_hashing_frozen_mutables=" + rand.nextBoolean(),
"--internal_skylark_flag_test_canary=" + rand.nextBoolean(),
"--experimental_function_equality_by_location=" + rand.nextBoolean());
"--internal_skylark_flag_test_canary=" + rand.nextBoolean());
}

/**
Expand Down Expand Up @@ -232,9 +230,7 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) {
.incompatibleDisallowDictLookupUnhashableKeys(rand.nextBoolean())
.incompatibleDisablePartitionDefaultParameter(rand.nextBoolean())
.incompatibleAssignmentIdentifiersHaveLocalScope(rand.nextBoolean())
.incompatibleDisallowHashingFrozenMutables(rand.nextBoolean())
.internalSkylarkFlagTestCanary(rand.nextBoolean())
.experimentalFunctionEqualityByLocation(rand.nextBoolean())
.build();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3230,6 +3230,8 @@ public void testUnknownStringEscapes() throws Exception {

@Test
public void testSplitEmptySeparatorForbidden() throws Exception {
setSkylarkSemanticsOptions("--incompatible_disallow_split_empty_separator=true");

scratch.file("test/extension.bzl", "y = 'abc'.split('')");

scratch.file("test/BUILD", "load('//test:extension.bzl', 'y')", "cc_library(name = 'r')");
Expand All @@ -3239,6 +3241,17 @@ public void testSplitEmptySeparatorForbidden() throws Exception {
assertContainsEvent("Empty separator");
}

@Test
public void testSplitEmptySeparator() throws Exception {
setSkylarkSemanticsOptions("--incompatible_disallow_split_empty_separator=false");

scratch.file("test/extension.bzl", "y = 'abc'.split('')");

scratch.file("test/BUILD", "load('//test:extension.bzl', 'y')", "cc_library(name = 'r')");

getConfiguredTarget("//test:r");
}

@Test
public void testIdentifierAssignmentFromOuterScope() throws Exception {
setSkylarkSemanticsOptions("--incompatible_assignment_identifiers_have_local_scope=false");
Expand Down Expand Up @@ -3279,46 +3292,6 @@ public void testIdentifierAssignmentFromOuterScopeForbidden() throws Exception {
assertContainsEvent("local variable 'a' is referenced before assignment");
}

@Test
public void testHashFrozenList() throws Exception {
setSkylarkSemanticsOptions("--incompatible_disallow_hashing_frozen_mutables=false");
scratch.file("test/extension.bzl", "y = []");

scratch.file(
"test/BUILD", "load('//test:extension.bzl', 'y')", "{y: 1}", "cc_library(name = 'r')");

getConfiguredTarget("//test:r");
}

@Test
public void testHashFrozenListForbidden() throws Exception {
setSkylarkSemanticsOptions("--incompatible_disallow_hashing_frozen_mutables=true");
scratch.file("test/extension.bzl", "y = []");

scratch.file(
"test/BUILD", "load('//test:extension.bzl', 'y')", "{y: 1}", "cc_library(name = 'r')");

reporter.removeHandler(failFastHandler);
getConfiguredTarget("//test:r");
assertContainsEvent("unhashable type: 'list'");
}

@Test
public void testHashFrozenDeepMutableForbidden() throws Exception {
setSkylarkSemanticsOptions("--incompatible_disallow_hashing_frozen_mutables=true");
scratch.file("test/extension.bzl", "y = {}");

scratch.file(
"test/BUILD",
"load('//test:extension.bzl', 'y')",
"{('a', (y,), True): None}",
"cc_library(name = 'r')");

reporter.removeHandler(failFastHandler);
getConfiguredTarget("//test:r");
assertContainsEvent("unhashable type: 'tuple'");
}

@Test
public void testNoOutputsError() throws Exception {
scratch.file(
Expand Down

0 comments on commit 8836311

Please sign in to comment.