Address review comments

go/private/extensions.bzl

@@ -46,8 +46,8 @@ def _go_sdk_impl(ctx):
             # This is acceptable if brought in by the root module as the user is responsible for any
             # conflicts that arise. rules_go itself provides "go_default_sdk", which is used by
             # Gazelle to bootstrap itself.
-            # TODO: Investigate whether Gazelle can use the first user-defined SDK instead to
-            #  prevent unnecessary downloads.
+            # TODO(https://github.com/bazelbuild/bazel-gazelle/issues/1469): Investigate whether
+            #  Gazelle can use the first user-defined SDK instead to prevent unnecessary downloads.
             if (not module.is_root and not module.name == "rules_go") and download_tag.name:
                 fail("go_sdk.download: name must not be specified in non-root module " + module.name)
 
@@ -132,6 +132,12 @@ def _default_go_sdk_name(*, module, multi_version, tag_type, index):
     )
 
 def _toolchain_prefix(index, name):
+    """Prefixes the given name with the index, padded with zeros to ensure lexicographic sorting.
+
+    Examples:
+      _toolchain_prefix(   2, "foo") == "_0002_foo_"
+      _toolchain_prefix(2000, "foo") == "_2000_foo_"
+    """
     return "_{}_{}_".format(_left_pad_zero(index, _TOOLCHAIN_INDEX_PAD_LENGTH), name)
 
 def _left_pad_zero(index, length):

go/private/sdk.bzl

@@ -162,9 +162,10 @@ def _define_version_constants(version, prefix = ""):
     )
 
 def _to_constant_name(s):
-    return "".join([c if c.isalnum() else "_" for c in s.elems()]).upper()
+    # Prefix with _ as identifiers are not allowed to start with numbers.
+    return "_" + "".join([c if c.isalnum() else "_" for c in s.elems()]).upper()
 
-def _go_toolchains_single_definition(ctx, *, prefix, goos, goarch, sdk_repo, sdk_type, sdk_version):
+def go_toolchains_single_definition(ctx, *, prefix, goos, goarch, sdk_repo, sdk_type, sdk_version):
     if not goos and not goarch:
         goos, goarch = _detect_host_platform(ctx)
     else:
@@ -190,7 +191,8 @@ def _go_toolchains_single_definition(ctx, *, prefix, goos, goarch, sdk_repo, sdk
     {identifier_prefix}MINOR_VERSION = "MINOR_VERSION",
     {identifier_prefix}PATCH_VERSION = "PATCH_VERSION",
     {identifier_prefix}PRERELEASE_SUFFIX = "PRERELEASE_SUFFIX",
-)""".format(
+)
+""".format(
             sdk_repo = sdk_repo,
             identifier_prefix = identifier_prefix,
         ))
@@ -228,14 +230,7 @@ def go_toolchains_build_file_content(
         sdk_repos,
         sdk_types,
         sdk_versions):
