[clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range

[russellmcc]

Summary

When clang-format is set to always use tabs (with tab width 4), when asked to format line 3 (using the -lines=3:3 flag):

int f() {
    int a;
    foobar();
    int b;
}

We would expect clang-format to not modify the leading whitespace on line 4. However, it does - see the new test. This is because the whitespace manager is called to replace the whitespace before the first token of line 4. This is necessary to edit the number of new lines before line 4, and to edit the trailing whitespace on line 3. I've added a flag to replaceWhitespace that allows it to not edit the leading whitespace, and only edit the whitespace up to the last newline.

We ran into this when trying to integrate clang-format with a line filter into our CI to ensure no new formatting diffs were introduced in a patch. With the buggy behavior without this patch, the number of affected lines grew each time clang-format was run with a line filter, so running clang-format locally on the changed lines was not enough to ensure that CI would pass.

[russellmcc]

Bump! Thanks for your consideration.

[JonasToth]

Hi russell

thank you for the patch! As I am not a clang-format reviewers these are only general things, and Nits anyway ;)
Hope the reviewers I added will evaluate better.

lib/Format/WhitespaceManager.cpp

54

is this a SourceRange? Please spell out the type, as there are different kind of ranges.

unittests/Format/FormatTest.cpp

9159

Spurious change.

[russellmcc]

Respond to feedback

[russellmcc]

Thanks for the feedback!

[lebedev.ri]

https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options comes into mind.

[russellmcc]

Thanks for the feedback! This actually isn't a new formatting option, rather it's fixing a bug where clang-format would change lines outside of the line range asked for by the user. This was preventing us from using clang-format in an automated setting.

[russellmcc]

Bump! Thanks for the consideration

[russellmcc]

Bump! Thanks for the consideration! Again, this is preventing us from rolling out automated style checking on diffs at work.

[russellmcc]

Bump!

[russellmcc]

Bump! Still waiting feedback on this.

[MyDeveloperDay]

From what I can tell this looks OK as you seem to be mainly only passing down the boolean, my guess is that while most people don't use the -lines directly that often but it probably gets called by some of the formatting tools that work over selections, maybe something like git-clang-format

