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

[via cfe-commits]

Author: Andrew Smith
Date: 2021-12-15T23:09:28Z
New Revision: 63a565768e8fdbb3260a88f584aa2c11a58fea93

URL: https://github.com/llvm/llvm-project/commit/63a565768e8fdbb3260a88f584aa2c11a58fea93
DIFF: https://github.com/llvm/llvm-project/commit/63a565768e8fdbb3260a88f584aa2c11a58fea93.diff

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

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.

Reviewed By: MyDeveloperDay, HazardyKnusperkeks

Differential Revision: https://reviews.llvm.org/D115769

Fixes #52705

Added: 
    

Modified: 
    clang/tools/clang-format/ClangFormat.cpp
    clang/unittests/Format/FormatTestJson.cpp

Removed: 
    


################################################################################
diff  --git a/clang/tools/clang-format/ClangFormat.cpp b/clang/tools/clang-format/ClangFormat.cpp
index 56dc628869a4d..893c17d917082 100644
--- a/clang/tools/clang-format/ClangFormat.cpp
+++ b/clang/tools/clang-format/ClangFormat.cpp
@@ -460,7 +460,7 @@ static bool format(StringRef FileName) {
 
   // 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) {

diff  --git a/clang/unittests/Format/FormatTestJson.cpp b/clang/unittests/Format/FormatTestJson.cpp
index a1680d80b92b2..14a48182f9dfe 100644
--- a/clang/unittests/Format/FormatTestJson.cpp
+++ b/clang/unittests/Format/FormatTestJson.cpp
@@ -27,7 +27,7 @@ class FormatTestJson : public ::testing::Test {
 
     // 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) {
@@ -60,10 +60,15 @@ class FormatTestJson : public ::testing::Test {
     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));
   }
 };
@@ -193,5 +198,24 @@ TEST_F(FormatTestJson, JsonNoStringSplit) {
                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