clang-format-vsix: fail when clang-format outputs to stderr

[amaiorano]

Summary

When clang-format outputs to stderr but returns 0, the extension will format the code anyway. This happens, for instance, when there's a syntax error or unknown value in a .clang-format file; the result is that the extension silently formats using the fallback style without informing the user of the problem. This change treats stderr output as an error, making sure it gets displayed to the user, and not formatting the code.

[amaiorano]

With this change, I now get the following message box when my .clang-format has an invalid field named "SomeInvalidField":

I do have to wonder whether the real bug is the fact that clang-format returns a success status (0) even though there's a failure while parsing. Feels odd to have it write to stderr but return 0.

[klimek]

Pondering this a bit - one question is whether we should make clang-format not return 0 if we pass -fallback-style=none (it already has this option) and we can't figure out a style. What do you think?

[amaiorano]

In D27440#614337, @klimek wrote:

Pondering this a bit - one question is whether we should make clang-format not return 0 if we pass -fallback-style=none (it already has this option) and we can't figure out a style. What do you think?

When you say "it already has this option", do you mean this is what fallback-style is set to by default in this extension? Because, in fact, by default it's set to LLVM. Personally, I think it should be set to "none" by default.

Having said that, to answer your question, personally I think "fallback-style" is really an option that only makes sense when "style=file" AND a file is not found. If a user has a .clang-format file, and it fails to parse correctly, they would likely find it surprising that it would then ignore that file and use the fallback style. I would much rather it just fail hard and not use the fallback style in this case.

[klimek]

Adding djasper, who had brought forward the idea.

[amaiorano]

Hi @djasper, just curious about your opinion on this change, and on the conversation following it, specifically regarding whether clang-format should really use the fallback style when failing to parse a .clang-format file. Thanks!

[amaiorano]

Hi @djasper, just curious about your opinion on this change, and on the conversation following it, specifically regarding whether clang-format should really use the fallback style when failing to parse a .clang-format file. Thanks!

[djasper]

I agree 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).

[amaiorano]

In D27440#624773, @djasper wrote:

I agree 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).

Cool, and do you agree that clang-format should also return non-zero when this happens? If so, I'll just abort this change. Perhaps I'll take a look at fixing this in clang-format as discussed.

[djasper]

Yes.. return non-zero seems right. This is an error condition.

[amaiorano]

In D27440#624917, @djasper wrote:

Yes.. return non-zero seems right. This is an error condition.

Hi @djasper ,

I started looking into making the changes to clang-format to have it return an error code when it's unable to parse the .clang-format file, but I'm not quite sure what the best approach is. The function that needs modifying is getStyle. There are multiple places in there where it outputs to stderr (llvm::errs()) when something goes wrong, like here.

So here's what I'm thinking in terms of solutions when an error occurs in getStyle:

1. Throw an exception. This is unidiomatic in clang-format, and isn't something I'm particularly fond of, but it means not modifying the return type, and not having to modify the tests since they assume the green path.

2. Return an llvm::ErrorOr. Of course, this changes the signature, which means changing the few places that call getStyle. From what I can tell, it's only being called in a couple of places, and in the tests, so perhaps it's not terrible.

3. Return an llvm::Optional. This is similar to ErrorOr, except that it may allow us to codify the fallback behaviour on the outside of this call. What I mean is that with Optional, we wouldn't have to pass in the fallback style, but rather, the function could return a non-value optional when the input style isn't found or parsed correctly, etc., and the calling code can decide what to do with the it: either stop right there (return an error code from main), or it can try to get the fallback style. Something like:

// The case where we don't care about errors and want to use a fallback style:
FormatStyle fallbackStyle = getLLVMStyle();
FormatStyle formatStyle = getStyle("", fileName).getValueOr(fallbackStyle);


// The case where we do care about errors
auto maybeFormatStyle = getStyle("", fileName);
if (!maybeFormatStyle)
    return 1; // Return error from main

FormatStyle formatStyle = maybeFormatStyle.getValue();

Obviously this third option would change the most code, but maybe it makes more sense for getStyle to not have this notion of fallback style within it, as it effectively hides errors.

Would love to hear your thoughts.

[klimek]

+eric, who has some experience llvm::Error'ing our interfaces :)
llvm::ErrorOr seems like the right approach here?