Assuming you have tested some of those usescase and run the FormatTests, then we should be able to apply the "Beyonce rule" for everything else. (I've actually run the tests with your patch and they are fine)

If that is the case then this LGTM (but I'm not the code owner, just someone wanting to help with these stranded reviews)

I've landed this patch in my fork (which contains list of desired patches which seem to never get landed here..)

https://github.com/mydeveloperday/clang-experimental

IMHO @russellmcc clang-format needs people like you who understand the internals, to help move along some of the reviews that just get stuck in no mans land.

[klimek]

I don't understand yet why running clang-format locally would not do the same change?

[russellmcc]

Sorry for the ambiguity. I meant, "running clang-format on the changed lines locally was not enough". I'll edit the description to clarify.

To go in more detail, let's imagine you changed line 3. You run clang-format locally on line 3, but because of the bug fixed in the patch, this affects lines 3 and 4. Then, when you commit and push to CI, CI will see diffs on lines 3 and 4. CI will then try to ensure that running clang-format on line range 3-4 will produce no diffs. However, due to the same bug will produce a diff on line 5, so your code will be rejected by the CI.

The reason our CI works like this is we want to ensure that no _new_ formatting errors are introduced into our code. We're extensive users of git blame and we don't want to do a full format pass on our whole codebase. Clang-format documents its line filter option, so in my opinion, it should work correctly.

[russellmcc]

@MyDeveloperDay Thanks for the approve! Yes, this patch has been working for us as we've been using it on an internal fork since I opened the patch. As I mentioned, without this patch clang-format is useless for us in CI.

[klimek]

I just verified why this doesn't break for our CI:
The trick is that if the indent is actually off (as opposed to being correct, but tab vs spaces), we do re-indent until we find an indent that's correct.
The fix that would make me happier, I think, is to do the same thing here:
Basically, when we expand the range until nothing changes, do not only check whether the indent is correct, but also whether we'd change the spaces in the indent.
WDYT?

[russellmcc]

klimek: I'm sorry, I don't fully understand your proposed fix. Could you explain it in more detail?

Without being able to respond to your proposal in detail, I strongly believe if you pass in a line range to clang-format, it should not change lines outside that range OR we should modify the documentation to clearly explain what the line filter option does.

[klimek]

In D54881#1449848, @russellmcc wrote:

klimek: I'm sorry, I don't fully understand your proposed fix. Could you explain it in more detail?

Without being able to respond to your proposal in detail, I strongly believe if you pass in a line range to clang-format, it should not change lines outside that range OR we should modify the documentation to clearly explain what the line filter option does.

That is a good point - generally, the problem is that one use case for clang-format is to give it some point in an expression, and have it fix that expression and currently "things around it that are off". I'm not saying that's the right solution, but if we want to change that it's a significant change to clang-format's workflow, so I think we'll need to carefully think about the effect this has, or provide reasons why we think this will not change those use cases.

For example, if I put {} around a range I typically use clang-format in some position in that range to fix up the whole range for me. This is a feature. I'm open to hearing more opinions, though.

[russellmcc]

Thanks for the explanation! I do understand your philosophy on this, and agree with the desired behavior case you brought up where you have put in new braces.

After thinking about this more, the thing I really care about is that clang-format is idempotent with a line filter - i.e., running it twice should always have the same effect as running it once, with a line-filter including the new changes.

So, either this fix, or your proposed fix of fixing all lines until the next correct indentation would meet that idempotence criteria.

However, I think in this particular case I still prefer my fix - to me, line filter is meant to limit the effect of clang-format to just fix a particular change and the fallout from that. However, if the lines _after_ a change were wrong before, this feels very unrelated to the change that was made - why is now the time to fix these unrelated lines?

[klimek]

In D54881#1462804, @russellmcc wrote:

Thanks for the explanation! I do understand your philosophy on this, and agree with the desired behavior case you brought up where you have put in new braces.

After thinking about this more, the thing I really care about is that clang-format is idempotent with a line filter - i.e., running it twice should always have the same effect as running it once.

So, either this fix, or your proposed fix of fixing all lines until the next correct indentation would meet that idempotence criteria.

However, I think in this particular case I still prefer my fix - to me, line filter is meant to limit the effect of clang-format to just fix a particular change and the fallout from that. However, if the lines _after_ a change were wrong before, this feels very unrelated to the change that was made - why is now the time to fix these unrelated lines?

I agree and would be happy with the change if it would only change the line-filtered workflow, but this afaict (unless I'm missing something :) will also affect the workflow where the provided range is 0-length range, which has an implicit "format stuff around this" request from the user inside it. I'd be happy with a patch that differentiates these two sides.

[MyDeveloperDay]

I agree and would be happy with the change if it would only change the line-filtered workflow, but this afaict (unless I'm missing something :) will also affect the workflow where the provided range is 0-length range, which has an implicit "format stuff around this" request from the user inside it. I'd be happy with a patch that differentiates these two sides.

This use case needs capturing in a unit test..it isn't obvious

[klimek]

In D54881#1462893, @MyDeveloperDay wrote:

I agree and would be happy with the change if it would only change the line-filtered workflow, but this afaict (unless I'm missing something :) will also affect the workflow where the provided range is 0-length range, which has an implicit "format stuff around this" request from the user inside it. I'd be happy with a patch that differentiates these two sides.

This use case needs capturing in a unit test..it isn't obvious

Agreed!

[russellmcc]

Sorry, coming back to this - line-filters are _inclusive_, so how do you indicate a 0-length range from the command line?

[russellmcc]

Hey, still motivated to land this, but definitely don't want to break any existing workflows.

In particular,

but this afaict (unless I'm missing something :) will also affect the workflow where the provided range is 0-length range

I'm curious how to do this - as I mentioned, I know how to trigger a format on a 1-length range, but I don't understand how to trigger a 0-length range.

[russellmcc]

Still waiting for feedback on this change! We've been happily using this patch at my company for some time.

[MyDeveloperDay]

If we are concerned about affecting other workflows about if we added a new command line option to trigger your behavior?

I'd also like to see some lit tests in clang/test/Format to exercise the -lines so we really understand its usage

I also wonder if anyone else would notice? if they do I sure wish their usage was captured in the unit tests

lib/Format/UnwrappedLineFormatter.h

unsigned format(const SmallVectorImpl<AnnotatedLine *> &Lines,
                   unsigned LastStartColumn = 0);
 
 private:
   /// Add a new line and the required indent before the first Token
   /// of the \c UnwrappedLine if there was no structural parsing error.
   void formatFirstToken(const AnnotatedLine &Line,
                         const AnnotatedLine *PreviousLine,
                         const SmallVectorImpl<AnnotatedLine *> &Lines,
-                        unsigned Indent, unsigned NewlineIndent);
+                        unsigned Indent, unsigned NewlineIndent,
+                        bool OnlyUntilLastNewline = false);
 
   /// Returns the column limit for a line, taking into account whether we
   /// need an escaped newline due to a continued preprocessor directive.
   unsigned getColumnLimit(bool InPPDirective,
                           const AnnotatedLine *NextLine) const;
 
   // Cache to store the penalty of formatting a vector of AnnotatedLines
   // starting from a specific additional offset. Improves performance if there

lib/Format/UnwrappedLineFormatter.cpp

if (ShouldFormat && TheLine.Type != LT_Invalid) {
       if (StartsNewLine)
         IndentTracker.adjustToUnmodifiedLine(TheLine);
       if (!DryRun) {
         bool ReformatLeadingWhitespace =
             StartsNewLine && ((PreviousLine && PreviousLine->Affected) ||
                               TheLine.LeadingEmptyLinesAffected);
         // Format the first token.
         if (ReformatLeadingWhitespace)
-          formatFirstToken(TheLine, PreviousLine, Lines,
-                           TheLine.First->OriginalColumn,
-                           TheLine.First->OriginalColumn);
+          formatFirstToken(
+              TheLine, PreviousLine, Lines, TheLine.First->OriginalColumn,
+              TheLine.First->OriginalColumn, /*OnlyUntilLastNewline=*/true);
         else
           Whitespaces->addUntouchableToken(*TheLine.First,
                                            TheLine.InPPDirective);
 
         // Notify the WhitespaceManager about the unchanged whitespace.
         for (FormatToken *Tok = TheLine.First->Next; Tok; Tok = Tok->Next)
           Whitespaces->addUntouchableToken(*Tok, TheLine.InPPDirective);
       }
       NextLine = Joiner.getNextMergedLine(DryRun, IndentTracker);
       RangeMinLevel = UINT_MAX;
     }
     if (!DryRun)
       markFinalized(TheLine.First);
     PreviousLine = &TheLine;
   }
   PenaltyCache[CacheKey] = Penalty;
   return Penalty;
 }
 
 void UnwrappedLineFormatter::formatFirstToken(
     const AnnotatedLine &Line, const AnnotatedLine *PreviousLine,
     const SmallVectorImpl<AnnotatedLine *> &Lines, unsigned Indent,
-    unsigned NewlineIndent) {
+    unsigned NewlineIndent, bool OnlyUntilLastNewline) {
   FormatToken &RootToken = *Line.First;
   if (RootToken.is(tok::eof)) {
     unsigned Newlines = std::min(RootToken.NewlinesBefore, 1u);
     unsigned TokenIndent = Newlines ? NewlineIndent : 0;
     Whitespaces->replaceWhitespace(RootToken, Newlines, TokenIndent,
                                    TokenIndent);
     return;
   }
if (Newlines)
     Indent = NewlineIndent;
 
   // Preprocessor directives get indented after the hash, if indented.
   if (Line.Type == LT_PreprocessorDirective || Line.Type == LT_ImportStatement)
     Indent = 0;
 
   Whitespaces->replaceWhitespace(RootToken, Newlines, Indent, Indent,
                                  Line.InPPDirective &&
-                                     !RootToken.HasUnescapedNewline);
+                                     !RootToken.HasUnescapedNewline,
+                                 OnlyUntilLastNewline);
 }
 
 unsigned
 UnwrappedLineFormatter::getColumnLimit(bool InPPDirective,
                                        const AnnotatedLine *NextLine) const {
   // In preprocessor directives reserve two chars for trailing " \" if the
   // next line continues the preprocessor directive.
   bool ContinuesPPDirective =

lib/Format/WhitespaceManager.h

public:
   /// Replaces the whitespace in front of \p Tok. Only call once for
   /// each \c AnnotatedToken.
   ///
   /// \p StartOfTokenColumn is the column at which the token will start after
   /// this replacement. It is needed for determining how \p Spaces is turned
   /// into tabs and spaces for some format styles.
   void replaceWhitespace(FormatToken &Tok, unsigned Newlines, unsigned Spaces,
                          unsigned StartOfTokenColumn,
-                         bool InPPDirective = false);
+                         bool InPPDirective = false,
+                         bool OnlyUntilLastNewline = false);
 
   /// Adds information about an unchangeable token's whitespace.
   ///
   /// Needs to be called for every token for which \c replaceWhitespace
   /// was not called.
   void addUntouchableToken(const FormatToken &Tok, bool InPPDirective);
 
   llvm::Error addReplacement(const tooling::Replacement &Replacement);

lib/Format/WhitespaceManager.cpp

: Tok(&Tok), CreateReplacement(CreateReplacement),
       ContinuesPPDirective(ContinuesPPDirective), Spaces(Spaces),
       IsInsideToken(IsInsideToken), IsTrailingComment(false), TokenLength(0),
       PreviousEndOfTokenColumn(0), EscapedNewlineColumn(0),
       StartOfBlockComment(nullptr), IndentationOffset(0) {}
 
 void WhitespaceManager::replaceWhitespace(FormatToken &Tok, unsigned Newlines,
                                           unsigned Spaces,
                                           unsigned StartOfTokenColumn,
-                                          bool InPPDirective) {
+                                          bool InPPDirective,
+                                          bool OnlyUntilLastNewline) {
   if (Tok.Finalized)
     return;
   Tok.Decision = (Newlines > 0) ? FD_Break : FD_Continue;
-  Changes.push_back(Change(Tok, /*CreateReplacement=*/true, Tok.WhitespaceRange,
-                           Spaces, StartOfTokenColumn, Newlines, "", "",
-                           InPPDirective && !Tok.IsFirst,
+  auto OriginalWhitespaceRange = Tok.WhitespaceRange;
+
+  if (OnlyUntilLastNewline) {
+    OriginalWhitespaceRange.setEnd(
+        OriginalWhitespaceRange.getBegin().getLocWithOffset(
+            Tok.LastNewlineOffset));
+    Spaces = 0;
+    StartOfTokenColumn = 0;
+  }
+  Changes.push_back(Change(Tok, /*CreateReplacement=*/true,
+                           OriginalWhitespaceRange, Spaces, StartOfTokenColumn,
+                           Newlines, "", "", InPPDirective && !Tok.IsFirst,
                            /*IsInsideToken=*/false));
 }
 
 void WhitespaceManager::addUntouchableToken(const FormatToken &Tok,
                                             bool InPPDirective) {
   if (Tok.Finalized)
     return;
   Changes.push_back(Change(Tok, /*CreateReplacement=*/false,

unittests/Format/FormatTest.cpp

 class FormatTest : public ::testing::Test {
 protected:
   enum StatusCheck {
     SC_ExpectComplete,
     SC_ExpectIncomplete,
     SC_DoNotCheck
   };
 
-  std::string format(llvm::StringRef Code,
+  std::string formatRange(llvm::StringRef Code, tooling::Range Range,
                           const FormatStyle &Style = getLLVMStyle(),
                           StatusCheck CheckComplete = SC_ExpectComplete) {
     LLVM_DEBUG(llvm::errs() << "---\n");
     LLVM_DEBUG(llvm::errs() << Code << "\n\n");
-    std::vector<tooling::Range> Ranges(1, tooling::Range(0, Code.size()));
+    std::vector<tooling::Range> Ranges(1, std::move(Range));
     FormattingAttemptStatus Status;
     tooling::Replacements Replaces =
         reformat(Style, Code, Ranges, "<stdin>", &Status);
     if (CheckComplete != SC_DoNotCheck) {
       bool ExpectedCompleteFormat = CheckComplete == SC_ExpectComplete;
       EXPECT_EQ(ExpectedCompleteFormat, Status.FormatComplete)
           << Code << "\n\n";
     }
     ReplacementCount = Replaces.size();
     auto Result = applyAllReplacements(Code, Replaces);
     EXPECT_TRUE(static_cast<bool>(Result));
     LLVM_DEBUG(llvm::errs() << "\n" << *Result << "\n\n");
     return *Result;
   }
 
+  std::string format(llvm::StringRef Code,
+                     const FormatStyle &Style = getLLVMStyle(),
+                     StatusCheck CheckComplete = SC_ExpectComplete) {
+    return formatRange(Code, tooling::Range(0, Code.size()), Style,
+                       CheckComplete);
+  }
+
   FormatStyle getStyleWithColumns(FormatStyle Style, unsigned ColumnLimit) {
     Style.ColumnLimit = ColumnLimit;
     return Style;
   }
 
   FormatStyle getLLVMStyleWithColumns(unsigned ColumnLimit) {
     return getStyleWithColumns(getLLVMStyle(), ColumnLimit);
   }
protected:
   }
 
   /// \brief Verify that clang-format does not crash on the given input.
   void verifyNoCrash(llvm::StringRef Code,
                      const FormatStyle &Style = getLLVMStyle()) {
     format(Code, Style, SC_DoNotCheck);
   }
 
+  void verifyWithPrefixAndSuffix(llvm::StringRef Expected, llvm::StringRef Code,
+                                 llvm::StringRef prefix, llvm::StringRef suffix,
+                                 const FormatStyle &Style = getLLVMStyle()) {
+    EXPECT_EQ(llvm::Twine(prefix + Expected + suffix).str(),
+              formatRange(llvm::Twine(prefix + Code + suffix).str(),
+                          tooling::Range(prefix.size(), Code.size()), Style));
+  }
+
   int ReplacementCount;
 };
 
 TEST_F(FormatTest, MessUp) {
   EXPECT_EQ("1 2 3", test::messUp("1 2 3"));
   EXPECT_EQ("1 2 3\n", test::messUp("1\n2\n3\n"));
   EXPECT_EQ("a\n//b\nc", test::messUp("a\n//b\nc"));
   EXPECT_EQ("a\n#b\nc", test::messUp("a\n#b\nc"));
EXPECT_EQ("{\n"
             "  int i;\n"
             "}",
             format("{\n"
                    "\t/*\n"
                    "\t * Comment\n"
                    "\t */\n"
                    "\t int i;\n"
                    "}"));
+
   Tab.AlignConsecutiveAssignments = true;
   Tab.AlignConsecutiveDeclarations = true;
   Tab.TabWidth = 4;
   Tab.IndentWidth = 4;
   verifyFormat("class Assign {\n"
                "\tvoid f() {\n"
                "\t\tint         x      = 123;\n"
                "\t\tint         random = 4;\n"
                "\t\tstd::string alphabet =\n"
                "\t\t\t\"abcdefghijklmnopqrstuvwxyz\";\n"
                "\t}\n"
                "};",
                Tab);
 }
 
+TEST_F(FormatTest, FormattingIndentationDoesNotAffectOtherLines) {
+  FormatStyle Tab = getLLVMStyleWithColumns(42);
+  Tab.TabWidth = 4;
+  Tab.UseTab = FormatStyle::UT_Always;
+  verifyWithPrefixAndSuffix("\tfoobar();", "    foobar();",
+                            "int f() {\n    int a;\n", "\n    int b;\n}\n",
+                            Tab);
+  Tab.UseTab = FormatStyle::UT_Never;
+  verifyWithPrefixAndSuffix("    foobar();", "\tfoobar();",
+                            "int f() {\n\tint a;\n", "\n\tint b;\n}\n", Tab);
+}
+
 TEST_F(FormatTest, CalculatesOriginalColumn) {
   EXPECT_EQ("\"qqqqqqqqqqqqqqqqqqqqqqqqqq\\\n"
             "q\"; /* some\n"
             "       comment */",
             format("  \"qqqqqqqqqqqqqqqqqqqqqqqqqq\\\n"
                    "q\"; /* some\n"
                    "       comment */",
                    getLLVMStyle()));