Make GetStyle return Expected<FormatStyle> instead of FormatStyle

[amaiorano]

Summary

Change the contract of GetStyle so that it returns an error when an error occurs (i.e. when it writes to stderr), and only returns the fallback style when it can't find a configuration file.

[amaiorano]

This change works, and passes all tests; however, I have a bunch of questions, which you'll find in the diffs. I look forward to your feedback. Thanks!

lib/Format/Format.cpp

1890

Since we won't always return a FormatStyle, we no longer construct one here. Rather, instances are created in local scopes below when required.

1901

I am unsure whether I should be returning these error strings at all. I say this because some of the functions called in this one will output to errs(), which means the caller doesn't get the full picture of what went wrong.

Maybe it's better to keep the original code that outputs to errs(), and just return an error code instead. Thoughts?

1911

I must remember to set the Language each time. I could do this right after declaring the local variable (line 1907). (NOTE: will fix formatting - ah the irony!).

1945

Better variable name IMHO. Condition checks both that it exists and is a regular file.

1984

See the TODO comment above (which will be removed obviously). Is it an error if we find no suitable config for the input language? If that happens, should the fallback style be returned?

lib/Tooling/Refactoring.cpp

87

As I wrote above, by returning the string, the caller has the burden of outputting the error. More importantly, getStyle may already have written other errors to errs(), so I'm not sure this makes sense.

If we go with only returning error code, I suppose I'd use llvm::ErrorOr<FormatStyle> as the return type.

unittests/Format/FormatTest.cpp

10948

Good way to validate Expected<T>?

10967

Below, I added tests for all the error cases. Only thing is that they may output to errs(); is that okay? Will that break test bots?

10985

Test 7 below: there's an error case in getStyle where it fails to open the file. Not sure how to make it fail or whether it's even worth testing. (See Format.cpp line 1961)

[amaiorano]

unittests/Format/FormatTestObjC.cpp

72

In these tests, I'm assuming getStyle returns a valid FormatSyle. I could add the same types of validation as in the FormatStyle.GetStyleOfFile tests.

[amaiorano]

One more thing I forgot to mention is that this change comes from a discussion we had on this other change: https://reviews.llvm.org/D27440 where @djasper agreed that "fallback-style should only be used when there is no .clang-format file. If we find one, and it doesn't parse correctly, we should neither use the fallback style nor scan in higher-level directories (not sure whether we currently do that).".

[ioeric]

include/clang/Format/Format.h

862

Is this still true?

lib/Format/Format.cpp

424

Why do you need a template if you only use this function with one argument?

1890

I'd probably keep the default style so that we don't need to set Language repeatedly below.

1900

Nit: prefer no curly brace around one liners. Same below.

1901

I think returning an Expected is the right approach. I think we should consider using customized format-relayed error codes (like that in the tooling::Replacements library) to provide richer information. For example, you could use customized error codes instead of llvm::inconvertibleErrorCode when constructing a StringError to provide additional information besides the error message. Other interfaces in the format library can potentially benefit from codes as well (e.g. ParseError can be merged).

lib/Tooling/Refactoring.cpp

86

Is this indentation intended?

87

I don't think getStyle should write to errs() if it returns an Error.

[amaiorano]

Thank you, @ioeric for your feedback!

include/clang/Format/Format.h

862

No, it's not true, and I intend to update this comment. Thanks for pointing it out :)

lib/Format/Format.cpp

424

In the end, I didn't need it. But since make_error accepts variable args, I figured this was more flexible. I can simplify it in the end, I only pass a single param.

1890

The thing is, it made a lot of sense to have a default constructed FormatStyle before when we were sure to always return it. Since now we only return a Style under 2 specific conditions (found a config file, or did not return the FallbackStyle), otherwise return an error, I feel like it's more clear this way. But if you firmly disagree, I can go back.

To make returning the Style with current Language more clear, I could use a function that ties the two together and returns it (perhaps a local lambda), like StyleWithLanguage(Style, Language).

1900

Will do.

1901

I agree with you, returning more specific error codes would be useful. However, I'm thinking we go at it incrementally. The point of this change is to have getStyle return when an error occurs so that we return non-zero from main, and more importantly, not use the fallback style when this happens.

