[clang][Format] Evaluate FallbackStyle only if needed

[kadircet]

Summary

Currently getStyle() fails immediately if FallbackStyle is not a
predefined style, even when it is not needed. This patch moves that evaluation
and bail-out to fallback-case, enabling getStyle() to succeed in cases where
FallbackStyle is malformed but not needed.

[MyDeveloperDay]

This needs a unit test, I personally don't understand what this is doing or why its needed, if its a bug can you reference a bug in bugzilla that explains the bug a little clearer please

[kadircet]

Sure adding tests, sorry for leaving them out the first time.

To give more insights, format::getStyle() tries to figure out FallbackStyle at the beginning of the function, and errors out if it can't. But in most cases FallbackStyle is not needed, e.g. there's already a .clang-format file in parent directories and StyleName is "file", or StyleName is a predefined/custom one.
This patch changes getStyle()s behaviour to succeed in such cases, even with an invalid FallbackStyle. As for a real world sample you can check https://github.com/clangd/coc-clangd/issues/39#issuecomment-703189067.

[kadircet]

• Add tests.

[curdeius]

Why is fixing (or removing) the fallback style in the .clang-format file not enough??

[kadircet]

In D95538#2527335, @curdeius wrote:

Why is fixing (or removing) the fallback style in the .clang-format file not enough??

sure that would be one way to go, and that's what the user did already.
It feels confusing to me that getStyle() fails when provided with a malformed FallbackStyle. Even if it is not going to be returned. Are there any workflows that benefit from this behaviour?

[curdeius]

I see little value in not checking FallbackStyle (even if it's not used).
However, I do see value in an early warning (error) on an incorrect style that can pop up later if not caught soon enough.

[curdeius]

I return the question, are there any workflows that would benefit from the proposed behaviour? Apart from those that have already a faulty config?

[kadircet]

I see little value in not checking FallbackStyle (even if it's not used).

note that this patch doesn't disable fallbackstyle checking, it is still checked, but not eagerly.

However, I do see value in an early warning (error) on an incorrect style that can pop up later if not caught soon enough.

i don't follow the value proposition here. if user has a valid way to provide "style", the fallbackstyle will never be used (malformed or otherwise).
so in such a case, user is currently going to get a "hard error" preventing them from formatting with the style they provided whenever they have a malformed fallback-style.

what i am trying to say is, "earliness" of the warning/error in such a scenario doesn't seem to benefit any user (at least to me).
since fixing their fallbackstyle won't change anything for formatting of the particular file in question, and they'll still see the error whenever they try to format a file that needs the fallbackstyle.
hence currently the eager validation of a malformed FallbackStyle is *only* preventing a user with a valid Style from formatting their file (it is erroring in all other cases, but there's nothing much to be done for those).

to add, there will never be a file formatted with the incorrect style.

I return the question, are there any workflows that would benefit from the proposed behaviour? Apart from those that have already a faulty config?

that's the exact set of workflows that this change is going to effect(if FallbackStyle is valid, this patch is a no-op). so, no, i don't think there will be any other workflows that can benefit from this change, but it feels like this is a non-empty set of users (while not regressing any users that benefit from current error/warning, as they'll still be surfaced when needed).

[MyDeveloperDay]

For the purposes of not checking a style we are not going to use and because I guess it might make clang-format startup, ever so slightly faster, I think its a reasonable change that likely does no harm.

clang/lib/Format/Format.cpp

llvm::Expected<FormatStyle> getStyle(StringRef StyleName, StringRef FileName,
                                      StringRef FallbackStyleName,
                                      StringRef Code, llvm::vfs::FileSystem *FS,
                                      bool AllowUnknownOptions) {
   if (!FS) {
     FS = llvm::vfs::getRealFileSystem().get();
   }
   FormatStyle Style = getLLVMStyle(guessLanguage(FileName, Code));
 
-  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(
             llvm::MemoryBufferRef(StyleName, "<command-line>"), &Style,
             AllowUnknownOptions))
       return make_string_error("Error parsing -style: " + ec.message());
     return Style;
   }
for (const auto &F : FilesToLookFor) {
         return Style;
       }
     }
   }
   if (!UnsuitableConfigFiles.empty())
     return make_string_error("Configuration file(s) do(es) not support " +
                              getLanguageName(Style.Language) + ": " +
                              UnsuitableConfigFiles);
+
+  FormatStyle FallbackStyle = getNoStyle();
+  if (!getPredefinedStyle(FallbackStyleName, Style.Language, &FallbackStyle))
+    return make_string_error("Invalid fallback style \"" + FallbackStyleName);
   return FallbackStyle;
 }
 
 } // namespace format
 } // namespace clang

clang/unittests/Format/FormatTest.cpp

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);
   ASSERT_EQ(*Style3, getGoogleStyle());
 
-  // Test 4: error on invalid fallback style
+  // Test 4.1: error on invalid fallback style
   auto Style4 = getStyle("file", "a.h", "KungFu", "", &FS);
   ASSERT_FALSE((bool)Style4);
   llvm::consumeError(Style4.takeError());
 
+  // Test 4.2: don't error on invalid fallback style, if it is not going to be
+  // used.
+  Style4 = getStyle("file", "/a/a.h", "KungFu", "", &FS);
+  ASSERT_TRUE((bool)Style4);
+  ASSERT_EQ(*Style4, getLLVMStyle());
+
   // Test 5: error on invalid yaml on command line
   auto Style5 = getStyle("{invalid_key=invalid_value}", "a.h", "LLVM", "", &FS);
   ASSERT_FALSE((bool)Style5);
   llvm::consumeError(Style5.takeError());
 
   // Test 6: error on invalid style
   auto Style6 = getStyle("KungFu", "a.h", "LLVM", "", &FS);
   ASSERT_FALSE((bool)Style6);