Transition on edges not self (#3116)

As per bazelbuild/bazel#15157 currently we
can't configure any transition-relevant attributes using select. This is
because we use self-transitions on targets, and when this happens,
configurable attributes aren't passed to the transition, so we make
incorrect transition decisions.

We only actually consult configuration data in `_go_context_data`, so we
only actually need to transition on the edges which (transitively) reach
a `_go_context_data`, which is `_go_context_data` itself and `deps`.

go/private/context.bzl

@@ -392,20 +392,21 @@ def go_context(ctx, attr = None):
     coverdata = None
     nogo = None
     if hasattr(attr, "_go_context_data"):
-        if CgoContextInfo in attr._go_context_data:
-            cgo_context_info = attr._go_context_data[CgoContextInfo]
-        go_config_info = attr._go_context_data[GoConfigInfo]
-        stdlib = attr._go_context_data[GoStdLib]
-        coverdata = attr._go_context_data[GoContextInfo].coverdata
-        nogo = attr._go_context_data[GoContextInfo].nogo
+        go_context_data = _flatten_possibly_transitioned_attr(attr._go_context_data)
+        if CgoContextInfo in go_context_data:
+            cgo_context_info = go_context_data[CgoContextInfo]
+        go_config_info = go_context_data[GoConfigInfo]
+        stdlib = go_context_data[GoStdLib]
+        coverdata = go_context_data[GoContextInfo].coverdata
+        nogo = go_context_data[GoContextInfo].nogo
     if getattr(attr, "_cgo_context_data", None) and CgoContextInfo in attr._cgo_context_data:
         cgo_context_info = attr._cgo_context_data[CgoContextInfo]
     if getattr(attr, "cgo_context_data", None) and CgoContextInfo in attr.cgo_context_data:
         cgo_context_info = attr.cgo_context_data[CgoContextInfo]
     if hasattr(attr, "_go_config"):
         go_config_info = attr._go_config[GoConfigInfo]
     if hasattr(attr, "_stdlib"):
-        stdlib = attr._stdlib[GoStdLib]
+        stdlib = _flatten_possibly_transitioned_attr(attr._stdlib)[GoStdLib]
 
     mode = get_mode(ctx, toolchain, cgo_context_info, go_config_info)
     tags = mode.tags
@@ -864,3 +865,17 @@ go_config = rule(
 
 def _expand_opts(go, attribute_name, opts):
     return [go._ctx.expand_make_variables(attribute_name, opt, {}) for opt in opts]
+
+_LIST_TYPE = type([])
+
+# Used to get attribute values which may have been transitioned.
+# Transitioned attributes end up as lists.
+# We never use split-transitions, so we always expect exactly one element in those lists.
+# But if the attribute wasn't transitioned, it won't be a list.
+def _flatten_possibly_transitioned_attr(maybe_list):
+    if type(maybe_list) == _LIST_TYPE:
+        if len(maybe_list) == 1:
+            return maybe_list[0]
+        else:
+            fail("Expected exactly one element in list but got {}".format(maybe_list))
+    return maybe_list

go/private/mode.bzl

@@ -80,8 +80,8 @@ def get_mode(ctx, go_toolchain, cgo_context_info, go_config_info):
     debug = go_config_info.debug if go_config_info else False
     linkmode = go_config_info.linkmode if go_config_info else LINKMODE_NORMAL
     cover_format = go_config_info and go_config_info.cover_format
-    goos = go_toolchain.default_goos
-    goarch = go_toolchain.default_goarch
+    goos = go_toolchain.default_goos if getattr(ctx.attr, "goos", "auto") == "auto" else ctx.attr.goos
+    goarch = go_toolchain.default_goarch if getattr(ctx.attr, "goarch", "auto") == "auto" else ctx.attr.goarch
 
     # TODO(jayconrod): check for more invalid and contradictory settings.
     if pure and race:

go/private/rules/binary.bzl

@@ -33,8 +33,7 @@ load(
 )
 load(
     "//go/private/rules:transition.bzl",
-    "go_transition_rule",
-    "non_go_transition",
+    "go_transition",
 )
 load(
     "//go/private:mode.bzl",
@@ -189,7 +188,6 @@ _go_binary_kwargs = {
     "attrs": {
         "srcs": attr.label_list(
             allow_files = go_exts + asm_exts + cgo_exts,
-            cfg = non_go_transition,
             doc = """The list of Go source files that are compiled to create the package.
             Only `.go` and `.s` files are permitted, unless the `cgo`
             attribute is set, in which case,
@@ -200,7 +198,6 @@ _go_binary_kwargs = {
         ),
         "data": attr.label_list(
             allow_files = True,
-            cfg = non_go_transition,
             doc = """List of files needed by this rule at run-time. This may include data files
             needed or other programs that may be executed. The [bazel] package may be
             used to locate run files; they may appear in different places depending on the
@@ -213,6 +210,7 @@ _go_binary_kwargs = {
             doc = """List of Go libraries this package imports directly.
             These may be `go_library` rules or compatible rules with the [GoLibrary] provider.
             """,
+            cfg = go_transition,
         ),
         "embed": attr.label_list(
             providers = [GoLibrary],
@@ -225,10 +223,10 @@ _go_binary_kwargs = {
             embedding binary may not also have `cgo = True`. See [Embedding] for
             more information.
             """,
+            cfg = go_transition,
         ),
         "embedsrcs": attr.label_list(
             allow_files = True,
-            cfg = non_go_transition,
             doc = """The list of files that may be embedded into the compiled package using
             `//go:embed` directives. All files must be in the same logical directory
             or a subdirectory as source files. All source files containing `//go:embed`
@@ -281,7 +279,6 @@ _go_binary_kwargs = {
             """,
         ),
         "cdeps": attr.label_list(
-            cfg = non_go_transition,
             doc = """The list of other libraries that the c code depends on.
             This can be anything that would be allowed in [cc_library deps]
             Only valid if `cgo` = `True`.
@@ -390,7 +387,7 @@ _go_binary_kwargs = {
             </ul>
             """,
         ),
-        "_go_context_data": attr.label(default = "//:go_context_data"),
+        "_go_context_data": attr.label(default = "//:go_context_data", cfg = go_transition),
         "_allowlist_function_transition": attr.label(
             default = "@bazel_tools//tools/allowlists/function_transition_allowlist",
         ),
@@ -410,8 +407,7 @@ _go_binary_kwargs = {
 }
 
 go_binary = rule(executable = True, **_go_binary_kwargs)
-go_transition_binary = go_transition_rule(executable = True, **_go_binary_kwargs)
-go_non_executable_transition_binary = go_transition_rule(executable = False, **_go_binary_kwargs)
+go_non_executable_binary = rule(executable = False, **_go_binary_kwargs)
 
 def _go_tool_binary_impl(ctx):
     sdk = ctx.attr.sdk[GoSDK]

go/private/rules/test.bzl

@@ -43,8 +43,7 @@ load(
 )
 load(
     "//go/private/rules:transition.bzl",
-    "go_transition_rule",
-    "non_go_transition",
+    "go_transition",
 )
 load(
     "//go/private:mode.bzl",
@@ -196,7 +195,6 @@ _go_test_kwargs = {
     "attrs": {
         "data": attr.label_list(
             allow_files = True,
-            cfg = non_go_transition,
             doc = """List of files needed by this rule at run-time. This may include data files
             needed or other programs that may be executed. The [bazel] package may be
             used to locate run files; they may appear in different places depending on the
@@ -206,7 +204,6 @@ _go_test_kwargs = {
         ),
         "srcs": attr.label_list(
             allow_files = go_exts + asm_exts + cgo_exts,
-            cfg = non_go_transition,
             doc = """The list of Go source files that are compiled to create the package.
             Only `.go` and `.s` files are permitted, unless the `cgo`
             attribute is set, in which case,
@@ -220,6 +217,7 @@ _go_test_kwargs = {
             doc = """List of Go libraries this test imports directly.
             These may be go_library rules or compatible rules with the [GoLibrary] provider.
             """,
+            cfg = go_transition,
         ),
         "embed": attr.label_list(
             providers = [GoLibrary],
@@ -231,10 +229,10 @@ _go_test_kwargs = {
             and the embedding library may not also have `cgo = True`. See [Embedding]
             for more information.
             """,
+            cfg = go_transition,
         ),
         "embedsrcs": attr.label_list(
             allow_files = True,
-            cfg = non_go_transition,
             doc = """The list of files that may be embedded into the compiled package using
             `//go:embed` directives. All files must be in the same logical directory
             or a subdirectory as source files. All source files containing `//go:embed`
@@ -307,7 +305,6 @@ _go_test_kwargs = {
             """,
         ),
         "cdeps": attr.label_list(
-            cfg = non_go_transition,
             doc = """The list of other libraries that the c code depends on.
             This can be anything that would be allowed in [cc_library deps]
             Only valid if `cgo` = `True`.
@@ -402,10 +399,11 @@ _go_test_kwargs = {
             See [Cross compilation] for more information.
             """,
         ),
-        "_go_context_data": attr.label(default = "//:go_context_data"),
+        "_go_context_data": attr.label(default = "//:go_context_data", cfg = go_transition),
         "_testmain_additional_deps": attr.label_list(
             providers = [GoLibrary],
             default = ["//go/tools/bzltestutil"],
+            cfg = go_transition,
         ),
         # Workaround for bazelbuild/bazel#6293. See comment in lcov_merger.sh.
         "_lcov_merger": attr.label(
@@ -453,7 +451,6 @@ _go_test_kwargs = {
 }
 
 go_test = rule(**_go_test_kwargs)
-go_transition_test = go_transition_rule(**_go_test_kwargs)
 
 def _recompile_external_deps(go, external_source, internal_archive, library_labels):
     """Recompiles some archives in order to split internal and external tests.

go/private/rules/transition.bzl

@@ -80,62 +80,6 @@ _SETTING_KEY_TO_ORIGINAL_SETTING_KEY = {
     for setting in TRANSITIONED_GO_SETTING_KEYS
 }
 
-def go_transition_wrapper(kind, transition_kind, name, **kwargs):
-    """Wrapper for rules that may use transitions.
-
-    This is used in place of instantiating go_binary or go_transition_binary
-    directly. If one of the transition attributes is set explicitly, it
-    instantiates the rule with a transition. Otherwise, it instantiates the
-    regular rule. This prevents targets from being rebuilt for an alternative
-    configuration identical to the default configuration.
-    """
-    transition_keys = ("goos", "goarch", "pure", "static", "msan", "race", "gotags", "linkmode")
-    need_transition = any([key in kwargs for key in transition_keys])
-    if need_transition:
-        transition_kind(name = name, **kwargs)
-    else:
-        kind(name = name, **kwargs)
-
-def go_transition_rule(**kwargs):
-    """Like "rule", but adds a transition and mode attributes."""
-    kwargs = dict(kwargs)
-    kwargs["attrs"].update({
-        "goos": attr.string(
-            default = "auto",
-            values = ["auto"] + {goos: None for goos, _ in GOOS_GOARCH}.keys(),
-        ),
-        "goarch": attr.string(
-            default = "auto",
-            values = ["auto"] + {goarch: None for _, goarch in GOOS_GOARCH}.keys(),
-        ),
-        "pure": attr.string(
-            default = "auto",
-            values = ["auto", "on", "off"],
-        ),
-        "static": attr.string(
-            default = "auto",
-            values = ["auto", "on", "off"],
-        ),
-        "msan": attr.string(
-            default = "auto",
-            values = ["auto", "on", "off"],
-        ),
-        "race": attr.string(
-            default = "auto",
-            values = ["auto", "on", "off"],
-        ),
-        "gotags": attr.string_list(default = []),
-        "linkmode": attr.string(
-            default = "auto",
-            values = ["auto"] + LINKMODES,
-        ),
-        "_whitelist_function_transition": attr.label(
-            default = "@bazel_tools//tools/whitelists/function_transition_whitelist",
-        ),
-    })
-    kwargs["cfg"] = go_transition
-    return rule(**kwargs)
-
 def _go_transition_impl(settings, attr):
     # NOTE: Keep the list of rules_go settings set by this transition in sync
     # with POTENTIALLY_TRANSITIONED_SETTINGS.

go/private/rules/wrappers.bzl

@@ -26,19 +26,15 @@ load(
 load(
     "//go/private/rules:binary.bzl",
     "go_binary",
-    "go_non_executable_transition_binary",
-    "go_transition_binary",
+    "go_non_executable_binary",
 )
 load(
     "//go/private/rules:test.bzl",
     "go_test",
-    "go_transition_test",
-)
-load(
-    "//go/private/rules:transition.bzl",
-    "go_transition_wrapper",
 )
 
+_SELECT_TYPE = type(select({"//conditions:default": ""}))
+
 def _cgo(name, kwargs):
     if "objc" in kwargs:
         fail("//{}:{}: the objc attribute has been removed. .m sources may be included in srcs or may be extracted into a separated objc_library listed in cdeps.".format(native.package_name(), name))
@@ -51,12 +47,24 @@ def go_library_macro(name, **kwargs):
 def go_binary_macro(name, **kwargs):
     """See docs/go/core/rules.md#go_binary for full documentation."""
     _cgo(name, kwargs)
+
+    if kwargs.get("goos") != None or kwargs.get("goarch") != None:
+        for key, value in kwargs.items():
+            if type(value) == _SELECT_TYPE:
+                # In the long term, we should replace goos/goarch with Bazel-native platform
+                # support, but while we have the mechanisms, we try to avoid people trying to use
+                # _both_ goos/goarch _and_ native platform support.
+                #
+                # It's unclear to users whether the select should happen before or after the
+                # goos/goarch is reconfigured, and we can't interpose code to force either
+                # behaviour, so we forbid this.
+                fail("Cannot use select for go_binary with goos/goarch set, but {} was a select".format(key))
+
     if kwargs.get("linkmode", default = LINKMODE_NORMAL) in LINKMODES_EXECUTABLE:
-        go_transition_wrapper(go_binary, go_transition_binary, name = name, **kwargs)
+        go_binary(name = name, **kwargs)
     else:
-        # A non-normal link mode implies the use of transitions, so we don't have to define a
-        # non-executable version of the untransitioned go_binary.
-        go_transition_wrapper(None, go_non_executable_transition_binary, name = name, **kwargs)
+        go_non_executable_binary(name = name, **kwargs)
+
     if kwargs.get("linkmode") in (LINKMODE_C_ARCHIVE, LINKMODE_C_SHARED):
         # Create an alias to tell users of the `.cc` rule that it is deprecated.
         native.alias(
@@ -70,4 +78,4 @@ def go_binary_macro(name, **kwargs):
 def go_test_macro(name, **kwargs):
     """See docs/go/core/rules.md#go_test for full documentation."""
     _cgo(name, kwargs)
-    go_transition_wrapper(go_test, go_transition_test, name = name, **kwargs)
+    go_test(name = name, **kwargs)

tests/core/go_binary/BUILD.bazel

@@ -4,6 +4,16 @@ load(":many_deps.bzl", "many_deps")
 
 test_suite(name = "go_binary")
 
+go_bazel_test(
+    name = "configurable_attribute_bad_test",
+    srcs = ["configurable_attribute_bad_test.go"],
+)
+
+go_bazel_test(
+    name = "configurable_attribute_good_test",
+    srcs = ["configurable_attribute_good_test.go"],
+)
+
 go_binary(
     name = "hello",
     srcs = ["hello.go"],