In that respect, what I'm wondering is whether I should just leave the errs() output in getStyle, and simply return an error code, or keep what I've done (returning StringError) as a stepping stone in the right direction - that is, eventually returning customized format-relayed error codes, as you say.

lib/Tooling/Refactoring.cpp

86

No. I will definitely do a formatting pass. I'm using Visual Studio with the clang-format extension, which works alright, but Visual Studio fights against it with its own formatting. Indeed, the reason I even started contributing to clang-format was because I wanted to improve the VS extension. I've already pushed a few changes to it, and intend to do a little more to make the experience simpler. In any case, my next diff upload will have proper formatting everywhere.

[ioeric]

lib/Format/Format.cpp

1890

I'd go with the old way with some comments, which should be clear enough.

1901

I think the interface should no longer write to errs() if we want to introduce error handling (either with error codes or llvm::Error).

StringError does not really collide with error codes AFAICT - it can also carry error codes which you would have return directly. And even if you just return error codes, you would need some sort of customized error codes right?

[amaiorano]

lib/Format/Format.cpp

1984

@ioeric Do you have any thoughts on my question here? Say the user specified "-file" and a fallback style, and we find files but they are not suitable (for a different language), do we use the fallback style, since it's as if we found no config file. If so, then we wouldn't consider this an error, and therefore would not print nor return the message that we see here ("Configuration file(s) do(es) not support...").

The fact that the original code output to errs() here leads me to believe that this should be considered an error condition, in which case I should keep my change - that is, return an error here and _not_ return the fallback style.

[ioeric]

lib/Format/Format.cpp

1984

I don't have strong opinion here, but I am inclined to return an error.

[amaiorano]

Updated with the changes @ioeric suggested.

[amaiorano]

Hello everyone, so after a few more tests, I've uncovered a bug and perhaps a different meaning for fallback style. First, the bug: if you set fallback style to "none", clang-format will perform no replacements. This happens because getStyle will first initialize its local Style variable to LLVM style, and then because a fallback style is set, will then set it to the "none" style, will ends up setting Style.DisableFormatting to true. After that, when we parse YAML (either from Style arg or a config file), we use the Style variable as the "template" for fields that haven't been set. In this case, the "none" fallback style causes DisableFormatting to remain true, so no formatting will take place.

As it happens, my first diff patch uploaded here fixed this issue by accident. Instead of reusing the same local Style variable, I declared one for each case where we'd need to parse. The fallback style case would use its own variable, FallbackStyle, which would not be used as the template style when parsing the YAML config.

What's interesting is that the way the code is originally written allows you to use fallback style as a way to set the "base" configuration for which the subsequently parsed YAML overlays. For example, if I don't set fallback style, the assumed base style is "LLVM", and any YAML parsed modifies this LLVM base style. But if I pass a fallback style of "Mozilla", then this becomes the base style over which the YAML overlays.

So to my mind, we have 2 approaches to fix the "none" style bug:

1. Go with a similar approach to what I did originally; that is, we always assume LLVM as the base style, and make sure that the fallback style is not used as the base style, but rather only as the style to return if none is found. I think this is what FallbackStyle was originally intended for.

2. Allow fallback style to maintain its current meaning - that is, as a way to set the base style when "style" is "file" or YAML. In this case, I believe the right thing is to treat FallbackStyle set to "none" as though no fallback style were passed in at all. Concretely, we might want t to modify getPredefinedStyle to return LLVM style when "none" is passed in, instead of what it does now. I personally think this is more confusing, and also introduces more risk.

Let me know what you think. If we go with option 1, I could fold the fix into this change.

lib/Format/Format.cpp

1903

Going over my diff, I realize that I didn't revert this local FormatStyle instance to make use of the top-level declared "FormatStyle Style" local variable. I will revert this one too.

[amaiorano]

lib/Format/Format.cpp

1903

I wrote the above comment before realizing there was a bug when fallback style is "none". As I explained in my long comment, we may actually want to keep this as a separate variable to fix the bug and ensure fallback style is only ever returned when no config is found, which is exactly what my change currently does.

[ioeric]

In D28081#633103, @amaiorano wrote:

