[clang-format] Remove spurious JSON binding when DisableFormat = true

[Andrew-William-Smith]

Summary

Relevant issue: https://github.com/llvm/llvm-project/issues/52705

When the DisableFormat option of clang-format is set to true and a JSON file is formatted, the ephemeral variable binding that is added to the top-level object is not removed from the formatted file. For example, this JSON:

{
  "key": "value"
}

Is reformatted to:

x = {
  "key": "value"
}

Which is not valid JSON syntax. This fix avoids the addition of this binding when DisableFormat is set to true, ensuring that it cannot be left behind when formatting is disabled.

[owenpan]

Can you add a test case?

[MyDeveloperDay]

Thank you for the patch, this looks good but we do need a unit test in clang/unittest/Format/FormatTestJson.cpp you can then build them with ninja FormatTests, if you need help let me know

[MyDeveloperDay]

The following will show the issue

TEST_F(FormatTestJson, DisableJsonFormat) {
  FormatStyle Style = getLLVMStyle(FormatStyle::LK_Json);
  verifyFormat("{}", Style);
  verifyFormat("{\n"
               "  \"name\": 1\n"
               "}",
               Style);
  Style.DisableFormat = true;
  verifyFormat("{}", Style);
  verifyFormat("{\n"
               "  \"name\": 1\n"
               "}",
               Style);
}

[Andrew-William-Smith]

Added a test with formatting disabled. I had to port the updated logic from ClangFormat.cpp into FormatTestJson::format to show the desired effect; I also couldn't call verifyFormat as-is with formatting disabled since the second assertion involving test::messUp would modify the formatting of the input JSON with no way to format it back. I've added a new function FormatTestJson::verifyFormatStable that only checks that formatting is stable for this scenario.

[MyDeveloperDay]

I think your previous change is dropped from the review, but the test in combination with your original change were good.

[MyDeveloperDay]

looks like you reverted the change

https://reviews.llvm.org/D115769?vs=394414&id=394582#toc

[Andrew-William-Smith]

Apologies, I undid the actual changes to ClangFormat.cpp. Still learning how to use arc...

[MyDeveloperDay]

LGTM, thank you for the patch

Do you need help landing this? if so we'll need your name and email address

[Andrew-William-Smith]

I'm going to assume that I don't have commit access to main, so that would be much appreciated.

• Name: Andrew Smith
• Email: aws <at> awsmith <dot> us

clang/tools/clang-format/ClangFormat.cpp

else
       FormatStyle->SortIncludes = FormatStyle::SI_Never;
   }
   unsigned CursorPosition = Cursor;
   Replacements Replaces = sortIncludes(*FormatStyle, Code->getBuffer(), Ranges,
                                        AssumedFileName, &CursorPosition);
 
   // To format JSON insert a variable to trick the code into thinking its
   // JavaScript.
-  if (FormatStyle->isJson()) {
+  if (FormatStyle->isJson() && !FormatStyle->DisableFormat) {
     auto Err = Replaces.add(tooling::Replacement(
         tooling::Replacement(AssumedFileName, 0, 0, "x = ")));
     if (Err) {
       llvm::errs() << "Bad Json variable insertion\n";
     }
   }
 
   auto ChangedCode = tooling::applyAllReplacements(Code->getBuffer(), Replaces);

clang/unittests/Format/FormatTestJson.cpp

static std::string format(llvm::StringRef Code, unsigned Offset,
                             unsigned Length, const FormatStyle &Style) {
     LLVM_DEBUG(llvm::errs() << "---\n");
     LLVM_DEBUG(llvm::errs() << Code << "\n\n");
 
     tooling::Replacements Replaces;
 
     // Mock up what ClangFormat.cpp will do for JSON by adding a variable
     // to trick JSON into being JavaScript
-    if (Style.isJson()) {
+    if (Style.isJson() && !Style.DisableFormat) {
       auto Err = Replaces.add(
           tooling::Replacement(tooling::Replacement("", 0, 0, "x = ")));
       if (Err) {
         llvm::errs() << "Bad Json variable insertion\n";
       }
     }
     auto ChangedCode = applyAllReplacements(Code, Replaces);
     if (!ChangedCode) {
protected:
   }
 
   static FormatStyle getStyleWithColumns(unsigned ColumnLimit) {
     FormatStyle Style = getLLVMStyle(FormatStyle::LK_Json);
     Style.ColumnLimit = ColumnLimit;
     return Style;
   }
 
+  static void verifyFormatStable(llvm::StringRef Code,
+                                 const FormatStyle &Style) {
+    EXPECT_EQ(Code.str(), format(Code, Style)) << "Expected code is not stable";
+  }
+
   static void
   verifyFormat(llvm::StringRef Code,
                const FormatStyle &Style = getLLVMStyle(FormatStyle::LK_Json)) {
-    EXPECT_EQ(Code.str(), format(Code, Style)) << "Expected code is not stable";
+    verifyFormatStable(Code, Style);
     EXPECT_EQ(Code.str(), format(test::messUp(Code), Style));
   }
 };
 
 TEST_F(FormatTestJson, JsonRecord) {
   verifyFormat("{}");
   verifyFormat("{\n"
                "  \"name\": 1\n"
verifyFormat("[\n"
                "\"foooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo"
                "oooooooooooooooooooooooooo\"\n"
                "    },\n"
                "    {}\n"
                "]",
                Style);
 }
 
+TEST_F(FormatTestJson, DisableJsonFormat) {
+  FormatStyle Style = getLLVMStyle(FormatStyle::LK_Json);
+  verifyFormatStable("{}", Style);
+  verifyFormatStable("{\n"
+                     "  \"name\": 1\n"
+                     "}",
+                     Style);
+
+  // Since we have to disable formatting to run this test, we shall refrain from
+  // calling test::messUp lest we change the unformatted code and cannot format
+  // it back to how it started.
+  Style.DisableFormat = true;
+  verifyFormatStable("{}", Style);
+  verifyFormatStable("{\n"
+                     "  \"name\": 1\n"
+                     "}",
+                     Style);
+}
+
 } // namespace format
 } // end namespace clang