In repo rules, don't warn about generator_* attributes being non-cano…

…nical

A recent change added the implicit generator_* attributes ("generator_name",
"generator_function", and possibly "generator_location") to all repo rules.
This had the unfortunate side-effect of spamming all users of certain repo
rules with a warning that the values for these attributes are non-canonical.

Since these attributes are implicitly created and supplied by Bazel itself,
and since they do not (or should not) affect repo rule determinism, there is
no action needed on the part of the user. This change hardcodes them to be
excluded from the canonical warning.

Fixes #11040. Closes #11113.

PiperOrigin-RevId: 306478566

src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryResolvedEvent.java

@@ -23,6 +23,7 @@
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.events.ExtendedEventHandler.ResolvedEvent;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.Rule;
@@ -244,18 +245,32 @@ public static String getRuleDefinitionInformation(Rule rule) {
     return buf.toString();
   }
 
+  /**
+   * Attributes that may be defined on a repository rule without affecting its canonical
+   * representation. These may be created implicitly by Bazel.
+   */
+  private static final ImmutableSet<String> IGNORED_ATTRIBUTE_NAMES =
+      ImmutableSet.of("generator_name", "generator_function", "generator_location");
+
   /**
    * Compare two maps from Strings to objects, returning a pair of the map with all entries not in
    * the original map or in the original map, but with a different value, and the keys dropped from
    * the original map. However, ignore changes where a value is explicitly set to its default.
+   *
+   * <p>Ignores attributes listed in {@code IGNORED_ATTRIBUTE_NAMES}.
    */
   static Pair<Map<String, Object>, List<String>> compare(
       Map<String, Object> orig, Map<String, Object> defaults, Map<?, ?> modified) {
     ImmutableMap.Builder<String, Object> valuesChanged = ImmutableMap.<String, Object>builder();
     for (Map.Entry<?, ?> entry : modified.entrySet()) {
       if (entry.getKey() instanceof String) {
-        Object value = entry.getValue();
         String key = (String) entry.getKey();
+        if (IGNORED_ATTRIBUTE_NAMES.contains(key)) {
+          // The dict returned by the repo rule really shouldn't know about these anyway, but
+          // for symmetry we'll ignore them if they happen to be present.
+          continue;
+        }
+        Object value = entry.getValue();
         Object old = orig.get(key);
         if (old == null) {
           Object defaultValue = defaults.get(key);
@@ -271,6 +286,9 @@ static Pair<Map<String, Object>, List<String>> compare(
     }
     ImmutableList.Builder<String> keysDropped = ImmutableList.<String>builder();
     for (String key : orig.keySet()) {
+      if (IGNORED_ATTRIBUTE_NAMES.contains(key)) {
+        continue;
+      }
       if (!modified.containsKey(key)) {
         keysDropped.add(key);
       }

src/test/shell/bazel/workspace_resolved_test.sh

@@ -824,6 +824,7 @@ time_rule(name="timestamprepo")
 EOF
 
     bazel sync --distdir=${EXTREPODIR}/test_WORKSPACE/distdir --experimental_repository_resolved_file=resolved.bzl
+    cat resolved.bzl > /dev/null || fail "resolved.bzl should exist"
     bazel sync --distdir=${EXTREPODIR}/test_WORKSPACE/distdir --experimental_repository_hash_file=`pwd`/resolved.bzl \
           --experimental_verify_repository_rules='//:rule.bzl%time_rule' \
           > "${TEST_log}" 2>&1 && fail "expected failure" || :
@@ -1216,4 +1217,52 @@ EOF
   expect_log "  TEST_TMPDIR/.*/external/bazel_tools/tools/build_defs/repo/http.bzl:"
 }
 
+# Regression test for #11040.
+#
+# Test that a canonical repo warning is generated for explicitly specified
+# attributes whose values differ, and that it is never generated for implicitly
+# created attributes (in particular, the generator_* attributes).
+test_canonical_warning() {
+  EXTREPODIR=`pwd`
+  tar xvf ${TEST_SRCDIR}/test_WORKSPACE_files/archives.tar
+
+  mkdir main
+  touch main/BUILD
+  cat > main/reporule.bzl <<EOF
+def _impl(repository_ctx):
+    repository_ctx.file("a.txt", "A")
+    repository_ctx.file("BUILD.bazel", "exports_files(['a.txt'])")
+    # Don't include "name", test that we warn about it below.
+    return {"myattr": "bar"}
+
+reporule = repository_rule(
+    implementation = _impl,
+    attrs = {
+        "myattr": attr.string()
+    })
+
+# We need to use a macro for the generator_* attributes to be defined.
+def instantiate_reporule(name, **kwargs):
+    reporule(name=name, **kwargs)
+EOF
+  cat > main/WORKSPACE <<EOF
+workspace(name = "main")
+load("//:reporule.bzl", "instantiate_reporule")
+instantiate_reporule(
+  name = "myrepo",
+  myattr = "foo"
+)
+EOF
+
+  cd main
+  # We should get a warning for "myattr" having a changed value and for "name"
+  # being dropped, but not for the generator_* attributes.
+  bazel sync --distdir=${EXTREPODIR}/test_WORKSPACE/distdir >/dev/null 2>$TEST_log
+  bazel clean --expunge
+  expect_log "Rule 'myrepo' indicated that a canonical reproducible form \
+can be obtained by modifying arguments myattr = \"bar\" and dropping \
+\[.*\"name\".*\]"
+  expect_not_log "Rule 'myrepo' indicated .*generator"
+}
+
 run_suite "workspace_resolved_test tests"