Hello everyone, so after a few more tests, I've uncovered a bug and perhaps a different meaning for fallback style. First, the bug: if you set fallback style to "none", clang-format will perform no replacements. This happens because getStyle will first initialize its local Style variable to LLVM style, and then because a fallback style is set, will then set it to the "none" style, will ends up setting Style.DisableFormatting to true. After that, when we parse YAML (either from Style arg or a config file), we use the Style variable as the "template" for fields that haven't been set. In this case, the "none" fallback style causes DisableFormatting to remain true, so no formatting will take place.

As it happens, my first diff patch uploaded here fixed this issue by accident. Instead of reusing the same local Style variable, I declared one for each case where we'd need to parse. The fallback style case would use its own variable, FallbackStyle, which would not be used as the template style when parsing the YAML config.

What's interesting is that the way the code is originally written allows you to use fallback style as a way to set the "base" configuration for which the subsequently parsed YAML overlays. For example, if I don't set fallback style, the assumed base style is "LLVM", and any YAML parsed modifies this LLVM base style. But if I pass a fallback style of "Mozilla", then this becomes the base style over which the YAML overlays.

So to my mind, we have 2 approaches to fix the "none" style bug:

1. Go with a similar approach to what I did originally; that is, we always assume LLVM as the base style, and make sure that the fallback style is not used as the base style, but rather only as the style to return if none is found. I think this is what FallbackStyle was originally intended for.

2. Allow fallback style to maintain its current meaning - that is, as a way to set the base style when "style" is "file" or YAML. In this case, I believe the right thing is to treat FallbackStyle set to "none" as though no fallback style were passed in at all. Concretely, we might want t to modify getPredefinedStyle to return LLVM style when "none" is passed in, instead of what it does now. I personally think this is more confusing, and also introduces more risk.

Let me know what you think. If we go with option 1, I could fold the fix into this change.

This is a good YAQ, which IMO should be tackled in a separate patch. In this patch though, it might be easier to proceed by keeping the original behavior and leaving a FIXME. In general, reviewers like smaller patches with single purpose :)

[amaiorano]

Reverted the FallbackStyle code and added a FIXME as @ioeric suggested. I'll fix the fallback style "none" bug in a separate change.

[ioeric]

Some nits. Some is almost good :)

BTW, do you have clang-tools-extra in your source tree? There are also some references in the subtree to the changed interface. It would be nice if you could also fix them in a separate patch and commit these two patches together (I mean, within a short period of time) so that you wouldn't break build bots.

References should be found in these files:

extra/change-namespace/ChangeNamespace.cpp
extra/clang-move/ClangMove.cpp
extra/include-fixer/tool/ClangIncludeFixer.cpp	 
extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
extra/clang-tidy/ClangTidy.cpp

Thanks!

lib/Format/Format.cpp

424

Maybe make this inline?

1904

I'd state the problem with a specific solution only if I am sure it is the best solution.

1960–1961

Redundant braces. Same below.

lib/Tooling/Refactoring.cpp

86–92

There is a NewReplacements below which is also llvm::Expected<T>. FormatStyleOrError is a fine name, but to be we have been naming Expected types without OrError postfix, so I'd go without OrError postfix for consistency append OrError postfix to other expected variables. Personally, I find expected variables without OrError postfix easier to understand, especially in error checking. For example, if (!FormatStyleOrError) is a bit awkward to read while if (!FormatStyle) is more straight-forward IMO.

Same for other changes.

unittests/Format/FormatTest.cpp

10972

This is a bit strange... Just llvm::consumeError(Style.takeError()) if you don't bother to check the error message, e.g. with llvm::StringRef::starswith or RE match.

unittests/Format/FormatTestObjC.cpp

72

Please add proper checking as above for returned values.

[ioeric]

Oops, sorry about the typo. I mean, code is almost good! :)

[amaiorano]

In D28081#633521, @ioeric wrote:

Some nits. Some is almost good :)

BTW, do you have clang-tools-extra in your source tree? There are also some references in the subtree to the changed interface. It would be nice if you could also fix them in a separate patch and commit these two patches together (I mean, within a short period of time) so that you wouldn't break build bots.

References should be found in these files:

