[6.4.0] Make lockfile's RepoSpec attributes more readable (#19748)

String-valued tag and repo rule attributes in the `MODULE.bazel.lock`
file have to be serialized in a way that distinguishes them from
label-valued attributes. This commit uses the fact that the string
representation of labels always starts with `@@` to escape strings only
if they start with `@@` (or happen to be of the form of an escaped
string). Since string-valued attributes rarely start with `@@`, in
particular when specified by humans and not macros, this greatly reduces
the need for escaping in the lockfile.

This commit raises the lockfile version to 3.

Along the way also disables HTML escaping for attributes as it isn't
needed and results in less readable representations of certain
characters.

Closes #19684.

PiperOrigin-RevId: 571037360
Change-Id: I04bb60d371cd09326780004122e729d78c99732a

Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/AttributeValuesAdapter.java

@@ -15,8 +15,11 @@
 
 package com.google.devtools.build.lib.bazel.bzlmod;
 
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.gson.Gson;
+import com.google.gson.GsonBuilder;
 import com.google.gson.JsonArray;
 import com.google.gson.JsonElement;
 import com.google.gson.JsonNull;
@@ -40,7 +43,7 @@
 /** Helps serialize/deserialize {@link AttributeValues}, which contains Starlark values. */
 public class AttributeValuesAdapter extends TypeAdapter<AttributeValues> {
 
-  private final Gson gson = new Gson();
+  private final Gson gson = new GsonBuilder().disableHtmlEscaping().create();
 
   @Override
   public void write(JsonWriter out, AttributeValues attributeValues) throws IOException {
@@ -129,32 +132,51 @@ private Object deserializeObject(JsonElement json) {
     }
   }
 
+  @VisibleForTesting static final String STRING_ESCAPE_SEQUENCE = "'";
+
   /**
-   * Serializes an object (Label or String) to String A label is converted to a String as it is. A
-   * String is being modified with a delimiter to be easily differentiated from the label when
-   * deserializing.
+   * Serializes an object (Label or String) to String. A label is converted to a String as it is. A
+   * String that looks like a label is escaped so that it can be differentiated from a label when
+   * deserializing, otherwise it is emitted as is.
    *
    * @param obj String or Label
    * @return serialized object
    */
   private String serializeObjToString(Object obj) {
     if (obj instanceof Label) {
-      return ((Label) obj).getUnambiguousCanonicalForm();
+      String labelString = ((Label) obj).getUnambiguousCanonicalForm();
+      Preconditions.checkState(labelString.startsWith("@@"));
+      return labelString;
+    }
+    String string = (String) obj;
+    // Strings that start with "@@" need to be escaped to avoid being interpreted as a label. We
+    // escape by wrapping the string in the escape sequence and strip one layer of this sequence
+    // during deserialization, so strings that happen to already start and end with the escape
+    // sequence also have to be escaped.
+    if (string.startsWith("@@")
+        || (string.startsWith(STRING_ESCAPE_SEQUENCE) && string.endsWith(STRING_ESCAPE_SEQUENCE))) {
+      return STRING_ESCAPE_SEQUENCE + string + STRING_ESCAPE_SEQUENCE;
     }
-    return "--" + obj;
+    return string;
   }
 
   /**
-   * Deserializes a string to either a label or a String depending on the prefix. A string will have
-   * a delimiter at the start, else it is converted to a label.
+   * Deserializes a string to either a label or a String depending on the prefix and presence of the
+   * escape sequence.
    *
    * @param value String to be deserialized
    * @return Object of type String of Label
    */
   private Object deserializeStringToObject(String value) {
-    if (value.startsWith("--")) {
-      return value.substring(2);
+    // A string represents a label if and only if it starts with "@@".
+    if (value.startsWith("@@")) {
+      return Label.parseCanonicalUnchecked(value);
+    }
+    // Strings that start and end with the escape sequence always require one layer to be stripped.
+    if (value.startsWith(STRING_ESCAPE_SEQUENCE) && value.endsWith(STRING_ESCAPE_SEQUENCE)) {
+      return value.substring(
+          STRING_ESCAPE_SEQUENCE.length(), value.length() - STRING_ESCAPE_SEQUENCE.length());
     }
-    return Label.parseCanonicalUnchecked(value);
+    return value;
   }
 }

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileValue.java

@@ -37,7 +37,7 @@
 @GenerateTypeAdapter
 public abstract class BazelLockFileValue implements SkyValue, Postable {
 
-  public static final int LOCK_FILE_VERSION = 2;
+  public static final int LOCK_FILE_VERSION = 3;
 
   @SerializationConstant public static final SkyKey KEY = () -> SkyFunctions.BAZEL_LOCK_FILE;
 

src/test/java/com/google/devtools/build/lib/bazel/bzlmod/AttributeValuesAdapterTest.java

@@ -16,6 +16,7 @@
 package com.google.devtools.build.lib.bazel.bzlmod;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.lib.bazel.bzlmod.AttributeValuesAdapter.STRING_ESCAPE_SEQUENCE;
 
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.testutil.FoundationTestCase;
@@ -43,7 +44,23 @@ public void testAttributeValuesAdapter() throws IOException {
     Label l2 = Label.parseCanonicalUnchecked("@//foo:tar");
     dict.put("Integer", StarlarkInt.of(56));
     dict.put("Boolean", false);
-    dict.put("String", "Hello");
+    dict.put("String", "Hello String");
+    dict.put("StringWithAngleBrackets", "<Hello>");
+    dict.put(
+        "LabelLikeString",
+        StarlarkList.of(Mutability.IMMUTABLE, "@//foo:bar", ":baz", "@@//baz:quz"));
+    dict.put(
+        "StringsWithEscapeSequence",
+        StarlarkList.of(
+            Mutability.IMMUTABLE,
+            "@@//foo:bar" + STRING_ESCAPE_SEQUENCE,
+            STRING_ESCAPE_SEQUENCE + "@@//foo:bar",
+            STRING_ESCAPE_SEQUENCE + "@@//foo:bar" + STRING_ESCAPE_SEQUENCE,
+            STRING_ESCAPE_SEQUENCE
+                + STRING_ESCAPE_SEQUENCE
+                + "@@//foo:bar"
+                + STRING_ESCAPE_SEQUENCE
+                + STRING_ESCAPE_SEQUENCE));
     dict.put("Label", l1);
     dict.put(
         "ListOfInts", StarlarkList.of(Mutability.IMMUTABLE, StarlarkInt.of(1), StarlarkInt.of(2)));
@@ -66,6 +83,10 @@ public void testAttributeValuesAdapter() throws IOException {
       attributeValues = attrAdapter.read(new JsonReader(stringReader));
     }
 
+    // Verify that the JSON string does not contain any escaped angle brackets.
+    assertThat(jsonString).doesNotContain("\\u003c");
+    // Verify that the String "Hello String" is preserved as is, without any additional escaping.
+    assertThat(jsonString).contains(":\"Hello String\"");
     assertThat((Map<?, ?>) attributeValues.attributes()).containsExactlyEntriesIn(builtDict);
   }
 }