From 166935764b114e2c7b2f82de4887d2a3a7dd22bd Mon Sep 17 00:00:00 2001 From: Angel Garcia Gomez Date: Fri, 16 Oct 2015 11:43:49 +0000 Subject: [PATCH] Fix overlapping replacements in clang-tidy. Summary: Prevent clang-tidy from applying fixes to errors that overlap with other errors' fixes, with one exception: if one fix is completely contained inside another one, then we can apply the big one. Reviewers: bkramer, klimek Subscribers: djasper, cfe-commits, alexfh Differential Revision: http://reviews.llvm.org/D13516 llvm-svn: 250509 --- .../ClangTidyDiagnosticConsumer.cpp | 168 +++++++++++++++--- .../clang-tidy/ClangTidyDiagnosticConsumer.h | 8 +- .../OverlappingReplacementsTest.cpp | 132 ++++++++------ 3 files changed, 229 insertions(+), 79 deletions(-) diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp index 8fbccbfb4fc4..3d14e1b7807e 100644 --- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp +++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp @@ -22,8 +22,8 @@ #include "clang/Basic/DiagnosticOptions.h" #include "clang/Frontend/DiagnosticRenderer.h" #include "llvm/ADT/SmallString.h" -#include #include +#include using namespace clang; using namespace tidy; @@ -146,8 +146,7 @@ static llvm::Regex ConsumeGlob(StringRef &GlobList) { } GlobList::GlobList(StringRef Globs) - : Positive(!ConsumeNegativeIndicator(Globs)), - Regex(ConsumeGlob(Globs)), + : Positive(!ConsumeNegativeIndicator(Globs)), Regex(ConsumeGlob(Globs)), NextGlob(Globs.empty() ? nullptr : new GlobList(Globs)) {} bool GlobList::contains(StringRef S, bool Contains) { @@ -222,9 +221,7 @@ const ClangTidyOptions &ClangTidyContext::getOptions() const { return CurrentOptions; } -void ClangTidyContext::setCheckProfileData(ProfileData *P) { - Profile = P; -} +void ClangTidyContext::setCheckProfileData(ProfileData *P) { Profile = P; } GlobList &ClangTidyContext::getChecksFilter() { assert(CheckFilter != nullptr); @@ -296,16 +293,16 @@ void ClangTidyDiagnosticConsumer::HandleDiagnostic( // This is a compiler diagnostic without a warning option. Assign check // name based on its level. switch (DiagLevel) { - case DiagnosticsEngine::Error: - case DiagnosticsEngine::Fatal: - CheckName = "clang-diagnostic-error"; - break; - case DiagnosticsEngine::Warning: - CheckName = "clang-diagnostic-warning"; - break; - default: - CheckName = "clang-diagnostic-unknown"; - break; + case DiagnosticsEngine::Error: + case DiagnosticsEngine::Fatal: + CheckName = "clang-diagnostic-error"; + break; + case DiagnosticsEngine::Warning: + CheckName = "clang-diagnostic-warning"; + break; + default: + CheckName = "clang-diagnostic-unknown"; + break; } } @@ -340,7 +337,7 @@ bool ClangTidyDiagnosticConsumer::passesLineFilter(StringRef FileName, unsigned LineNumber) const { if (Context.getGlobalOptions().LineFilter.empty()) return true; - for (const FileFilter& Filter : Context.getGlobalOptions().LineFilter) { + for (const FileFilter &Filter : Context.getGlobalOptions().LineFilter) { if (FileName.endswith(Filter.Name)) { if (Filter.LineRanges.empty()) return true; @@ -398,26 +395,147 @@ llvm::Regex *ClangTidyDiagnosticConsumer::getHeaderFilter() { return HeaderFilter.get(); } +void ClangTidyDiagnosticConsumer::removeIncompatibleErrors( + SmallVectorImpl &Errors) const { + // Each error is modelled as the set of intervals in which it applies + // replacements. To detect overlapping replacements, we use a sweep line + // algorithm over these sets of intervals. + // An event here consists of the opening or closing of an interval. During the + // proccess, we maintain a counter with the amount of open intervals. If we + // find an endpoint of an interval and this counter is different from 0, it + // means that this interval overlaps with another one, so we set it as + // inapplicable. + struct Event { + // An event can be either the begin or the end of an interval. + enum EventType { + ET_Begin = 1, + ET_End = -1, + }; + + Event(unsigned Begin, unsigned End, EventType Type, unsigned ErrorId, + unsigned ErrorSize) + : Type(Type), ErrorId(ErrorId) { + // The events are going to be sorted by their position. In case of draw: + // + // * If an interval ends at the same position at which other interval + // begins, this is not an overlapping, so we want to remove the ending + // interval before adding the starting one: end events have higher + // priority than begin events. + // + // * If we have several begin points at the same position, we will mark as + // inapplicable the ones that we proccess later, so the first one has to + // be the one with the latest end point, because this one will contain + // all the other intervals. For the same reason, if we have several end + // points in the same position, the last one has to be the one with the + // earliest begin point. In both cases, we sort non-increasingly by the + // position of the complementary. + // + // * In case of two equal intervals, the one whose error is bigger can + // potentially contain the other one, so we want to proccess its begin + // points before and its end points later. + // + // * Finally, if we have two equal intervals whose errors have the same + // size, none of them will be strictly contained inside the other. + // Sorting by ErrorId will guarantee that the begin point of the first + // one will be proccessed before, disallowing the second one, and the + // end point of the first one will also be proccessed before, + // disallowing the first one. + if (Type == ET_Begin) + Priority = std::make_tuple(Begin, Type, -End, -ErrorSize, ErrorId); + else + Priority = std::make_tuple(End, Type, -Begin, ErrorSize, ErrorId); + } + + bool operator<(const Event &Other) const { + return Priority < Other.Priority; + } + + // Determines if this event is the begin or the end of an interval. + EventType Type; + // The index of the error to which the interval that generated this event + // belongs. + unsigned ErrorId; + // The events will be sorted based on this field. + std::tuple Priority; + }; + + // Compute error sizes. + std::vector Sizes; + for (const auto &Error : Errors) { + int Size = 0; + for (const auto &Replace : Error.Fix) + Size += Replace.getLength(); + Sizes.push_back(Size); + } + + // Build events from error intervals. + std::vector Events; + for (unsigned I = 0; I < Errors.size(); ++I) { + for (const auto &Replace : Errors[I].Fix) { + unsigned Begin = Replace.getOffset(); + unsigned End = Begin + Replace.getLength(); + // FIXME: Handle empty intervals, such as those from insertions. + if (Begin == End) + continue; + Events.push_back(Event(Begin, End, Event::ET_Begin, I, Sizes[I])); + Events.push_back(Event(Begin, End, Event::ET_End, I, Sizes[I])); + } + } + std::sort(Events.begin(), Events.end()); + + // Sweep. + std::vector Apply(Errors.size(), true); + int OpenIntervals = 0; + for (const auto &Event : Events) { + if (Event.Type == Event::ET_End) + --OpenIntervals; + // This has to be checked after removing the interval from the count if it + // is an end event, or before adding it if it is a begin event. + if (OpenIntervals != 0) + Apply[Event.ErrorId] = false; + if (Event.Type == Event::ET_Begin) + ++OpenIntervals; + } + assert(OpenIntervals == 0 && "Amount of begin/end points doesn't match"); + + for (unsigned I = 0; I < Errors.size(); ++I) { + if (!Apply[I]) { + Errors[I].Fix.clear(); + Errors[I].Notes.push_back( + ClangTidyMessage("this fix will not be applied because" + " it overlaps with another fix")); + } + } +} + namespace { struct LessClangTidyError { - bool operator()(const ClangTidyError *LHS, const ClangTidyError *RHS) const { - const ClangTidyMessage &M1 = LHS->Message; - const ClangTidyMessage &M2 = RHS->Message; + bool operator()(const ClangTidyError &LHS, const ClangTidyError &RHS) const { + const ClangTidyMessage &M1 = LHS.Message; + const ClangTidyMessage &M2 = RHS.Message; return std::tie(M1.FilePath, M1.FileOffset, M1.Message) < std::tie(M2.FilePath, M2.FileOffset, M2.Message); } }; +struct EqualClangTidyError { + static LessClangTidyError Less; + bool operator()(const ClangTidyError &LHS, const ClangTidyError &RHS) const { + return !Less(LHS, RHS) && !Less(RHS, LHS); + } +}; } // end anonymous namespace // Flushes the internal diagnostics buffer to the ClangTidyContext. void ClangTidyDiagnosticConsumer::finish() { finalizeLastError(); - std::set UniqueErrors; - for (const ClangTidyError &Error : Errors) - UniqueErrors.insert(&Error); - for (const ClangTidyError *Error : UniqueErrors) - Context.storeError(*Error); + std::sort(Errors.begin(), Errors.end(), LessClangTidyError()); + Errors.erase(std::unique(Errors.begin(), Errors.end(), EqualClangTidyError()), + Errors.end()); + removeIncompatibleErrors(Errors); + + for (const ClangTidyError &Error : Errors) + Context.storeError(Error); Errors.clear(); } diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h index a452d68cc847..f13d4fe2ce3a 100644 --- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h +++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h @@ -179,8 +179,8 @@ public: /// /// Setting a non-null pointer here will enable profile collection in /// clang-tidy. - void setCheckProfileData(ProfileData* Profile); - ProfileData* getCheckProfileData() const { return Profile; } + void setCheckProfileData(ProfileData *Profile); + ProfileData *getCheckProfileData() const { return Profile; } private: // Calls setDiagnosticsEngine() and storeError(). @@ -231,9 +231,11 @@ public: private: void finalizeLastError(); + void removeIncompatibleErrors(SmallVectorImpl &Errors) const; + /// \brief Returns the \c HeaderFilter constructed for the options set in the /// context. - llvm::Regex* getHeaderFilter(); + llvm::Regex *getHeaderFilter(); /// \brief Updates \c LastErrorRelatesToUserCode and LastErrorPassesLineFilter /// according to the diagnostic \p Location. diff --git a/clang-tools-extra/unittests/clang-tidy/OverlappingReplacementsTest.cpp b/clang-tools-extra/unittests/clang-tidy/OverlappingReplacementsTest.cpp index 01c43a738c03..b7c4805c38ca 100644 --- a/clang-tools-extra/unittests/clang-tidy/OverlappingReplacementsTest.cpp +++ b/clang-tools-extra/unittests/clang-tidy/OverlappingReplacementsTest.cpp @@ -77,11 +77,12 @@ public: auto *VD = Result.Nodes.getNodeAs(BoundDecl); std::string NewName = newName(VD->getName()); - auto Diag = diag(VD->getLocation(), "refactor") + auto Diag = diag(VD->getLocation(), "refactor %0 into %1") + << VD->getName() << NewName << FixItHint::CreateReplacement( - CharSourceRange::getTokenRange(VD->getLocation(), - VD->getLocation()), - NewName); + CharSourceRange::getTokenRange(VD->getLocation(), + VD->getLocation()), + NewName); class UsageVisitor : public RecursiveASTVisitor { public: @@ -281,7 +282,7 @@ TEST(OverlappingReplacementsTest, ReplacementInsideOtherReplacement) { // Apply the UseCharCheck together with the IfFalseCheck. // - // The 'If' fix is bigger, so that is the one that has to be applied. + // The 'If' fix contains the other, so that is the one that has to be applied. // } else if (int a = 0) { // ^^^ -> char // ~~~~~~~~~ -> false @@ -294,7 +295,9 @@ TEST(OverlappingReplacementsTest, ReplacementInsideOtherReplacement) { } })"; Res = runCheckOnCode(Code); - // FIXME: EXPECT_EQ(CharIfFix, Res); + EXPECT_EQ(CharIfFix, Res); + Res = runCheckOnCode(Code); + EXPECT_EQ(CharIfFix, Res); // Apply the IfFalseCheck with the StartsWithPotaCheck. // @@ -303,7 +306,7 @@ TEST(OverlappingReplacementsTest, ReplacementInsideOtherReplacement) { // ^^^^^^ -> tomato // ~~~~~~~~~~~~~~~ -> false // - // But the refactoring is bigger here: + // But the refactoring is the one that contains the other here: // char potato = 0; // ^^^^^^ -> tomato // if (potato) potato; @@ -318,60 +321,87 @@ TEST(OverlappingReplacementsTest, ReplacementInsideOtherReplacement) { } })"; Res = runCheckOnCode(Code); - // FIXME: EXPECT_EQ(IfStartsFix, Res); - - // Silence warnings. - (void)CharIfFix; - (void)IfStartsFix; + EXPECT_EQ(IfStartsFix, Res); + Res = runCheckOnCode(Code); + EXPECT_EQ(IfStartsFix, Res); } -TEST(OverlappingReplacementsTest, ApplyFullErrorOrNothingWhenOverlapping) { +TEST(OverlappingReplacements, TwoReplacementsInsideOne) { + std::string Res; + const char Code[] = + R"(void f() { + if (int potato = 0) { + int a = 0; + } +})"; + + // The two smallest replacements should not be applied. + // if (int potato = 0) { + // ^^^^^^ -> tomato + // *** -> char + // ~~~~~~~~~~~~~~ -> false + // But other errors from the same checks should not be affected. + // int a = 0; + // *** -> char + const char Fix[] = + R"(void f() { + if (false) { + char a = 0; + } +})"; + Res = runCheckOnCode(Code); + EXPECT_EQ(Fix, Res); + Res = runCheckOnCode(Code); + EXPECT_EQ(Fix, Res); +} + +TEST(OverlappingReplacementsTest, + ApplyAtMostOneOfTheChangesWhenPartialOverlapping) { + std::string Res; + const char Code[] = + R"(void f() { + if (int potato = 0) { + int a = potato; + } +})"; + + // These two replacements overlap, but none of them is completely contained + // inside the other. + // if (int potato = 0) { + // ^^^^^^ -> tomato + // ~~~~~~~~~~~~~~ -> false + // int a = potato; + // ^^^^^^ -> tomato + // + // The 'StartsWithPotaCheck' fix has endpoints inside the 'IfFalseCheck' fix, + // so it is going to be set as inapplicable. The 'if' fix will be applied. + const char IfFix[] = + R"(void f() { + if (false) { + int a = potato; + } +})"; + Res = runCheckOnCode(Code); + EXPECT_EQ(IfFix, Res); +} + +TEST(OverlappingReplacementsTest, TwoErrorsHavePerfectOverlapping) { std::string Res; const char Code[] = R"(void f() { int potato = 0; potato += potato * potato; - if (char this_name_make_this_if_really_long = potato) potato; + if (char a = potato) potato; })"; - // StartsWithPotaCheck will try to refactor 'potato' into 'tomato', - // and EndsWithTatoCheck will try to use 'pomelo'. We have to apply - // either all conversions from one check, or all from the other. - const char StartsFix[] = - R"(void f() { - int tomato = 0; - tomato += tomato * tomato; - if (char this_name_make_this_if_really_long = tomato) tomato; -})"; - const char EndsFix[] = - R"(void f() { - int pomelo = 0; - pomelo += pomelo * pomelo; - if (char this_name_make_this_if_really_long = pomelo) pomelo; -})"; - // In case of overlapping, we will prioritize the biggest fix. However, these - // two fixes have the same size and position, so we don't know yet which one - // will have preference. + // StartsWithPotaCheck will try to refactor 'potato' into 'tomato', and + // EndsWithTatoCheck will try to use 'pomelo'. Both fixes have the same set of + // ranges. This is a corner case of one error completely containing another: + // the other completely contains the first one as well. Both errors are + // discarded. + Res = runCheckOnCode(Code); - // FIXME: EXPECT_TRUE(Res == StartsFix || Res == EndsFix); - - // StartsWithPotaCheck will try to refactor 'potato' into 'tomato', but - // replacing the 'if' condition is a bigger change than all the refactoring - // changes together (48 vs 36), so this is the one that is going to be - // applied. - const char IfFix[] = - R"(void f() { - int potato = 0; - potato += potato * potato; - if (true) potato; -})"; - Res = runCheckOnCode(Code); - // FIXME: EXPECT_EQ(IfFix, Res); - - // Silence warnings. - (void)StartsFix; - (void)EndsFix; - (void)IfFix; + EXPECT_EQ(Code, Res); } } // namespace test