extra/change-namespace/ChangeNamespace.cpp
extra/clang-move/ClangMove.cpp
extra/include-fixer/tool/ClangIncludeFixer.cpp	 
extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
extra/clang-tidy/ClangTidy.cpp

Thanks!

I'll grab clang-tools-extras and make the second patch as you suggest. Btw, can you explain how I would avoid breaking build bots? I assume you mean that clang-tools-extras gets built separately against some version of clang, which gets auto-updated. When would I know the right time to push the second patch through?

Also, I assume I'd have to get this second patch approved before ever pushing the first, right?

lib/Format/Format.cpp

424

Yes.

1904

Good point, will remove the solution.

lib/Tooling/Refactoring.cpp

86–92

Agreed. For consistency, would you prefer I also use 'auto' for the return type rather than llvm::Expected<format::FormatStyle> as is done for NewReplacements?

unittests/Format/FormatTest.cpp

10972

Yeah, I'll go with the consumeError.

unittests/Format/FormatTestObjC.cpp

72

Hmm, so I could replace the Style member of the fixture class with Expected<FormatStyle>, and then change all "Style." with "Style->" in the rest of the test file, or only in this specific test, I could store the result in a local Expected<FormatStyle>, check that it's valid, and then assign to Style. The latter is simpler; only question I have is how to name the local variable - can I go with StyleOrError? Style2?

[amaiorano]

unittests/Format/FormatTestObjC.cpp

72

Another option here is to make this a non-fixture TEST and just declare a local Expected<FormatStyle> Style for this specific test, which would work fine.

[amaiorano]

More changes as suggested by @ioeric. I asked a question earlier about clang-tools-extras and understanding how not to break the build bots. If you can shed some light on that, I'd appreciate it, thanks :)

[ioeric]

The patch LGTM now. I'll accept both this and the one for clang-tool-extra when it is ready.

