clang-format: fix fallback style set to "none" not formatting

[amaiorano]

Summary

This change fixes the fact that fallback style set to "none" should not format. Without this change, fallback style "none" ends up applying LLVM formatting.

[amaiorano]

lib/Format/Format.cpp

1906 ↗

(On Diff #84795)

Technically, we don't need to initialize FallbackStyle to getNoStyle() here as the call to getPredefinedStyle just below will initialize it. Let me know what you would prefer here.

test/Format/style-on-command-line.cpp

15 ↗

(On Diff #84795)

Tests no config file found, WebKit fallback style is applied

16 ↗

(On Diff #84795)

Tests no config file found, no fallback style, LLVM style is applied

17 ↗

(On Diff #84795)

Tests no config file found, fallback style set to "none", no formatting is applied

[djasper]

lib/Format/Format.cpp

1906 ↗

(On Diff #84795)

This seems fine.

test/Format/style-on-command-line.cpp

15 ↗

(On Diff #84795)

Please put these into comments (as long as they don't start with "RUN", they should be just left alone as normal comments.

cfe/trunk/lib/Format/Format.cpp

if (FileName.endswith_lower(".proto") ||
       FileName.endswith_lower(".protodevel"))
     return FormatStyle::LK_Proto;
   if (FileName.endswith_lower(".td"))
     return FormatStyle::LK_TableGen;
   return FormatStyle::LK_Cpp;
 }
 
 llvm::Expected<FormatStyle> getStyle(StringRef StyleName, StringRef FileName,
-                                     StringRef FallbackStyle, StringRef Code,
-                                     vfs::FileSystem *FS) {
+                                     StringRef FallbackStyleName,
+                                     StringRef Code, vfs::FileSystem *FS) {
   if (!FS) {
     FS = vfs::getRealFileSystem().get();
   }
   FormatStyle Style = getLLVMStyle();
   Style.Language = getLanguageByFileName(FileName);
 
   // This is a very crude detection of whether a header contains ObjC code that
   // should be improved over time and probably be done on tokens, not one the
   // bare content of the file.
   if (Style.Language == FormatStyle::LK_Cpp && FileName.endswith(".h") &&
       (Code.contains("\n- (") || Code.contains("\n+ (")))
     Style.Language = FormatStyle::LK_ObjC;
 
-  // FIXME: If FallbackStyle is explicitly "none", format is disabled.
-  if (!getPredefinedStyle(FallbackStyle, Style.Language, &Style))
-    return make_string_error("Invalid fallback style \"" + FallbackStyle.str());
+  FormatStyle FallbackStyle = getNoStyle();
+  if (!getPredefinedStyle(FallbackStyleName, Style.Language, &FallbackStyle))
+    return make_string_error("Invalid fallback style \"" + FallbackStyleName);
 
   if (StyleName.startswith("{")) {
     // Parse YAML/JSON style from the command line.
     if (std::error_code ec = parseConfiguration(StyleName, &Style))
       return make_string_error("Error parsing -style: " + ec.message());
     return Style;
   }
 
if (FoundConfigFile) {
       DEBUG(llvm::dbgs() << "Using configuration file " << ConfigFile << "\n");
       return Style;
     }
   }
   if (!UnsuitableConfigFiles.empty())
     return make_string_error("Configuration file(s) do(es) not support " +
                              getLanguageName(Style.Language) + ": " +
                              UnsuitableConfigFiles);
-  return Style;
+  return FallbackStyle;
 }
 
 } // namespace format
 } // namespace clang

cfe/trunk/test/Format/style-on-command-line.cpp

 // RUN: clang-format -style="{BasedOnStyle: Google, IndentWidth: 8}" %s | FileCheck -strict-whitespace -check-prefix=CHECK1 %s
 // RUN: clang-format -style="{BasedOnStyle: LLVM, IndentWidth: 7}" %s | FileCheck -strict-whitespace -check-prefix=CHECK2 %s
 // RUN: not clang-format -style="{BasedOnStyle: invalid, IndentWidth: 7}" -fallback-style=LLVM %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK3 %s
 // RUN: not clang-format -style="{lsjd}" %s -fallback-style=LLVM 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK4 %s
 // RUN: printf "BasedOnStyle: google\nIndentWidth: 5\n" > %T/.clang-format
 // RUN: clang-format -style=file -assume-filename=%T/foo.cpp < %s | FileCheck -strict-whitespace -check-prefix=CHECK5 %s
 // RUN: printf "\n" > %T/.clang-format
 // RUN: not clang-format -style=file -fallback-style=webkit -assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK6 %s
 // RUN: rm %T/.clang-format
 // RUN: printf "BasedOnStyle: google\nIndentWidth: 6\n" > %T/_clang-format
 // RUN: clang-format -style=file -assume-filename=%T/foo.cpp < %s | FileCheck -strict-whitespace -check-prefix=CHECK7 %s
 // RUN: clang-format -style="{BasedOnStyle: LLVM, PointerBindsToType: true}" %s | FileCheck -strict-whitespace -check-prefix=CHECK8 %s
 // RUN: clang-format -style="{BasedOnStyle: WebKit, PointerBindsToType: false}" %s | FileCheck -strict-whitespace -check-prefix=CHECK9 %s
+
+// Fallback style tests
+// RUN: rm %T/_clang-format
+// Test no config file found, WebKit fallback style is applied
+// RUN: clang-format -style=file -fallback-style=WebKit -assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK10 %s
+// Test no config file and no fallback style, LLVM style is applied
+// RUN: clang-format -style=file -assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK11 %s
+// Test no config file and fallback style "none", no formatting is applied
+// RUN: clang-format -style=file -fallback-style=none -assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK12 %s
+// Test config file with no based style, and fallback style "none", formatting is applied
+// RUN: printf "IndentWidth: 6\n" > %T/_clang-format
+// RUN: clang-format -style=file -fallback-style=none -assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK13 %s
+// Test yaml with no based style, and fallback style "none", LLVM formatting applied
+// RUN: clang-format -style="{IndentWidth: 7}" -fallback-style=none %s | FileCheck -strict-whitespace -check-prefix=CHECK14 %s
+
 void f() {
 // CHECK1: {{^        int\* i;$}}
 // CHECK2: {{^       int \*i;$}}
 // CHECK3: Unknown value for BasedOnStyle: invalid
 // CHECK3: Error parsing -style: {{I|i}}nvalid argument
 // CHECK4: Error parsing -style: {{I|i}}nvalid argument
 // CHECK5: {{^     int\* i;$}}
 // CHECK6: {{^Error reading .*\.clang-format: (I|i)nvalid argument}}
 // CHECK7: {{^      int\* i;$}}
 // CHECK8: {{^  int\* i;$}}
 // CHECK9: {{^    int \*i;$}}
+// CHECK10: {{^    int\* i;$}}
+// CHECK11: {{^  int \*i;$}}
+// CHECK12: {{^int\*i;$}}
+// CHECK13: {{^      int \*i;$}}
+// CHECK14: {{^       int \*i;$}}
 int*i;
 int j;
 }
 
 // On Windows, the 'rm' commands fail when the previous process is still alive.
 // This happens enough to make the test useless.
 // REQUIRES: shell

cfe/trunk/unittests/Format/FormatTest.cpp

ASSERT_TRUE(
       FS.addFile("/a/.clang-format", 0,
                  llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: LLVM")));
   ASSERT_TRUE(
       FS.addFile("/a/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int i;")));
   auto Style1 = getStyle("file", "/a/.clang-format", "Google", "", &FS);
   ASSERT_TRUE((bool)Style1);
   ASSERT_EQ(*Style1, getLLVMStyle());
 
-  // Test 2: fallback to default.
+  // Test 2.1: fallback to default.
   ASSERT_TRUE(
       FS.addFile("/b/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int i;")));
   auto Style2 = getStyle("file", "/b/test.cpp", "Mozilla", "", &FS);
   ASSERT_TRUE((bool)Style2);
   ASSERT_EQ(*Style2, getMozillaStyle());
 
+  // Test 2.2: no format on 'none' fallback style.
+  Style2 = getStyle("file", "/b/test.cpp", "none", "", &FS);
+  ASSERT_TRUE((bool)Style2);
+  ASSERT_EQ(*Style2, getNoStyle());
+
+  // Test 2.3: format if config is found with no based style while fallback is
+  // 'none'.
+  ASSERT_TRUE(FS.addFile("/b/.clang-format", 0,
+                         llvm::MemoryBuffer::getMemBuffer("IndentWidth: 2")));
+  Style2 = getStyle("file", "/b/test.cpp", "none", "", &FS);
+  ASSERT_TRUE((bool)Style2);
+  ASSERT_EQ(*Style2, getLLVMStyle());
+
+  // Test 2.4: format if yaml with no based style, while fallback is 'none'.
+  Style2 = getStyle("{}", "a.h", "none", "", &FS);
+  ASSERT_TRUE((bool)Style2);
+  ASSERT_EQ(*Style2, getLLVMStyle());
+
   // Test 3: format file in parent directory.
   ASSERT_TRUE(
       FS.addFile("/c/.clang-format", 0,
                  llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
   ASSERT_TRUE(FS.addFile("/c/sub/sub/sub/test.cpp", 0,
                          llvm::MemoryBuffer::getMemBuffer("int i;")));
   auto Style3 = getStyle("file", "/c/sub/sub/sub/test.cpp", "LLVM", "", &FS);
   ASSERT_TRUE((bool)Style3);