-    if len({len(l): None for l in [
-        prefixes,
-        geese,
-        goarchs,
-        sdk_repos,
-        sdk_types,
-        sdk_versions,
-    ]}) != 1:
+    if not _have_same_length(prefixes, geese, goarchs, sdk_repos, sdk_types, sdk_versions):
         fail("all lists must have the same length")
 
     loads = [
@@ -246,7 +241,7 @@ def go_toolchains_build_file_content(
     ]
 
     for i in range(len(geese)):
-        definition = _go_toolchains_single_definition(
+        definition = go_toolchains_single_definition(
             ctx,
             prefix = prefixes[i],
             goos = geese[i],
@@ -622,6 +617,11 @@ def _version_string(v):
         v = v[:-1]
     return ".".join([str(n) for n in v]) + suffix
 
+def _have_same_length(*lists):
+    if not lists:
+        fail("expected at least one list")
+    return len({len(l): None for l in lists}) == 1
+
 def go_register_toolchains(version = None, nogo = None, go_version = None, experiments = None):
     """See /go/toolchains.rst#go-register-toolchains for full documentation."""
     if not version:

tests/core/starlark/BUILD.bazel

@@ -1,3 +1,6 @@
 load(":common_tests.bzl", "common_test_suite")
+load(":sdk_tests.bzl", "sdk_test_suite")
 
 common_test_suite()
+
+sdk_test_suite()

tests/core/starlark/sdk_tests.bzl

@@ -0,0 +1,86 @@
+load("@bazel_skylib//lib:unittest.bzl", "asserts", "unittest")
+load("//go/private:sdk.bzl", "go_toolchains_single_definition")
+
+def _go_toolchains_single_definition_with_version_test(ctx):
+    env = unittest.begin(ctx)
+
+    result = go_toolchains_single_definition(
+        ctx = None,
+        prefix = "123_prefix_",
+        goos = "linux",
+        goarch = "amd64",
+        sdk_repo = "sdk_repo",
+        sdk_type = "download",
+        sdk_version = "1.20.2rc1",
+    )
+    asserts.equals(env, [], result.loads)
+    asserts.equals(env, [
+        """
+_123_PREFIX_MAJOR_VERSION = "1"
+_123_PREFIX_MINOR_VERSION = "20"
+_123_PREFIX_PATCH_VERSION = "2"
+_123_PREFIX_PRERELEASE_SUFFIX = "rc1"
+""",
+        """declare_bazel_toolchains(
+    prefix = "123_prefix_",
+    go_toolchain_repo = "@sdk_repo",
+    host_goarch = "amd64",
+    host_goos = "linux",
+    major = _123_PREFIX_MAJOR_VERSION,
+    minor = _123_PREFIX_MINOR_VERSION,
+    patch = _123_PREFIX_PATCH_VERSION,
+    prerelease = _123_PREFIX_PRERELEASE_SUFFIX,
+    sdk_type = "download",
+)
+""",
+    ], result.chunks)
+
+    return unittest.end(env)
+
+go_toolchains_single_definition_with_version_test = unittest.make(_go_toolchains_single_definition_with_version_test)
+
+def _go_toolchains_single_definition_without_version_test(ctx):
+    env = unittest.begin(ctx)
+
+    result = go_toolchains_single_definition(
+        ctx = None,
+        prefix = "123_prefix_",
+        goos = "linux",
+        goarch = "amd64",
+        sdk_repo = "sdk_repo",
+        sdk_type = "download",
+        sdk_version = None,
+    )
+    asserts.equals(env, ["""load(
+    "@sdk_repo//:version.bzl",
+    _123_PREFIX_MAJOR_VERSION = "MAJOR_VERSION",
+    _123_PREFIX_MINOR_VERSION = "MINOR_VERSION",
+    _123_PREFIX_PATCH_VERSION = "PATCH_VERSION",
+    _123_PREFIX_PRERELEASE_SUFFIX = "PRERELEASE_SUFFIX",
+)
+"""], result.loads)
+    asserts.equals(env, [
+        """declare_bazel_toolchains(
+    prefix = "123_prefix_",
+    go_toolchain_repo = "@sdk_repo",
+    host_goarch = "amd64",
+    host_goos = "linux",
+    major = _123_PREFIX_MAJOR_VERSION,
+    minor = _123_PREFIX_MINOR_VERSION,
+    patch = _123_PREFIX_PATCH_VERSION,
+    prerelease = _123_PREFIX_PRERELEASE_SUFFIX,
+    sdk_type = "download",
+)
+""",
+    ], result.chunks)
+
+    return unittest.end(env)
+
+go_toolchains_single_definition_without_version_test = unittest.make(_go_toolchains_single_definition_without_version_test)
+
+def sdk_test_suite():
+    unittest.suite(
+        "sdk_tests",
+        go_toolchains_single_definition_with_version_test,
+        go_toolchains_single_definition_without_version_test,
+    )