Regarding builbots, we have bots that continually run builds/tests (http://lab.llvm.org:8011/). Many buildbots test llvm and clang as well as clang-tools-extra (e.g. with ninja check-all) at some revision. Also note that although llvm/clang and clang-tools-extra are different repos, they do share the same revision sequence. So if clang-tools-extra is in a inconsistent state, many buildbots can fail and affect llvm/clang builds. Unfortunately, there is no atomic way to commit two revisions to two repositories, so we just commit them quickly one after another so that we do less damage. Do you have commit access to LLVM btw?

lib/Format/Format.cpp

1903

maybe "format is disabled" is clearer?

lib/Tooling/Refactoring.cpp

86–92

auto is fine.

unittests/Format/FormatTestObjC.cpp

72

Yeah, making this non-fixture sounds like a better idea. StyleOrError is fine here.

[amaiorano]

Minor comment change, turned the ObjC test into a non-fixture test, and renamed FormatStyleOrError to FormatStyle in format function.

[amaiorano]

In D28081#633786, @ioeric wrote:

The patch LGTM now. I'll accept both this and the one for clang-tool-extra when it is ready.

Regarding builbots, we have bots that continually run builds/tests (http://lab.llvm.org:8011/). Many buildbots test llvm and clang as well as clang-tools-extra (e.g. with ninja check-all) at some revision. Also note that although llvm/clang and clang-tools-extra are different repos, they do share the same revision sequence. So if clang-tools-extra is in a inconsistent state, many buildbots can fail and affect llvm/clang builds. Unfortunately, there is no atomic way to commit two revisions to two repositories, so we just commit them quickly one after another so that we do less damage. Do you have commit access to LLVM btw?

I do have commit access. I'll get to work on the clang-tools-extras and open a new review for it once it's ready. Thanks.

[amaiorano]

@ioeric I think you removed cfe-commits as a reviewer, then added it back, but it didn't work. Should I re-add?

EDIT: never mind, just realized you're supposed to add cfe-commits as subscriber, not reviewer.

[alexander-shaposhnikov]

lib/Format/Format.cpp

424

regarding this function and some other helper functions in this file:

1. why aren't they static ?

(if this stuff is not used outside of this .cpp - but yeah, i assume i'm missing smth)

2. http://llvm.org/docs/CodingStandards.html naming conventions are

"Function names should be verb phrases (as they represent actions), and command-like function should be imperative. The name should be camel case, and start with a lower case letter (e.g. openFile() or isFoo())"

[amaiorano]

Fixed Format/style-on-command-line.cpp test to match new expected output.

[amaiorano]

lib/Format/Format.cpp

424

1. IMHO I think inline is a valid substitute for static. Even though inline functions have external linkage by default, in practice they get inlined and not seen by the linker, or handled properly by the linker (it picks one).

2. You're right, but the reason I went with make_error_code was because I'm wrapping a call to llvm::make_error, which already breaks the camel case convention. But I have no problem renaming this makeErrorCode, just let me know.

[amaiorano]

Rebased changes on latest, no functional change in this diff.

[ioeric]

Lgtm. Thanks!

include/clang/Format/Format.h

 /// == "file".
 /// \param[in] FallbackStyle The name of a predefined style used to fallback to
 /// in case the style can't be determined from \p StyleName.
 /// \param[in] Code The actual code to be formatted. Used to determine the
 /// language if the filename isn't sufficient.
 /// \param[in] FS The underlying file system, in which the file resides. By
 /// default, the file system is the real file system.
 ///
 /// \returns FormatStyle as specified by ``StyleName``. If no style could be
 /// determined, the default is LLVM Style (see ``getLLVMStyle()``).
-FormatStyle getStyle(StringRef StyleName, StringRef FileName,
-                     StringRef FallbackStyle, StringRef Code = "",
+llvm::Expected<FormatStyle> getStyle(StringRef StyleName, StringRef FileName,
+                                     StringRef FallbackStyle,
+                                     StringRef Code = "",
                                      vfs::FileSystem *FS = nullptr);
 
 // \brief Returns a string representation of ``Language``.
 inline StringRef getLanguageName(FormatStyle::LanguageKind Language) {
   switch (Language) {
   case FormatStyle::LK_Cpp:
     return "C++";
   case FormatStyle::LK_Java:
     return "Java";

lib/Format/Format.cpp

 const std::error_category &getParseCategory() {
   static ParseErrorCategory C;
   return C;
 }
 std::error_code make_error_code(ParseError e) {
   return std::error_code(static_cast<int>(e), getParseCategory());
 }
 
+template <typename... ArgTs> llvm::Error make_string_error(ArgTs &&... Args) {
+  return llvm::make_error<llvm::StringError>(std::forward<ArgTs>(Args)...,
+                                             llvm::inconvertibleErrorCode());
+}
+
 const char *ParseErrorCategory::name() const noexcept {
   return "clang-format.parse_error";
 }
 
 std::string ParseErrorCategory::message(int EV) const {
   switch (static_cast<ParseError>(EV)) {
   case ParseError::Success:
     return "Success";
static FormatStyle::LanguageKind getLanguageByFileName(StringRef FileName) {
   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;
 }
 
-FormatStyle getStyle(StringRef StyleName, StringRef FileName,
-                     StringRef FallbackStyle, StringRef Code,
-                     vfs::FileSystem *FS) {
+llvm::Expected<FormatStyle> getStyle(StringRef StyleName, StringRef FileName,
+                                     StringRef FallbackStyleName,
+                                     StringRef Code, vfs::FileSystem *FS) {
   if (!FS) {
     FS = vfs::getRealFileSystem().get();
   }
-  FormatStyle Style = getLLVMStyle();
-  Style.Language = getLanguageByFileName(FileName);
+  FormatStyle::LanguageKind 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") &&
+  if (Language == FormatStyle::LK_Cpp && FileName.endswith(".h") &&
       (Code.contains("\n- (") || Code.contains("\n+ (")))
-    Style.Language = FormatStyle::LK_ObjC;
+    Language = FormatStyle::LK_ObjC;
 
-  if (!getPredefinedStyle(FallbackStyle, Style.Language, &Style)) {
-    llvm::errs() << "Invalid fallback style \"" << FallbackStyle
-                 << "\" using LLVM style\n";
-    return Style;
+  FormatStyle FallbackStyle = getNoStyle();
+  if (!getPredefinedStyle(FallbackStyleName, Language, &FallbackStyle)) {
+    return make_string_error("Invalid fallback style \"" +
+                             FallbackStyleName.str());
   }
 
   if (StyleName.startswith("{")) {
     // Parse YAML/JSON style from the command line.
+    FormatStyle Style;
     if (std::error_code ec = parseConfiguration(StyleName, &Style)) {
-      llvm::errs() << "Error parsing -style: " << ec.message() << ", using "
-                   << FallbackStyle << " style\n";
+      return make_string_error("Error parsing -style: " + ec.message());
     }
+  Style.Language = Language;
     return Style;
   }
 
   if (!StyleName.equals_lower("file")) {
+    FormatStyle Style;
     if (!getPredefinedStyle(StyleName, Style.Language, &Style))
-      llvm::errs() << "Invalid value for -style, using " << FallbackStyle
-                   << " style\n";
-    return Style;
+      return make_string_error("Invalid value for -style");
+  Style.Language = Language;
+  return Style;
   }
 
   // Look for .clang-format/_clang-format file in the file's parent directories.
   SmallString<128> UnsuitableConfigFiles;
   SmallString<128> Path(FileName);
   if (std::error_code EC = FS->makeAbsolute(Path)) {
-    llvm::errs() << EC.message() << "\n";
-    return Style;
+    return make_string_error(EC.message());
   }
 
   for (StringRef Directory = Path; !Directory.empty();
        Directory = llvm::sys::path::parent_path(Directory)) {
 
     auto Status = FS->status(Directory);
     if (!Status ||
         Status->getType() != llvm::sys::fs::file_type::directory_file) {
       continue;
     }
 
     SmallString<128> ConfigFile(Directory);
 
     llvm::sys::path::append(ConfigFile, ".clang-format");
     DEBUG(llvm::dbgs() << "Trying " << ConfigFile << "...\n");
 
     Status = FS->status(ConfigFile.str());
-    bool IsFile =
+    bool FoundConfigFile =
         Status && (Status->getType() == llvm::sys::fs::file_type::regular_file);
-    if (!IsFile) {
+    if (!FoundConfigFile) {
       // Try _clang-format too, since dotfiles are not commonly used on Windows.
       ConfigFile = Directory;
       llvm::sys::path::append(ConfigFile, "_clang-format");
       DEBUG(llvm::dbgs() << "Trying " << ConfigFile << "...\n");
       Status = FS->status(ConfigFile.str());
-      IsFile = Status &&
-               (Status->getType() == llvm::sys::fs::file_type::regular_file);
+      FoundConfigFile = Status && (Status->getType() ==
+                                   llvm::sys::fs::file_type::regular_file);
     }
 
-    if (IsFile) {
+    if (FoundConfigFile) {
       llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> Text =
           FS->getBufferForFile(ConfigFile.str());
       if (std::error_code EC = Text.getError()) {
-        llvm::errs() << EC.message() << "\n";
-        break;
+        return make_string_error(EC.message());
       }
+      FormatStyle Style;
       if (std::error_code ec =
               parseConfiguration(Text.get()->getBuffer(), &Style)) {
         if (ec == ParseError::Unsuitable) {
           if (!UnsuitableConfigFiles.empty())
             UnsuitableConfigFiles.append(", ");
           UnsuitableConfigFiles.append(ConfigFile);
           continue;
         }
-        llvm::errs() << "Error reading " << ConfigFile << ": " << ec.message()
-                     << "\n";
-        break;
+        return make_string_error("Error reading " + ConfigFile + ": " +
+                                 ec.message());
       }
       DEBUG(llvm::dbgs() << "Using configuration file " << ConfigFile << "\n");
-      return Style;
+    Style.Language = Language;
+    return Style;
     }
   }
   if (!UnsuitableConfigFiles.empty()) {
-    llvm::errs() << "Configuration file(s) do(es) not support "
-                 << getLanguageName(Style.Language) << ": "
-                 << UnsuitableConfigFiles << "\n";
+    // TODO: If we find an unsuitable file for this language, are we expected to
+    // use the fallback style?
+    // If so, can't return this error here...
+    return make_string_error("Configuration file(s) do(es) not support " +
+                             getLanguageName(Language) + ": " +
+                             UnsuitableConfigFiles);
   }
-  return Style;
+
+  // No config file found, return the fallback style, which may be no style
+  return FallbackStyle;
 }
 
 } // namespace format
 } // namespace clang

lib/Tooling/Refactoring.cpp

for (const auto &FileAndReplaces : groupReplacementsByFile(
            Rewrite.getSourceMgr().getFileManager(), FileToReplaces)) {
     const std::string &FilePath = FileAndReplaces.first;
     auto &CurReplaces = FileAndReplaces.second;
 
     const FileEntry *Entry = Files.getFile(FilePath);
     FileID ID = SM.getOrCreateFileID(Entry, SrcMgr::C_User);
     StringRef Code = SM.getBufferData(ID);
 
-    format::FormatStyle CurStyle = format::getStyle(Style, FilePath, "LLVM");
+  llvm::Expected<format::FormatStyle> FormatStyleOrError = format::getStyle(Style, FilePath, "LLVM");
+  if (!FormatStyleOrError) {
+    llvm::errs() << llvm::toString(FormatStyleOrError.takeError()) << "\n";
+    return false;
+  }
+  const format::FormatStyle CurStyle = *FormatStyleOrError;
+
     auto NewReplacements =
         format::formatReplacements(Code, CurReplaces, CurStyle);
     if (!NewReplacements) {
       llvm::errs() << llvm::toString(NewReplacements.takeError()) << "\n";
       return false;
     }
     Result = applyAllReplacements(*NewReplacements, Rewrite) && Result;
   }
   return Result;
 }
 
 } // end namespace tooling
 } // end namespace clang

tools/clang-format/ClangFormat.cpp

static bool format(StringRef FileName) {
   }
   std::unique_ptr<llvm::MemoryBuffer> Code = std::move(CodeOrErr.get());
   if (Code->getBufferSize() == 0)
     return false; // Empty files are formatted correctly.
   std::vector<tooling::Range> Ranges;
   if (fillRanges(Code.get(), Ranges))
     return true;
   StringRef AssumedFileName = (FileName == "-") ? AssumeFileName : FileName;
-  FormatStyle FormatStyle =
+
+  llvm::Expected<FormatStyle> FormatStyleOrError =
       getStyle(Style, AssumedFileName, FallbackStyle, Code->getBuffer());
+  if (!FormatStyleOrError) {
+    llvm::errs() << llvm::toString(FormatStyleOrError.takeError()) << "\n";
+    return true;
+  }
+  FormatStyle FormatStyle = *FormatStyleOrError;
   if (SortIncludes.getNumOccurrences() != 0)
     FormatStyle.SortIncludes = SortIncludes;
   unsigned CursorPosition = Cursor;
   Replacements Replaces = sortIncludes(FormatStyle, Code->getBuffer(), Ranges,
                                        AssumedFileName, &CursorPosition);
   auto ChangedCode = tooling::applyAllReplacements(Code->getBuffer(), Replaces);
   if (!ChangedCode) {
     llvm::errs() << llvm::toString(ChangedCode.takeError()) << "\n";
cl::ParseCommandLineOptions(
       "If <file>s are given, it reformats the files. If -i is specified\n"
       "together with <file>s, the files are edited in-place. Otherwise, the\n"
       "result is written to the standard output.\n");
 
   if (Help)
     cl::PrintHelpMessage();
 
   if (DumpConfig) {
-    std::string Config =
-        clang::format::configurationAsText(clang::format::getStyle(
+    llvm::Expected<clang::format::FormatStyle> FormatStyleOrError =
+        clang::format::getStyle(
             Style, FileNames.empty() ? AssumeFileName : FileNames[0],
-            FallbackStyle));
+            FallbackStyle);
+    if (!FormatStyleOrError) {
+      llvm::errs() << llvm::toString(FormatStyleOrError.takeError()) << "\n";
+      return 1;
+    }
+    std::string Config =
+        clang::format::configurationAsText(*FormatStyleOrError);
     outs() << Config << "\n";
     return 0;
   }
 
   bool Error = false;
   switch (FileNames.size()) {
   case 0:
     Error = clang::format::format("-");

unittests/Format/FormatTest.cpp

TEST(FormatStyle, GetStyleOfFile) {
   vfs::InMemoryFileSystem FS;
   // Test 1: format file in the same directory.
   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_EQ(Style1, getLLVMStyle());
+  ASSERT_TRUE((bool)Style1);
+  ASSERT_EQ(*Style1, getLLVMStyle());
 
   // Test 2: 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_EQ(Style2, getMozillaStyle());
+  ASSERT_TRUE((bool)Style2);
+  ASSERT_EQ(*Style2, getMozillaStyle());
 
   // 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_EQ(Style3, getGoogleStyle());
+  ASSERT_TRUE((bool)Style3);
+  ASSERT_EQ(*Style3, getGoogleStyle());
+
+  // Test 4: error on invalid fallback style
+  auto Style4 = getStyle("file", "a.h", "KungFu", "", &FS);
+  ASSERT_FALSE((bool)Style4);
+  auto ErrorMsg4 = llvm::toString(Style4.takeError());
+  ASSERT_GT(ErrorMsg4.length(), 0);
+
+  // Test 5: error on invalid yaml on command line
+  auto Style5 = getStyle("{invalid_key=invalid_value}", "a.h", "LLVM", "", &FS);
+  ASSERT_FALSE((bool)Style5);
+  auto ErrorMsg5 = llvm::toString(Style5.takeError());
+  ASSERT_GT(ErrorMsg5.length(), 0);
+
+  // Test 6: error on invalid style
+  auto Style6 = getStyle("KungFu", "a.h", "LLVM", "", &FS);
+  ASSERT_FALSE((bool)Style6);
+  auto ErrorMsg6 = llvm::toString(Style6.takeError());
+  ASSERT_GT(ErrorMsg6.length(), 0);
+
+  // Test 7: found config file, error on fail to open
+  // TODO: not sure how to test this
+
+  // Test 8: found config file, error on parsing it
+  ASSERT_TRUE(
+      FS.addFile("/d/.clang-format", 0,
+                 llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: LLVM\n"
+                                                  "InvalidKey: InvalidValue")));
+  ASSERT_TRUE(
+      FS.addFile("/d/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int i;")));
+  auto Style8 = getStyle("file", "/d/.clang-format", "LLVM", "", &FS);
+  ASSERT_FALSE((bool)Style8);
+  auto ErrorMsg8 = llvm::toString(Style8.takeError());
+  ASSERT_GT(ErrorMsg8.length(), 0);
 }
 
 TEST_F(ReplacementTest, FormatCodeAfterReplacements) {
   // Column limit is 20.
   std::string Code = "Type *a =\n"
                      "    new Type();\n"
                      "g(iiiii, 0, jjjjj,\n"
                      "  0, kkkkk, 0, mm);\n"

unittests/Format/FormatTestObjC.cpp

protected:
   void verifyIncompleteFormat(StringRef Code) {
     EXPECT_EQ(Code.str(), format(test::messUp(Code), IC_ExpectIncomplete));
   }
 
   FormatStyle Style;
 };
 
 TEST_F(FormatTestObjC, DetectsObjCInHeaders) {
-  Style = getStyle("LLVM", "a.h", "none", "@interface\n"
+  Style = *getStyle("LLVM", "a.h", "none", "@interface\n"
                                           "- (id)init;");
   EXPECT_EQ(FormatStyle::LK_ObjC, Style.Language);
-  Style = getStyle("LLVM", "a.h", "none", "@interface\n"
+  Style = *getStyle("LLVM", "a.h", "none", "@interface\n"
                                           "+ (id)init;");
   EXPECT_EQ(FormatStyle::LK_ObjC, Style.Language);
 
   // No recognizable ObjC.
-  Style = getStyle("LLVM", "a.h", "none", "void f() {}");
+  Style = *getStyle("LLVM", "a.h", "none", "void f() {}");
   EXPECT_EQ(FormatStyle::LK_Cpp, Style.Language);
 }
 
 TEST_F(FormatTestObjC, FormatObjCTryCatch) {
   verifyFormat("@try {\n"
                "  f();\n"
                "} @catch (NSException e) {\n"
                "  @throw;\n"