[clang-format] Left/Right alignment fixer can cause false positive replacements when they don't actually change anything

[MyDeveloperDay]

Summary

Earlier during the development of D69764: [clang-format] Add Left/Right Const fixer capability I felt it was no longer necessary to ensure we were not trying to change code which didn't need to change and we felt this could be removed, however I'd like to bring this back for now as I am seeing some false positives in terms of the "replacements"

What I see is the generation of a replacement which is a "No Op" on the original code, I think this comes about because of the merging of replacements:

static const a;
->
const static a;
->
static const a;

The replacements don't really merge, in such a way as to identify when we have gone back to the original

Also remove the Penalty as I'm not using it (and it became marked as set and no used, I'd rather get rid of it if it means nothing)

I think we need to do this step for now, as many people use the --output-replacements-xml to identify that the file "needs a clang-format"

The same can be seen with the -n or --dry-run option as this uses the replacements to drive the error/warning output.

[HazardyKnusperkeks]

clang/lib/Format/QualifierAlignmentFixer.cpp

95

The message may be confusing, but honestly I don't know what to put in either, since "adding non no op replacements" may be even more confusing.

clang/unittests/Format/QualifierFixerTest.cpp

818

Doesn't the two argument version suffice?

[MyDeveloperDay]

Address review comment

clang/lib/Format/QualifierAlignmentFixer.cpp

std::pair<tooling::Replacements, unsigned> QualifierAlignmentFixer::analyze(
     TokenAnnotator &Annotator, SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
     FormatTokenLexer &Tokens) {
 
   auto Env =
       std::make_unique<Environment>(Code, FileName, Ranges, FirstStartColumn,
                                     NextStartColumn, LastStartColumn);
   llvm::Optional<std::string> CurrentCode = None;
   tooling::Replacements Fixes;
-  unsigned Penalty = 0;
   for (size_t I = 0, E = Passes.size(); I < E; ++I) {
     std::pair<tooling::Replacements, unsigned> PassFixes = Passes[I](*Env);
     auto NewCode = applyAllReplacements(
         CurrentCode ? StringRef(*CurrentCode) : Code, PassFixes.first);
     if (NewCode) {
       Fixes = Fixes.merge(PassFixes.first);
-      Penalty += PassFixes.second;
       if (I + 1 < E) {
         CurrentCode = std::move(*NewCode);
         Env = std::make_unique<Environment>(
             *CurrentCode, FileName,
             tooling::calculateRangesAfterReplacements(Fixes, Ranges),
             FirstStartColumn, NextStartColumn, LastStartColumn);
       }
     }
   }
-  return {Fixes, Penalty};
+
+  // Don't make replacements that replace nothing.
+  tooling::Replacements NonNoOpFixes;
+
+  for (auto I = Fixes.begin(), E = Fixes.end(); I != E; ++I) {
+    StringRef OriginalCode = Code.substr(I->getOffset(), I->getLength());
+
+    if (!OriginalCode.equals(I->getReplacementText())) {
+      auto Err = NonNoOpFixes.add(*I);
+      if (Err)
+        llvm::errs() << "Error adding replacements : "
+                     << llvm::toString(std::move(Err)) << "\n";
+    }
+  }
+  return {NonNoOpFixes, 0};
 }
 
 static void replaceToken(const SourceManager &SourceMgr,
                          tooling::Replacements &Fixes,
                          const CharSourceRange &Range, std::string NewText) {
   auto Replacement = tooling::Replacement(SourceMgr, Range, NewText);
   auto Err = Fixes.add(Replacement);
 

clang/unittests/Format/QualifierFixerTest.cpp

TEST_F(QualifierFixerTest, UnsignedQualifier) {
 
   Style.QualifierAlignment = FormatStyle::QAS_Right;
   Style.QualifierOrder = {"type", "const"};
 
   verifyFormat("Foo(unsigned char const *bytes)",
                "Foo(unsigned const char *bytes)", Style);
 }
 
+TEST_F(QualifierFixerTest, NoOpQualifierReplacements) {
+
+  FormatStyle Style = getLLVMStyle();
+  Style.QualifierAlignment = FormatStyle::QAS_Custom;
+  Style.QualifierOrder = {"static", "const", "type"};
+
+  ReplacementCount = 0;
+  EXPECT_EQ(ReplacementCount, 0);
+  verifyFormat("static const uint32 foo[] = {0, 31};", Style);
+  EXPECT_EQ(ReplacementCount, 0);
+}
+
 } // namespace format
 } // namespace clang