Skip to content

Commit

Permalink
Flip symbolic macro flags
Browse files Browse the repository at this point in the history
This enables `--experimental_enable_first_class_macros` and `--incompatible_simplify_unconditional_selects_in_rule_attrs` to true. The former releases Symbolic Macros, and with it, Finalizers and Macro-Aware Visibility. The latter is a quality-of-life improvement for macro users that is technically a breaking change: Any rule instantiated with an attribute value of `select({"//conditions:default": foo})` instead sees just `foo`.

Main milestone of bazelbuild#19922.

RELNOTES: Symbolic Macros -- and with them, Finalizers and the new Macro-Aware Visibility model -- are now generally available (`--experimental_enable_first_class_macros` now defaults to true). Trivial `select()` values are automatically unwrapped (`--incompatible_simplify_unconditional_selects_in_rule_attrs` now defaults to true).
PiperOrigin-RevId: 680779247
Change-Id: Icdbf4b47bc893ba61417eddc22c79d85b6f5d920
  • Loading branch information
brandjon authored and copybara-github committed Oct 1, 2024
1 parent cd82f68 commit 0c100ef
Show file tree
Hide file tree
Showing 16 changed files with 86 additions and 80 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -235,10 +235,10 @@ public final class BuildLanguageOptions extends OptionsBase {

@Option(
name = "experimental_enable_first_class_macros",
defaultValue = "false",
defaultValue = "true",
documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
effectTags = OptionEffectTag.BUILD_FILE_SEMANTICS,
help = "If set to true, enables the `macro()` construct for defining first-class macros.")
help = "If set to true, enables the `macro()` construct for defining symbolic macros.")
public boolean experimentalEnableFirstClassMacros;

