Index: clang-tidy/ClangTidyDiagnosticConsumer.h =================================================================== --- clang-tidy/ClangTidyDiagnosticConsumer.h +++ clang-tidy/ClangTidyDiagnosticConsumer.h @@ -179,8 +179,8 @@ /// /// 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,21 @@ private: void finalizeLastError(); + using ErrorInterval = std::pair; + using ErrorIntervals = std::vector; + enum OverlappingKind { + OK_Disjoint, + OK_FirstInsideSecond, + OK_SecondInsideFirst, + OK_Overlap, + }; + OverlappingKind getOverlappingKind(const ErrorIntervals &First, + const ErrorIntervals &Second) const; + 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. Index: clang-tidy/ClangTidyDiagnosticConsumer.cpp =================================================================== --- clang-tidy/ClangTidyDiagnosticConsumer.cpp +++ clang-tidy/ClangTidyDiagnosticConsumer.cpp @@ -22,6 +22,7 @@ #include "clang/Basic/DiagnosticOptions.h" #include "clang/Frontend/DiagnosticRenderer.h" #include "llvm/ADT/SmallString.h" +#include #include #include using namespace clang; @@ -146,8 +147,7 @@ } 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 +222,7 @@ return CurrentOptions; } -void ClangTidyContext::setCheckProfileData(ProfileData *P) { - Profile = P; -} +void ClangTidyContext::setCheckProfileData(ProfileData *P) { Profile = P; } GlobList &ClangTidyContext::getChecksFilter() { assert(CheckFilter != nullptr); @@ -296,16 +294,16 @@ // 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 +338,7 @@ 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 +396,152 @@ return HeaderFilter.get(); } +ClangTidyDiagnosticConsumer::OverlappingKind +ClangTidyDiagnosticConsumer::getOverlappingKind( + const ErrorIntervals &First, const ErrorIntervals &Second) const { + // We are using a sweep line algorithm over both sets of intervals. An event + // here consists of the opening or closing of an interval from any of the + // two sets, and at which position. This way, for each range between two + // events, we can know if there are intervals from a concrete set that are + // covering that range. + // + // All the events in the same position are handled together. This way, if + // one of the sets has two consecutive intervals (like "[a, b)[b, c)"), we + // won't erroneously think that there is a hole in the middle. + // + // FIXME: This approach doesn't cope well with empty intervals (such as those + // from insertions). For now, we are blind to them, but we could pretend them + // to be one-unit length intervals to check out how this works. + struct Event { + bool operator<(const Event &E) const { return Pos < E.Pos; } + // File offset at which this event happens. + unsigned Pos; + // This event represents the beggining or the ending of an interval (1 or + // -1 respectively). + int Type; + // Identifies the error (0 or 1) that owns the interval related with this + // event. + int ErrorId; + }; + std::priority_queue Queue; + for (const auto &Interval : First) { + Queue.push({Interval.first, 1, 0}); + Queue.push({Interval.second, -1, 0}); + } + for (const auto &Interval : Second) { + Queue.push({Interval.first, 1, 1}); + Queue.push({Interval.second, -1, 1}); + } + + // Keep track of how many open intervals from each set we have. + int Count[2] = {0, 0}; + + // A range between events can either be covered by the intervals or not. + enum { + Empty = 0, + Covered = 1, + }; + // Keep track of the different coverage situations that have been spotted + // during the process: Coverage[Covered][Empty] will tell if there exists any + // range that is covered by the first set but not by the second one, + // Coverage[Covered][Covered] will tell if there is a range covered by both + // sets, etc. + bool Coverage[2][2] = {{false, false}, {false, false}}; + + while (!Queue.empty()) { + unsigned CurrPos = Queue.top().Pos; + do { + const Event &E = Queue.top(); + Count[E.ErrorId] += E.Type; + Queue.pop(); + } while (!Queue.empty() && Queue.top().Pos == CurrPos); + Coverage[Count[0] != 0][Count[1] != 0] = true; + } + + // If they never appeared at the same position, there is no overlapping. + if (!Coverage[Covered][Covered]) + return OK_Disjoint; + + // If both are true, then we have partial overlapping. If both are false, + // that means that both interval lists cover exactly the same ranges. + if (Coverage[Empty][Covered] == Coverage[Covered][Empty]) + return OK_Overlap; + + // There are points covered by the second list that are not covered by the + // first one, but not the other way around. + if (Coverage[Empty][Covered]) + return OK_FirstInsideSecond; + + return OK_SecondInsideFirst; +} + +void ClangTidyDiagnosticConsumer::removeIncompatibleErrors( + SmallVectorImpl &Errors) const { + // Build the sets of intervals. + std::vector IntervalsList; + for (const auto &Error : Errors) { + ErrorIntervals Intervals; + for (const auto &Replace : Error.Fix) { + Intervals.push_back(std::make_pair( + Replace.getOffset(), Replace.getOffset() + Replace.getLength())); + } + IntervalsList.push_back(std::move(Intervals)); + } + + // If an error overlaps with another one, we don't want to apply any of them, + // with an exception: if an error is completely contained inside other error, + // we are still allowed to apply the biggest one. + int NumErrors = Errors.size(); + std::vector Apply(NumErrors, true); + for (int I = 0; I < NumErrors; ++I) { + for (int J = I + 1; J < NumErrors; ++J) { + OverlappingKind Kind = + getOverlappingKind(IntervalsList[I], IntervalsList[J]); + if (Kind == OK_Overlap || Kind == OK_FirstInsideSecond) + Apply[I] = false; + if (Kind == OK_Overlap || Kind == OK_SecondInsideFirst) + Apply[J] = false; + } + } + + for (int I = 0; I < NumErrors; ++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(); } Index: unittests/clang-tidy/OverlappingReplacementsTest.cpp =================================================================== --- unittests/clang-tidy/OverlappingReplacementsTest.cpp +++ unittests/clang-tidy/OverlappingReplacementsTest.cpp @@ -77,11 +77,12 @@ 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 @@ // 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,7 @@ } })"; Res = runCheckOnCode(Code); - // FIXME: EXPECT_EQ(CharIfFix, Res); + EXPECT_EQ(CharIfFix, Res); // Apply the IfFalseCheck with the StartsWithPotaCheck. // @@ -303,7 +304,7 @@ // ^^^^^^ -> 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 +319,73 @@ } })"; Res = runCheckOnCode(Code); - // FIXME: EXPECT_EQ(IfStartsFix, Res); - - // Silence warnings. - (void)CharIfFix; - (void)IfStartsFix; + EXPECT_EQ(IfStartsFix, Res); } -TEST(OverlappingReplacementsTest, ApplyFullErrorOrNothingWhenOverlapping) { - std::string Res; +TEST(OverlappingReplacements, TwoReplacementsInsideOne) { const char Code[] = R"(void f() { - int potato = 0; - potato += potato * potato; - if (char this_name_make_this_if_really_long = potato) potato; + if (int potato = 0) { + int a = 0; + } })"; - // 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[] = + // 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() { - int tomato = 0; - tomato += tomato * tomato; - if (char this_name_make_this_if_really_long = tomato) tomato; + if (false) { + char a = 0; + } })"; - const char EndsFix[] = + std::string Res = + runCheckOnCode(Code); + EXPECT_EQ(Fix, Res); +} + +TEST(OverlappingReplacementsTest, DiscardBothChangesWhenPartialOverlapping) { + const char Code[] = R"(void f() { - int pomelo = 0; - pomelo += pomelo * pomelo; - if (char this_name_make_this_if_really_long = pomelo) pomelo; + if (int potato = 0) { + int a = potato; + } })"; - // 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. - 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[] = + // These two replacements overlap, but none of them is completely contained + // inside the other. Both of them are discarded. + // if (int potato = 0) { + // ^^^^^^ -> tomato + // ~~~~~~~~~~~~~~ -> false + // int a = potato; + // ^^^^^^ -> tomato + std::string Res = runCheckOnCode(Code); + EXPECT_EQ(Code, Res); +} + +TEST(OverlappingReplacementsTest, TwoErrorsHavePerfectOverlapping) { + std::string Res; + const char Code[] = R"(void f() { int potato = 0; potato += potato * potato; - if (true) potato; + if (char a = potato) potato; })"; - Res = runCheckOnCode(Code); - // FIXME: EXPECT_EQ(IfFix, Res); - // Silence warnings. - (void)StartsFix; - (void)EndsFix; - (void)IfFix; + // 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); + EXPECT_EQ(Code, Res); + Res = runCheckOnCode(Code); + EXPECT_EQ(Code, Res); } } // namespace test