diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp --- a/clang-tools-extra/clang-tidy/ClangTidy.cpp +++ b/clang-tools-extra/clang-tidy/ClangTidy.cpp @@ -177,6 +177,17 @@ FixLoc = getLocation(FixAbsoluteFilePath, Repl.getOffset()); FixLocations.push_back(std::make_pair(FixLoc, CanBeApplied)); } + for (const auto &FR : FileAndReplacements.second.affected_ranges()) { + if (!FR.isValid()) + continue; + SmallString<128> FixAbsoluteFilePath = FR.getFilePath(); + Files.makeAbsolutePath(FixAbsoluteFilePath); + tooling::FileRange R(FixAbsoluteFilePath, FR.getRange()); + Replacements &Replacements = FileReplacements[R.getFilePath()]; + // This should never fail as there addReformatRange only returns an + // error if the files mismatch. + cantFail(Replacements.addReformatRange(R)); + } } } reportFix(Diag, Error.Message.Fix); diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp --- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp +++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp @@ -93,16 +93,22 @@ assert(Range.getBegin().isFileID() && Range.getEnd().isFileID() && "Only file locations supported in fix-it hints."); - tooling::Replacement Replacement(Loc.getManager(), Range, - FixIt.CodeToInsert); - llvm::Error Err = - DiagWithFix->Fix[Replacement.getFilePath()].add(Replacement); - // FIXME: better error handling (at least, don't let other replacements be - // applied). - if (Err) { - llvm::errs() << "Fix conflicts with existing fix! " - << llvm::toString(std::move(Err)) << "\n"; - assert(false && "Fix conflicts with existing fix!"); + tooling::FileRange FRange(Loc.getManager(), Range); + tooling::Replacements &Replacements = + DiagWithFix->Fix[FRange.getFilePath()]; + + if (FixIt.isReformatFixit()) { + cantFail(Replacements.addReformatRange(FRange)); + } else { + tooling::Replacement Replacement(FRange, FixIt.CodeToInsert); + llvm::Error Err = Replacements.add(Replacement); + // FIXME: better error handling (at least, don't let other replacements + // be applied). + if (Err) { + llvm::errs() << "Fix conflicts with existing fix! " + << llvm::toString(std::move(Err)) << "\n"; + assert(false && "Fix conflicts with existing fix!"); + } } } } diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp --- a/clang-tools-extra/clangd/Diagnostics.cpp +++ b/clang-tools-extra/clangd/Diagnostics.cpp @@ -631,8 +631,22 @@ // Copy as we may modify the ranges. auto FixIts = Info.getFixItHints().vec(); + + // If this doesn't store a value, there are no code modifying fix-its + // attached to this diagnostic. + // If it stores a nullptr, there are multiple code modifiying fix-its, + // otherwise it stores a pointer to the only code modifying fix-it attacahed + // to this diagnostic. + Optional SingleFixIt; + llvm::SmallVector Edits; for (auto &FixIt : FixIts) { + + // These don't modify code and aren't supported by clangd. + if (FixIt.isReformatFixit()) + continue; + + SingleFixIt = SingleFixIt ? nullptr : &FixIt; // Allow fixits within a single macro-arg expansion to be applied. // This can be incorrect if the argument is expanded multiple times in // different contexts. Hopefully this is rare! @@ -654,10 +668,14 @@ Edits.push_back(toTextEdit(FixIt, SM, *LangOpts)); } + // If this has no value, then no code modifying fix-its were attached + if (!SingleFixIt) + return false; + llvm::SmallString<64> Message; // If requested and possible, create a message like "change 'foo' to 'bar'". - if (SyntheticMessage && FixIts.size() == 1) { - const auto &FixIt = FixIts.front(); + if (SyntheticMessage && *SingleFixIt) { + const auto &FixIt = **SingleFixIt; bool Invalid = false; llvm::StringRef Remove = Lexer::getSourceText(FixIt.RemoveRange, SM, *LangOpts, &Invalid); diff --git a/clang/include/clang/Basic/Diagnostic.h b/clang/include/clang/Basic/Diagnostic.h --- a/clang/include/clang/Basic/Diagnostic.h +++ b/clang/include/clang/Basic/Diagnostic.h @@ -91,6 +91,10 @@ return !RemoveRange.isValid(); } + /// Signifies that the code modification doesn't modify code, just a hint that + /// it should be reformatted. + bool isReformatFixit() const { return RemoveRange == InsertFromRange; } + /// Create a code modification hint that inserts the given /// code string at a specific location. static FixItHint CreateInsertion(SourceLocation InsertionLoc, @@ -142,6 +146,15 @@ StringRef Code) { return CreateReplacement(CharSourceRange::getTokenRange(RemoveRange), Code); } + + /// Creates a Fix that doesn't modify code, just hints that \p Range needs + /// reformatting. + static FixItHint CreateReformatFixIt(SourceRange Range) { + FixItHint Hint; + Hint.RemoveRange = Hint.InsertFromRange = + CharSourceRange::getCharRange(Range); + return Hint; + } }; struct DiagnosticStorage { diff --git a/clang/include/clang/Basic/SourceLocation.h b/clang/include/clang/Basic/SourceLocation.h --- a/clang/include/clang/Basic/SourceLocation.h +++ b/clang/include/clang/Basic/SourceLocation.h @@ -294,6 +294,15 @@ bool isInvalid() const { return !isValid(); } }; +inline bool operator==(const CharSourceRange &LHS, const CharSourceRange &RHS) { + return LHS.isCharRange() == RHS.isCharRange() && + LHS.getAsRange() == RHS.getAsRange(); +} + +inline bool operator!=(const CharSourceRange &LHS, const CharSourceRange &RHS) { + return !(LHS == RHS); +} + /// Represents an unpacked "presumed" location which can be presented /// to the user. /// diff --git a/clang/include/clang/Tooling/Core/Replacement.h b/clang/include/clang/Tooling/Core/Replacement.h --- a/clang/include/clang/Tooling/Core/Replacement.h +++ b/clang/include/clang/Tooling/Core/Replacement.h @@ -76,6 +76,41 @@ unsigned Length = 0; }; +class FileRange { + +public: + FileRange(); + FileRange(StringRef FilePath, Range RangeInFile); + FileRange(StringRef FilePath, unsigned Offset, unsigned Length); + + FileRange(const SourceManager &Sources, SourceLocation Start, + unsigned Length); + + FileRange(const SourceManager &Sources, const CharSourceRange &Range, + const LangOptions &LangOpts = LangOptions()); + + template + FileRange(const SourceManager &Sources, const Node &ASTNode, + const LangOptions &LangOpts = LangOptions()); + + StringRef getFilePath() const { return FilePath; } + const Range &getRange() const { return RangeInFile; } + unsigned getOffset() const { return RangeInFile.getOffset(); } + unsigned getLength() const { return RangeInFile.getLength(); } + + bool isValid() const; + +private: + void setFromSourceLocation(const SourceManager &Sources, SourceLocation Start, + unsigned Length); + void setFromSourceRange(const SourceManager &Sources, + const CharSourceRange &Range, + const LangOptions &LangOpts); + + std::string FilePath; + Range RangeInFile; +}; + /// A text replacement. /// /// Represents a SourceManager independent replacement of a range of text in a @@ -85,6 +120,8 @@ /// Creates an invalid (not applicable) replacement. Replacement(); + Replacement(FileRange FileRange, StringRef ReplacementText); + /// Creates a replacement of the range [Offset, Offset+Length) in /// FilePath with ReplacementText. /// @@ -108,7 +145,9 @@ template Replacement(const SourceManager &Sources, const Node &NodeToReplace, StringRef ReplacementText, - const LangOptions &LangOpts = LangOptions()); + const LangOptions &LangOpts = LangOptions()) + : ReplacementRange(Sources, NodeToReplace, LangOpts), + ReplacementText(ReplacementText) {} /// Returns whether this replacement can be applied to a file. /// @@ -117,10 +156,11 @@ /// Accessors. /// @{ - StringRef getFilePath() const { return FilePath; } + StringRef getFilePath() const { return ReplacementRange.getFilePath(); } unsigned getOffset() const { return ReplacementRange.getOffset(); } unsigned getLength() const { return ReplacementRange.getLength(); } StringRef getReplacementText() const { return ReplacementText; } + const FileRange &getFileRange() const { return ReplacementRange; } /// @} /// Applies the replacement on the Rewriter. @@ -130,15 +170,7 @@ std::string toString() const; private: - void setFromSourceLocation(const SourceManager &Sources, SourceLocation Start, - unsigned Length, StringRef ReplacementText); - void setFromSourceRange(const SourceManager &Sources, - const CharSourceRange &Range, - StringRef ReplacementText, - const LangOptions &LangOpts); - - std::string FilePath; - Range ReplacementRange; + FileRange ReplacementRange; std::string ReplacementText; }; @@ -197,6 +229,15 @@ llvm::Optional ExistingReplacement; }; +/// Less-than operator between two Ranges. +bool operator<(const Range &LHS, const Range &RHS); + +/// Less-than operator between two FileRanges. +bool operator<(const FileRange &LHS, const FileRange &RHS); + +/// Equal-to operator between two FileRanges. +bool operator==(const FileRange &LHS, const FileRange &RHS); + /// Less-than operator between two Replacements. bool operator<(const Replacement &LHS, const Replacement &RHS); @@ -209,6 +250,7 @@ class Replacements { private: using ReplacementsImpl = std::set; + using AffectedImpl = llvm::SmallVector; public: using const_iterator = ReplacementsImpl::const_iterator; @@ -257,6 +299,8 @@ /// category of replacements. llvm::Error add(const Replacement &R); + llvm::Error addReformatRange(const FileRange &FormatRange); + /// Merges \p Replaces into the current replacements. \p Replaces /// refers to code after applying the current replacements. LLVM_NODISCARD Replacements merge(const Replacements &Replaces) const; @@ -271,6 +315,8 @@ unsigned size() const { return Replaces.size(); } + llvm::Optional getFilePath() const; + void clear() { Replaces.clear(); } bool empty() const { return Replaces.empty(); } @@ -283,8 +329,20 @@ const_reverse_iterator rend() const { return Replaces.rend(); } + AffectedImpl::const_iterator affected_ranges_begin() const { + return Affected.begin(); + } + + AffectedImpl::const_iterator affected_ranges_end() const { + return Affected.end(); + } + + llvm::iterator_range affected_ranges() const { + return {affected_ranges_begin(), affected_ranges_end()}; + } + bool operator==(const Replacements &RHS) const { - return Replaces == RHS.Replaces; + return Replaces == RHS.Replaces && Affected == RHS.Affected; } private: @@ -308,6 +366,7 @@ mergeIfOrderIndependent(const Replacement &R) const; ReplacementsImpl Replaces; + AffectedImpl Affected; }; /// Apply all replacements in \p Replaces to the Rewriter \p Rewrite. @@ -356,12 +415,11 @@ const std::map &FileToReplaces); template -Replacement::Replacement(const SourceManager &Sources, - const Node &NodeToReplace, StringRef ReplacementText, - const LangOptions &LangOpts) { +FileRange::FileRange(const SourceManager &Sources, const Node &ASTNode, + const LangOptions &LangOpts) { const CharSourceRange Range = - CharSourceRange::getTokenRange(NodeToReplace->getSourceRange()); - setFromSourceRange(Sources, Range, ReplacementText, LangOpts); + CharSourceRange::getTokenRange(ASTNode->getSourceRange()); + setFromSourceRange(Sources, Range, LangOpts); } } // namespace tooling diff --git a/clang/lib/Frontend/DiagnosticRenderer.cpp b/clang/lib/Frontend/DiagnosticRenderer.cpp --- a/clang/lib/Frontend/DiagnosticRenderer.cpp +++ b/clang/lib/Frontend/DiagnosticRenderer.cpp @@ -59,14 +59,19 @@ static void mergeFixits(ArrayRef FixItHints, const SourceManager &SM, const LangOptions &LangOpts, SmallVectorImpl &MergedFixits) { + // Never expext more than one range per diag. + SmallVector Reformats; edit::Commit commit(SM, LangOpts); for (const auto &Hint : FixItHints) if (Hint.CodeToInsert.empty()) { - if (Hint.InsertFromRange.isValid()) - commit.insertFromRange(Hint.RemoveRange.getBegin(), - Hint.InsertFromRange, /*afterToken=*/false, - Hint.BeforePreviousInsertions); - else + if (Hint.InsertFromRange.isValid()) { + if (Hint.isReformatFixit()) + Reformats.push_back(&Hint); + else + commit.insertFromRange(Hint.RemoveRange.getBegin(), + Hint.InsertFromRange, /*afterToken=*/false, + Hint.BeforePreviousInsertions); + } else commit.remove(Hint.RemoveRange); } else { if (Hint.RemoveRange.isTokenRange() || @@ -82,6 +87,9 @@ FixitReceiver Rec(MergedFixits); Editor.applyRewrites(Rec); } + for (const auto *Hint : Reformats) { + MergedFixits.push_back(*Hint); + } } void DiagnosticRenderer::emitDiagnostic(FullSourceLoc Loc, diff --git a/clang/lib/Frontend/Rewrite/FixItRewriter.cpp b/clang/lib/Frontend/Rewrite/FixItRewriter.cpp --- a/clang/lib/Frontend/Rewrite/FixItRewriter.cpp +++ b/clang/lib/Frontend/Rewrite/FixItRewriter.cpp @@ -152,16 +152,17 @@ // Make sure that we can perform all of the modifications we // in this diagnostic. edit::Commit commit(Editor); - for (unsigned Idx = 0, Last = Info.getNumFixItHints(); - Idx < Last; ++Idx) { - const FixItHint &Hint = Info.getFixItHint(Idx); - + unsigned NonApplicableFixits = 0; + for (const FixItHint &Hint : Info.getFixItHints()) { if (Hint.CodeToInsert.empty()) { - if (Hint.InsertFromRange.isValid()) - commit.insertFromRange(Hint.RemoveRange.getBegin(), - Hint.InsertFromRange, /*afterToken=*/false, - Hint.BeforePreviousInsertions); - else + if (Hint.InsertFromRange.isValid()) { + if (Hint.isReformatFixit()) + ++NonApplicableFixits; + else + commit.insertFromRange(Hint.RemoveRange.getBegin(), + Hint.InsertFromRange, /*afterToken=*/false, + Hint.BeforePreviousInsertions); + } else commit.remove(Hint.RemoveRange); } else { if (Hint.RemoveRange.isTokenRange() || @@ -172,7 +173,8 @@ /*afterToken=*/false, Hint.BeforePreviousInsertions); } } - bool CanRewrite = Info.getNumFixItHints() > 0 && commit.isCommitable(); + bool CanRewrite = + Info.getNumFixItHints() > NonApplicableFixits && commit.isCommitable(); if (!CanRewrite) { if (Info.getNumFixItHints() > 0) diff --git a/clang/lib/Tooling/Core/Replacement.cpp b/clang/lib/Tooling/Core/Replacement.cpp --- a/clang/lib/Tooling/Core/Replacement.cpp +++ b/clang/lib/Tooling/Core/Replacement.cpp @@ -22,6 +22,7 @@ #include "clang/Rewrite/Core/RewriteBuffer.h" #include "clang/Rewrite/Core/Rewriter.h" #include "llvm/ADT/IntrusiveRefCntPtr.h" +#include "llvm/ADT/Optional.h" #include "llvm/ADT/SmallPtrSet.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/Error.h" @@ -42,32 +43,57 @@ static const char * const InvalidLocation = ""; -Replacement::Replacement() : FilePath(InvalidLocation) {} +FileRange::FileRange() : FilePath(InvalidLocation), RangeInFile(0, 0) {} + +FileRange::FileRange(StringRef FilePath, Range RangeInFile) + : FilePath(FilePath), RangeInFile(RangeInFile) {} +FileRange::FileRange(StringRef FilePath, unsigned Offset, unsigned Length) + : FileRange(FilePath, Range(Offset, Length)) {} + +FileRange::FileRange(const SourceManager &Sources, SourceLocation Start, + unsigned Length) { + setFromSourceLocation(Sources, Start, Length); +} + +FileRange::FileRange(const SourceManager &Sources, const CharSourceRange &Range, + const LangOptions &LangOpts) { + setFromSourceRange(Sources, Range, LangOpts); +} + +bool FileRange::isValid() const { return FilePath != InvalidLocation; } + +void FileRange::setFromSourceLocation(const SourceManager &Sources, + SourceLocation Start, unsigned Length) { + const std::pair DecomposedLocation = + Sources.getDecomposedLoc(Start); + const FileEntry *Entry = Sources.getFileEntryForID(DecomposedLocation.first); + this->FilePath = std::string(Entry ? Entry->getName() : InvalidLocation); + this->RangeInFile = {DecomposedLocation.second, Length}; +} + +Replacement::Replacement() : ReplacementRange() {} + +Replacement::Replacement(FileRange FileRange, StringRef ReplacementText) + : ReplacementRange(FileRange), ReplacementText(ReplacementText) {} Replacement::Replacement(StringRef FilePath, unsigned Offset, unsigned Length, StringRef ReplacementText) - : FilePath(std::string(FilePath)), ReplacementRange(Offset, Length), - ReplacementText(std::string(ReplacementText)) {} + : Replacement(FileRange(FilePath, Offset, Length), ReplacementText) {} Replacement::Replacement(const SourceManager &Sources, SourceLocation Start, - unsigned Length, StringRef ReplacementText) { - setFromSourceLocation(Sources, Start, Length, ReplacementText); -} + unsigned Length, StringRef ReplacementText) + : Replacement(FileRange(Sources, Start, Length), ReplacementText) {} Replacement::Replacement(const SourceManager &Sources, const CharSourceRange &Range, - StringRef ReplacementText, - const LangOptions &LangOpts) { - setFromSourceRange(Sources, Range, ReplacementText, LangOpts); -} + StringRef ReplacementText, const LangOptions &LangOpts) + : Replacement(FileRange(Sources, Range, LangOpts), ReplacementText) {} -bool Replacement::isApplicable() const { - return FilePath != InvalidLocation; -} +bool Replacement::isApplicable() const { return ReplacementRange.isValid(); } bool Replacement::apply(Rewriter &Rewrite) const { SourceManager &SM = Rewrite.getSourceMgr(); - auto Entry = SM.getFileManager().getFile(FilePath); + auto Entry = SM.getFileManager().getFile(getFilePath()); if (!Entry) return false; @@ -87,47 +113,45 @@ std::string Replacement::toString() const { std::string Result; llvm::raw_string_ostream Stream(Result); - Stream << FilePath << ": " << ReplacementRange.getOffset() << ":+" - << ReplacementRange.getLength() << ":\"" << ReplacementText << "\""; + Stream << getFilePath() << ": " << getOffset() << ":+" << getLength() << ":\"" + << ReplacementText << "\""; return Stream.str(); } namespace clang { namespace tooling { -bool operator<(const Replacement &LHS, const Replacement &RHS) { +bool operator<(const Range &LHS, const Range &RHS) { if (LHS.getOffset() != RHS.getOffset()) return LHS.getOffset() < RHS.getOffset(); + return LHS.getLength() < RHS.getLength(); +} - if (LHS.getLength() != RHS.getLength()) - return LHS.getLength() < RHS.getLength(); +bool operator<(const FileRange &LHS, const FileRange &RHS) { + if (!(LHS.getRange() == RHS.getRange())) + return LHS.getRange() < RHS.getRange(); + return LHS.getFilePath() < RHS.getFilePath(); +} + +bool operator==(const FileRange &LHS, const FileRange &RHS) { + return LHS.getRange() == RHS.getRange() && + LHS.getFilePath() == RHS.getFilePath(); +} - if (LHS.getFilePath() != RHS.getFilePath()) - return LHS.getFilePath() < RHS.getFilePath(); +bool operator<(const Replacement &LHS, const Replacement &RHS) { + if (!(LHS.getFileRange() == RHS.getFileRange())) + return LHS.getFileRange() < RHS.getFileRange(); return LHS.getReplacementText() < RHS.getReplacementText(); } bool operator==(const Replacement &LHS, const Replacement &RHS) { - return LHS.getOffset() == RHS.getOffset() && - LHS.getLength() == RHS.getLength() && - LHS.getFilePath() == RHS.getFilePath() && + return LHS.getFileRange() == RHS.getFileRange() && LHS.getReplacementText() == RHS.getReplacementText(); } } // namespace tooling } // namespace clang -void Replacement::setFromSourceLocation(const SourceManager &Sources, - SourceLocation Start, unsigned Length, - StringRef ReplacementText) { - const std::pair DecomposedLocation = - Sources.getDecomposedLoc(Start); - const FileEntry *Entry = Sources.getFileEntryForID(DecomposedLocation.first); - this->FilePath = std::string(Entry ? Entry->getName() : InvalidLocation); - this->ReplacementRange = Range(DecomposedLocation.second, Length); - this->ReplacementText = std::string(ReplacementText); -} - // FIXME: This should go into the Lexer, but we need to figure out how // to handle ranges for refactoring in general first - there is no obvious // good way how to integrate this into the Lexer yet. @@ -144,13 +168,11 @@ return End.second - Start.second; } -void Replacement::setFromSourceRange(const SourceManager &Sources, - const CharSourceRange &Range, - StringRef ReplacementText, - const LangOptions &LangOpts) { +void FileRange::setFromSourceRange(const SourceManager &Sources, + const CharSourceRange &Range, + const LangOptions &LangOpts) { setFromSourceLocation(Sources, Sources.getSpellingLoc(Range.getBegin()), - getRangeSize(Sources, Range, LangOpts), - ReplacementText); + getRangeSize(Sources, Range, LangOpts)); } Replacement @@ -242,11 +264,31 @@ R, *Replaces.begin()); } +llvm::Optional Replacements::getFilePath() const { + if (!Replaces.empty()) + return Replaces.begin()->getFilePath(); + if (!Affected.empty()) + return Affected.front().getFilePath(); + return None; +} + +llvm::Error Replacements::addReformatRange(const FileRange &FormatRange) { + if (llvm::Optional Filepath = getFilePath()) { + if (FormatRange.getFilePath() != Filepath) + return llvm::make_error( + replacement_error::wrong_file_path); + } + Affected.push_back(FormatRange); + return llvm::Error::success(); +} + llvm::Error Replacements::add(const Replacement &R) { // Check the file path. - if (!Replaces.empty() && R.getFilePath() != Replaces.begin()->getFilePath()) - return llvm::make_error( - replacement_error::wrong_file_path, R, *Replaces.begin()); + if (llvm::Optional Filepath = getFilePath()) { + if (R.getFilePath() != Filepath) + return llvm::make_error( + replacement_error::wrong_file_path, R, *Replaces.begin()); + } // Special-case header insertions. if (R.getOffset() == std::numeric_limits::max()) { @@ -540,6 +582,11 @@ Shift += Length - R.getLength(); ChangedRanges.push_back(Range(Offset, Length)); } + for (const auto &A : Affected) { + unsigned Start = getShiftedCodePosition(A.getOffset()); + unsigned End = getShiftedCodePosition(A.getOffset() + A.getLength()); + ChangedRanges.push_back(Range(Start, End - Start)); + } return combineAndSortRanges(ChangedRanges); } diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp --- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp +++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp @@ -1167,9 +1167,10 @@ // This is cobbed from clang::Rewrite::FixItRewriter. if (fixit.CodeToInsert.empty()) { if (fixit.InsertFromRange.isValid()) { - commit.insertFromRange(fixit.RemoveRange.getBegin(), - fixit.InsertFromRange, /*afterToken=*/false, - fixit.BeforePreviousInsertions); + if (!fixit.isReformatFixit()) + commit.insertFromRange(fixit.RemoveRange.getBegin(), + fixit.InsertFromRange, /*afterToken=*/false, + fixit.BeforePreviousInsertions); return; } commit.remove(fixit.RemoveRange);