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

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

Tests no config file found, WebKit fallback style is applied

16

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

17

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

[djasper]

lib/Format/Format.cpp

1906

This seems fine.

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

15

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

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

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
+// RUN: rm %T/_clang-format
+// RUN: clang-format -style=file -fallback-style=WebKit -assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK10 %s
+// RUN: clang-format -style=file -assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK11 %s
+// RUN: clang-format -style=file -fallback-style=none -assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK12 %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;$}}
 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

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 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);