[ioeric]

llvm::ErrorOr carries std::error_code. If you want richer information (e.g. error_code + error message), llvm::Expcted<T> and llvm::Error are your friends.

FYI, if you only need error_code + error_message in the returned error, there is also llvm::StringError. And if you want to carry even more information in the errors, you can implement llvm::ErrorInfo, which is what we are doing in libTooling replacements library: https://github.com/llvm-mirror/clang/blob/master/include/clang/Tooling/Core/Replacement.h#L150

[amaiorano]

In D27440#626415, @ioeric wrote:

llvm::ErrorOr carries std::error_code. If you want richer information (e.g. error_code + error message), llvm::Expcted<T> and llvm::Error are your friends.

FYI, if you only need error_code + error_message in the returned error, there is also llvm::StringError. And if you want to carry even more information in the errors, you can implement llvm::ErrorInfo, which is what we are doing in libTooling replacements library: https://github.com/llvm-mirror/clang/blob/master/include/clang/Tooling/Core/Replacement.h#L150

Thanks, I'll check these out! Btw, I noticed that the clang-format tests are non-Windows due to path assumptions. Is this a lost cause, or just something no one's bothered to look into yet?

[klimek]

In D27440#626477, @amaiorano wrote:

In D27440#626415, @ioeric wrote:

llvm::ErrorOr carries std::error_code. If you want richer information (e.g. error_code + error message), llvm::Expcted<T> and llvm::Error are your friends.

FYI, if you only need error_code + error_message in the returned error, there is also llvm::StringError. And if you want to carry even more information in the errors, you can implement llvm::ErrorInfo, which is what we are doing in libTooling replacements library: https://github.com/llvm-mirror/clang/blob/master/include/clang/Tooling/Core/Replacement.h#L150

Thanks, I'll check these out! Btw, I noticed that the clang-format tests are non-Windows due to path assumptions. Is this a lost cause, or just something no one's bothered to look into yet?

No one's bothered looking into it yet.

[cameron314]

Thanks, I'll check these out! Btw, I noticed that the clang-format tests are non-Windows due to path assumptions. Is this a lost cause, or just something no one's bothered to look into yet?

No one's bothered looking into it yet.

Which tests? We run the unit tests (FormatTests.exe) locally just fine on Windows.

[amaiorano]

In D27440#634043, @cameron314 wrote:

Thanks, I'll check these out! Btw, I noticed that the clang-format tests are non-Windows due to path assumptions. Is this a lost cause, or just something no one's bothered to look into yet?

No one's bothered looking into it yet.

Which tests? We run the unit tests (FormatTests.exe) locally just fine on Windows.

There was one test disabled for MSVC, which I fixed and enabled in D27971.

I will soon close this issue (D27440) once D28081 goes through as clang-format should return non-zero when an error occurs.

(EDIT: formatting)

[amaiorano]

This change is no longer needed since clang-format now returns failure status (non-zero) whenever it writes to stderr (e.g. on parse error) since https://llvm.org/svn/llvm-project/cfe/trunk@292174 (D28081)

tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs

public sealed class ClangFormatPackage : Package
             // 3. We notify clang-format that the input is done - after this point clang-format
             //    will start analyzing the input and eventually write the output.
             process.StandardInput.Close();
             // 4. We must read clang-format's output before waiting for it to exit; clang-format
             //    will close the channel by exiting.
             string output = process.StandardOutput.ReadToEnd();
             // 5. clang-format is done, wait until it is fully shut down.
             process.WaitForExit();
-            if (process.ExitCode != 0)
-            {
-                // FIXME: If clang-format writes enough to the standard error stream to block,
-                // we will never reach this point; instead, read the standard error asynchronously.
-                throw new Exception(process.StandardError.ReadToEnd());
+
+            string stdErr = process.StandardError.ReadToEnd();
+
+            if (process.ExitCode != 0 || stdErr.Length > 0)
+            {
+                throw new Exception(
+                    (stdErr.Length > 0 ? stdErr + "\n\n" : "") +
+                    "(Exit Code: " + process.ExitCode + ")");
             }
+
             return output;
         }
 
         /// <summary>
         /// Returns the currently active view if it is a IWpfTextView.
         /// </summary>
         private IWpfTextView GetCurrentView()
         {