[clang-tidy] Skip reporting of not applicable fixes.

[etienneb]

Summary

Invalid source location are causing clang-tidy to crash when manipulating an invalid file.

Macro definitions on the command line have locations in a virtual buffer and therefore
don't have a corresponding valid FilePath.

A recent patch added path conversion to absolute path. As the FilePath may now be empty,
the result of makeAbsolutePath may incorrectly be the folder WorkingDir. The crash occurs
in getLocation which is not able to find the appropriate FileEntry (null pointer).

SmallString<128> FixAbsoluteFilePath = Fix.getFilePath();
Files.makeAbsolutePath(FixAbsoluteFilePath);
FixLoc = getLocation(FixAbsoluteFilePath, Fix.getOffset());

With relative path, the code was not crashing because getLocation was skipping empty path.

Example of code:

int main() { return X; }

With the given command-line:

clang-tidy test.cc --checks=misc-macro-*  --  -DX=0+0

[aaron.ballman]

Thank you for working on this! A question below, but also, the patch is missing test cases for the change.

ClangTidy.cpp

144 ↗

(On Diff #51010)

Is there a purpose to emitting the CreateReplacement if the fix isn't applicable?

[alexfh]

Thank you, Etienne!

Please add a regression test.

ClangTidy.cpp

144 ↗

(On Diff #51010)

No, I don't think we need a replacement with an invalid range.

[etienneb]

Added test.

ClangTidy.cpp

144 ↗

(On Diff #51010)

The CreateReplacement fix is ignored when appended. In the append function, there is a check to avoid invalid source range.

But, I think it's cleaner to move the statement within the if-block.

[aaron.ballman]

LGTM!

[alexfh]

LG. Thanks for the fix!

[etienneb]

rebase + fixing invalid paths

clang-tidy/ClangTidy.cpp

SmallVector<std::pair<SourceLocation, bool>, 4> FixLocations;
       if (Error.IsWarningAsError) {
         Name += ",-warnings-as-errors";
         Level = DiagnosticsEngine::Error;
         WarningsAsErrors++;
       }
       auto Diag = Diags.Report(Loc, Diags.getCustomDiagID(Level, "%0 [%1]"))
                   << Message.Message << Name;
       for (const tooling::Replacement &Fix : Error.Fix) {
+        // Retrieve the source range for applicable fixes. Macro definitions
+        // on the command line have locations in a virtual buffer and don't
+        // have valid file paths and are therefore not applicable.
+        SourceRange Range;
+        SourceLocation FixLoc;
+        if (Fix.isApplicable()) {
           SmallString<128> FixAbsoluteFilePath = Fix.getFilePath();
           Files.makeAbsolutePath(FixAbsoluteFilePath);
-        SourceLocation FixLoc =
-            getLocation(FixAbsoluteFilePath, Fix.getOffset());
+          FixLoc = getLocation(FixAbsoluteFilePath, Fix.getOffset());
           SourceLocation FixEndLoc = FixLoc.getLocWithOffset(Fix.getLength());
-        Diag << FixItHint::CreateReplacement(SourceRange(FixLoc, FixEndLoc),
-                                             Fix.getReplacementText());
+          Range = SourceRange(FixLoc, FixEndLoc);
+          Diag << FixItHint::CreateReplacement(Range, Fix.getReplacementText());
+        }
+              
         ++TotalFixes;
         if (ApplyFixes) {
           bool Success = Fix.isApplicable() && Fix.apply(Rewrite);
           if (Success)
             ++AppliedFixes;
           FixLocations.push_back(std::make_pair(FixLoc, Success));
         }
       }

test/clang-tidy/misc-macro-parentheses-cmdline.cpp

+// RUN: %check_clang_tidy %s misc-macro-parentheses %t -- -- -DVAL=0+0
+
+// The previous command-line is producing warnings and fixes with the source
+// locations from a virtual buffer. VAL is replaced by '0+0'.
+// Fixes could not be applied and should not be reported.
+int foo() { return VAL; }
+
+#define V 0+0
+int bar() { return V; }
+// CHECK-FIXES: #define V (0+0)