@Option(
Expand Down Expand Up @@ -787,7 +787,7 @@ public final class BuildLanguageOptions extends OptionsBase {

@Option(
name = "incompatible_simplify_unconditional_selects_in_rule_attrs",
defaultValue = "false",
defaultValue = "true",
documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS},
metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
Expand Down Expand Up @@ -940,7 +940,7 @@ public StarlarkSemantics toStarlarkSemantics() {
public static final String EXPERIMENTAL_SINGLE_PACKAGE_TOOLCHAIN_BINDING =
"-experimental_single_package_toolchain_binding";
public static final String EXPERIMENTAL_ENABLE_FIRST_CLASS_MACROS =
"-experimental_enable_first_class_macros";
"+experimental_enable_first_class_macros";
public static final String EXPERIMENTAL_ENABLE_SCL_DIALECT = "+experimental_enable_scl_dialect";
public static final String ENABLE_BZLMOD = "+enable_bzlmod";
public static final String ENABLE_WORKSPACE = "-enable_workspace";
Expand Down Expand Up @@ -1012,7 +1012,7 @@ public StarlarkSemantics toStarlarkSemantics() {
public static final String INCOMPATIBLE_DISALLOW_CTX_RESOLVE_TOOLS =
"+incompatible_disallow_ctx_resolve_tools";
public static final String INCOMPATIBLE_SIMPLIFY_UNCONDITIONAL_SELECTS_IN_RULE_ATTRS =
"-incompatible_simplify_unconditional_selects_in_rule_attrs";
"+incompatible_simplify_unconditional_selects_in_rule_attrs";
// non-booleans
public static final StarlarkSemantics.Key<String> EXPERIMENTAL_BUILTINS_BZL_PATH =
new StarlarkSemantics.Key<>("experimental_builtins_bzl_path", "%bundled%");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,22 @@ public final void createMapper() throws Exception {
scratchConfiguredTargetAndData(
"foo",
"myrule",
"cc_binary(",
" name = 'myrule',",
" srcs = [':a.cc'],",
" linkstatic = select({'//conditions:default': 1}))");
"""
# Needed to avoid select() being eliminated as trivial.
config_setting(
name = "config",
values = {"define": "pi=3"},
)
cc_binary(
name = "myrule",
srcs = [":a.cc"],
linkstatic = select({
":config": 1,
"//conditions:default": 1,
}),
)
""");

RuleConfiguredTarget ct = (RuleConfiguredTarget) ctad.getConfiguredTarget();
rule = (Rule) ctad.getTargetForTesting();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2146,9 +2146,8 @@ public void stringListDictTypeConcatConfigurable() throws Exception {
}

@Test
public void incompatibleSimplifyUnconditionalSelectsInRuleAttrs_checksForNonconfigurableAttrs()
public void assigningSelectToNonconfigurableAttr_fails_evenIfSelectIsSimplifiableUnconditional()
throws Exception {
setBuildLanguageOptions("--incompatible_simplify_unconditional_selects_in_rule_attrs=true");
writeConfigRules();
scratch.file(
"foo/BUILD",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
import com.google.testing.junit.testparameterinjector.TestParameterInjector;
import com.google.testing.junit.testparameterinjector.TestParameters;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;

Expand All @@ -34,11 +33,6 @@
@RunWith(TestParameterInjector.class)
public final class MacroVisibilityTest extends BuildViewTestCase {

@Before
public void setUp() throws Exception {
setBuildLanguageOptions("--experimental_enable_first_class_macros");
}

@Override
protected String getDefaultVisibility() {
// We're testing visibility. Avoid having to litter our test cases with `visibility=` attribute
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,21 +35,13 @@
import java.util.List;
import javax.annotation.Nullable;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;

/** Tests the execution of symbolic macro implementations. */
@RunWith(TestParameterInjector.class)
public final class SymbolicMacroTest extends BuildViewTestCase {

@Before
public void setUp() throws Exception {
setBuildLanguageOptions(
"--experimental_enable_first_class_macros",
"--incompatible_simplify_unconditional_selects_in_rule_attrs");
}

/**
* Returns a package by the given name (no leading "//"), or null upon {@link
* NoSuchPackageException}.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ public final class VisibilityProviderTest extends BuildViewTestCase {
@Before
public void setUp() throws Exception {
setBuildLanguageOptions(
"--experimental_enable_first_class_macros",
// Let's test the case where input files have proper visibilities by default.
"--incompatible_no_implicit_file_export");

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,20 @@ public void testNoneValueOnDefaultConditionWithNullDefault() throws Exception {

@Test
public void testNoneValueOnMandatoryAttribute() throws Exception {
scratch.file("a/BUILD", "alias(name='a', actual=select({'//conditions:default': None}))");
scratch.file(
"a/BUILD",
"""
# Needed to avoid select() being eliminated as trivial.
config_setting(
name = "config",
values = {"define": "pi=3"},
)
alias(
name = "a",
actual = select({":config": None, "//conditions:default": None}),
)
""");
reporter.removeHandler(failFastHandler);
getConfiguredTarget("//a:a");
assertContainsEvent("Mandatory attribute 'actual' resolved to 'None'");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,9 +123,18 @@ def _impl(ctx):
"""
load("//test/starlark:rulestr.bzl", "my_rule", "save_dep")
# Needed to avoid select() being eliminated as trivial.
config_setting(
name = "config",
values = {"define": "pi=3"},
)
my_rule(
name = "x",
dep = select({"//conditions:default": None}),
dep = select({
":config": None,
"//conditions:default": None,
}),
)
save_dep("x")
Expand All @@ -135,7 +144,8 @@ def _impl(ctx):
assertThat(getConfiguredTarget("//test/getrule:x")).isNotNull();

// We have to compare by stringification because SelectorValue has reference equality semantics.
assertThat(getSaved("dep").toString()).isEqualTo("select({\"//conditions:default\": None})");
assertThat(getSaved("dep").toString())
.isEqualTo("select({\":config\": None, \"//conditions:default\": None})");
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1181,7 +1181,6 @@ public void testPackageDefaultRestrictionDuplicates() throws Exception {
* <p>The macro does not define any targets.
*/
private void defineEmptyMacroBzl() throws Exception {
setBuildLanguageOptions("--experimental_enable_first_class_macros");
scratch.file(
"pkg/my_macro.bzl",
"""
Expand Down Expand Up @@ -1372,7 +1371,6 @@ public void testSymbolicMacro_macroPreventsImplicitCreationOfInputFilesUnderItsN
@Test
public void testSymbolicMacro_implicitCreationOfInputFilesIsNotTriggeredByMacros()
throws Exception {
setBuildLanguageOptions("--experimental_enable_first_class_macros");
scratch.file(
"pkg/my_macro.bzl",
"""
Expand Down Expand Up @@ -1401,7 +1399,6 @@ def _impl(name, visibility):

@Test
public void testSymbolicMacro_deferredEvaluationExpandsTransitively() throws Exception {
setBuildLanguageOptions("--experimental_enable_first_class_macros");
scratch.file(
"pkg/my_macro.bzl",
"""
Expand Down Expand Up @@ -1456,7 +1453,6 @@ def _impl(name, visibility, height):

@Test
public void testSymbolicMacro_recursionProhibitedWithEagerEvaluation() throws Exception {
setBuildLanguageOptions("--experimental_enable_first_class_macros");
defineRecursiveMacro(/* deferredEvaluation= */ false);
expectEvalError(
"""
Expand All @@ -1475,7 +1471,6 @@ public void testSymbolicMacro_recursionProhibitedWithEagerEvaluation() throws Ex

@Test
public void testSymbolicMacro_recursionProhibitedWithDeferredEvaluation() throws Exception {
setBuildLanguageOptions("--experimental_enable_first_class_macros");
defineRecursiveMacro(/* deferredEvaluation= */ true);
expectEvalError(
"""
Expand All @@ -1494,7 +1489,6 @@ public void testSymbolicMacro_recursionProhibitedWithDeferredEvaluation() throws

@Test
public void testSymbolicMacro_indirectRecursionAlsoProhibited() throws Exception {
setBuildLanguageOptions("--experimental_enable_first_class_macros");
// Define a pair of macros where A calls B calls A (and then would stop, if allowed to get that
// far). Wrap it in a different entry point to test that the non-cyclic part is included in the
// traceback.
Expand Down Expand Up @@ -1561,7 +1555,6 @@ private void assertVisibilityIs(Target target, String... visibilityLabels) {
}

private void enableMacrosAndUsePrivateVisibility() throws Exception {
setBuildLanguageOptions("--experimental_enable_first_class_macros");
// BuildViewTestCase makes everything public by default.
setPackageOptions("--default_visibility=private");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import javax.annotation.Nullable;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
Expand All @@ -29,11 +28,6 @@
@RunWith(JUnit4.class)
public final class RuleFinalizerTest extends BuildViewTestCase {

@Before
public void setUp() throws Exception {
setBuildLanguageOptions("--experimental_enable_first_class_macros");
}

/**
* Returns a package by the given name (no leading "//"), or null upon {@link
* NoSuchPackageException}.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1678,8 +1678,21 @@ def _rule_impl(ctx):
scratch.file(
"test/BUILD",
"""
load('//test:aspect.bzl', 'my_rule')
my_rule(name = 'xxx', my_attr = select({'//conditions:default': 'foo'}))
load("//test:aspect.bzl", "my_rule")
# Needed to avoid the select() being eliminated as trivial.
config_setting(
name = "config",
values = {"defines": "something"},
)
my_rule(
name = "xxx",
my_attr = select({
":config": "foo",
"//conditions:default": "bar",
}),
)
""");

reporter.removeHandler(failFastHandler);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -660,9 +660,18 @@ public void flagIsAlias_usesSelect() throws Exception {
scratch.file(
"test/pkg/BUILD",
"""
# Needed to avoid select() being eliminated as trivial.
config_setting(
name = "config",
values = {"define": "pi=3"},
)
alias(
name = "two",
actual = select({"//conditions:default": "//test:three"}),
actual = select({
":config": "//test:three",
"//conditions:default": "//test:three",
}),
)
""");

Expand Down
Loading

0 comments on commit 0c100ef

Please sign in to comment.