[clang-format] DisableFormat also now disables SortIncludes

[SamMaier]

Summary

Previously, in order to have clang-format not format something, you had to give both:

SortIncludes: false
DisableFormat: true

This is confusing to users, who would expect that DisableFormat does what it says, and prevents clang-format from editing any files it applies to. See https://stackoverflow.com/questions/55833838/clang-format-still-formatting-with-disableformat-true for example.

[thakis]

Makes sense to me, but krasimir should probably approve this.

Please upload patches with lots of context (git diff -U9999 master); that makes reviewing on phab easier.

[SamMaier]

Diff with more context

[MyDeveloperDay]

I assume the intention was that users could have DisableFormat=true and SortIncludes=true when they want to sort the includes but not perform any additional formatting in the code.

I think by making this change you make it impossible to run clang-format through a codebase with the sole intention of just sorting the headers. (which I could see as potentially useful isolated functionality)..

If SortIncludes is false by default? (which you are making it not for no style so I'm unclear what it would be now if you running without a BasedOnStyle setting (uninitialized?)) then you don't need to supply both unless you are using LLVM style or one of the other styles that turn it on.

Are you sure this is the right change?

clang/lib/Format/Format.cpp

2131

this no longer allows DisableFormat=true but SortInclude=true... i.e. running ClangFormat to just sort the headers.

[SamMaier]

In D67843#1677983, @MyDeveloperDay wrote:

I assume the intention was that users could have DisableFormat=true and SortIncludes=true when they want to sort the includes but not perform any additional formatting in the code.

I think by making this change you make it impossible to run clang-format through a codebase with the sole intention of just sorting the headers. (which I could see as potentially useful isolated functionality)..

If SortIncludes is false by default? (which you are making it not for no style so I'm unclear what it would be now if you running without a BasedOnStyle setting (uninitialized?)) then you don't need to supply both unless you are using LLVM style or one of the other styles that turn it on.

Are you sure this is the right change?

This is from my experience with Chromium's use of clang-format.

People intuitively use DisableFormat: true to (they think) turn off formatting for that subdirectory. The Chromium project has 10 instances of this (https://cs.chromium.org/search/?q=file:.clang-format+DisableFormat:%5CWtrue&sq=package:chromium&type=cs). The "correct" thing to do here is to specify BasedOnStyle: none if they actually want no formatting. If you don't provide a BasedOnStyle, the DefaultFallbackStyle is LLVM, which means that SortIncludes is always true unless directly overridden.

I see 3 options:

1. Take this change as is. This breaks the ability to only sort includes without doing any other formatting. I'm not sure how much sort includes only is used, but my guess would be not often. It makes DisableFormat actually disable all formatting.
2. Don't change anything. This means that DisableFormat does not disable all formatting, despite the docs (https://clang.llvm.org/docs/ClangFormatStyleOptions.html) stating it "disables formatting completely". The only real way to disable formatting for a directory is to use BasedOnStyle: none, or to specify SortIncludes: false, which both are pretty unintuitive IMO.
3. Change the default fallback to something else if there is a .clang-format file with no BasedOnStyle. This could have farther reaching consequences which could affect more users who depend on clang-format just applying LLVM style even if they don't specify it.

[thakis]

4. Make it so that if DisableFormat is explicitly set to true and SortIncludes isn't explicitly set, then it disables SortIncludes. Or, put a different way, when DisableFormat is set, set SortIncludes to false at that point. Then an explicit DisableFormat: true; SortIncludes: true would still work.

MyDeveloperDay, would you find that intuitive?

I think the patch as-is is fine as I said, but if folks want to sort includes without formatting, that might be another option.

[SamMaier]

In D67843#1679529, @thakis wrote:

4. Make it so that if DisableFormat is explicitly set to true and SortIncludes isn't explicitly set, then it disables SortIncludes. Or, put a different way, when DisableFormat is set, set SortIncludes to false at that point. Then an explicit DisableFormat: true; SortIncludes: true would still work.

MyDeveloperDay, would you find that intuitive?

I think the patch as-is is fine as I said, but if folks want to sort includes without formatting, that might be another option.

I wasn't sure about this since I didn't see any way of distinguishing an explicitly set Style attribute vs an inherited Style attribute from a BasedOnStyle. If there's an easy way to distinguish those two, I'm sure this is the preferred method.

[MyDeveloperDay]

I basically agree with all the comments, I agree with you that I doubt its ever used in SortIncludes:true and DisableFormat:true, I just saw this as a hole that probably based on Myrums Law (https://www.hyrumslaw.com/) means someone somewhere is already using this fact and we could come unstuck.

I have to admit I am surprised that BasedOnStyle:none really turns everything off, I should go back and check that as I've not seen why that is the case (I'm sure you correct)

I also have to admit the project I work on doesn't have a "BasedOnStyle" in our .clang-format, but actually this cause me out when developing a new setting which needed to be true by default (these seemed nowhere for me to set the default)

I think I like the idea of SortIncludes being turned off if not specifically turned on if thats possible, otherwise I'd say this patch LG but I'd check with @klimek as to what he thinks.

[krasimir]

I also am worried that this makes it impossible to just sort includes.

Alternatively, we could update the documentation of the DisableFormat option to mention this quirk:
https://github.com/llvm/llvm-project/blob/master/clang/include/clang/Format/Format.h#L1231
It's not ideal, but it would be an improvement over the current version.

clang/lib/Format/Format.cpp

FormatStyle getMicrosoftStyle(FormatStyle::LanguageKind Language) {
   Style.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
   Style.AllowShortLoopsOnASingleLine = false;
   return Style;
 }
 
 FormatStyle getNoStyle() {
   FormatStyle NoStyle = getLLVMStyle();
   NoStyle.DisableFormat = true;
-  NoStyle.SortIncludes = false;
-  NoStyle.SortUsingDeclarations = false;
   return NoStyle;
 }
 
 bool getPredefinedStyle(StringRef Name, FormatStyle::LanguageKind Language,
                         FormatStyle *Style) {
   if (Name.equals_lower("llvm")) {
     *Style = getLLVMStyle(Language);
   } else if (Name.equals_lower("chromium")) {

 }
 
 bool isLikelyXml(StringRef Code) { return Code.ltrim().startswith("<"); }
 
 tooling::Replacements sortIncludes(const FormatStyle &Style, StringRef Code,
                                    ArrayRef<tooling::Range> Ranges,
                                    StringRef FileName, unsigned *Cursor) {
   tooling::Replacements Replaces;
-  if (!Style.SortIncludes)
+  if (!Style.SortIncludes || Style.DisableFormat)
     return Replaces;
   if (isLikelyXml(Code))
     return Replaces;
   if (Style.Language == FormatStyle::LanguageKind::LK_JavaScript &&
       isMpegTS(Code))
     return Replaces;
   if (Style.Language == FormatStyle::LanguageKind::LK_JavaScript)
     return sortJavaScriptImports(Style, Code, Ranges, FileName);

clang/test/Format/disable-format.cpp

 // RUN: grep -Ev "// *[A-Z-]+:" %s | clang-format -style=none \
 // RUN:   | FileCheck -strict-whitespace %s
 
-// CHECK: int   i;
+// CHECK: #include <b>
+// CHECK-NEXT: #include <a>
+// CHECK-NEXT: int   i;
+#include <b>
+#include <a>
 int   i;