[clang-tidy] Add a Standalone diagnostics mode to clang-tidy

Adds a flag to `ClangTidyContext` that is used to indicate to checks that fixes will only be applied one at a time.
This is to indicate to checks that each fix emitted should not depend on any other fixes emitted across the translation unit.
I've currently implemented the `IncludeInserter`, `LoopConvertCheck` and `PreferMemberInitializerCheck` to use these support these modes.

Reasoning behind this is in use cases like `clangd` it's only possible to apply one fix at a time.
For include inserter checks, the include is only added once for the first diagnostic that requires it, this will result in subsequent fixes not having the included needed.

A similar issue is seen in the `PreferMemberInitializerCheck` where the `:` will only be added for the first member that needs fixing.

Fixes emitted in `StandaloneDiagsMode` will likely result in malformed code if they are applied all together, conversely fixes currently emitted may result in malformed code if they are applied one at a time.
For this reason invoking `clang-tidy` from the binary will always with `StandaloneDiagsMode` disabled, However using it as a library its possible to select the mode you wish to use, `clangd` always selects `StandaloneDiagsMode`.

This is an example of the current behaviour failing
```lang=c++
struct Foo {
  int A, B;
  Foo(int D, int E) {
    A = D;
    B = E; // Fix Here
  }
};
```
Incorrectly transformed to:
```lang=c++
struct Foo {
  int A, B;
  Foo(int D, int E), B(E) {
    A = D;
     // Fix Here
  }
};
```
In `StandaloneDiagsMode`, it gets transformed to:
```lang=c++
struct Foo {
  int A, B;
  Foo(int D, int E) : B(E) {
    A = D;
     // Fix Here
  }
};
```

Reviewed By: sammccall

Differential Revision: https://reviews.llvm.org/D97121
This commit is contained in:
Nathan James 2022-04-16 09:53:32 +01:00
parent e75d8b7037
commit b859c39c40
No known key found for this signature in database
GPG Key ID: CC007AFCDA90AA5F
23 changed files with 170 additions and 28 deletions

View File

@ -417,6 +417,11 @@ protected:
StringRef getCurrentMainFile() const { return Context->getCurrentFile(); }
/// Returns the language options from the context.
const LangOptions &getLangOpts() const { return Context->getLangOpts(); }
/// Returns true when the check is run in a use case when only 1 fix will be
/// applied at a time.
bool areDiagsSelfContained() const {
return Context->areDiagsSelfContained();
}
};
/// Read a named option from the ``Context`` and parse it as a bool.

View File

@ -162,7 +162,8 @@ ClangTidyContext::ClangTidyContext(
bool AllowEnablingAnalyzerAlphaCheckers)
: DiagEngine(nullptr), OptionsProvider(std::move(OptionsProvider)),
Profile(false),
AllowEnablingAnalyzerAlphaCheckers(AllowEnablingAnalyzerAlphaCheckers) {
AllowEnablingAnalyzerAlphaCheckers(AllowEnablingAnalyzerAlphaCheckers),
SelfContainedDiags(false) {
// Before the first translation unit we can get errors related to command-line
// parsing, use empty string for the file name in this case.
setCurrentFile("");

View File

@ -187,6 +187,10 @@ public:
return AllowEnablingAnalyzerAlphaCheckers;
}
void setSelfContainedDiags(bool Value) { SelfContainedDiags = Value; }
bool areDiagsSelfContained() const { return SelfContainedDiags; }
using DiagLevelAndFormatString = std::pair<DiagnosticIDs::Level, std::string>;
DiagLevelAndFormatString getDiagLevelAndFormatString(unsigned DiagnosticID,
SourceLocation Loc) {
@ -223,6 +227,8 @@ private:
bool AllowEnablingAnalyzerAlphaCheckers;
bool SelfContainedDiags;
NoLintDirectiveHandler NoLintHandler;
};

View File

@ -27,7 +27,8 @@ StringFindStartswithCheck::StringFindStartswithCheck(StringRef Name,
StringLikeClasses(utils::options::parseStringList(
Options.get("StringLikeClasses", "::std::basic_string"))),
IncludeInserter(Options.getLocalOrGlobal("IncludeStyle",
utils::IncludeSorter::IS_LLVM)),
utils::IncludeSorter::IS_LLVM),
areDiagsSelfContained()),
AbseilStringsMatchHeader(
Options.get("AbseilStringsMatchHeader", "absl/strings/match.h")) {}

View File

@ -43,8 +43,8 @@ ImplicitWideningOfMultiplicationResultCheck::
Options.get("UseCXXStaticCastsInCppSources", true)),
UseCXXHeadersInCppSources(Options.get("UseCXXHeadersInCppSources", true)),
IncludeInserter(Options.getLocalOrGlobal("IncludeStyle",
utils::IncludeSorter::IS_LLVM)) {
}
utils::IncludeSorter::IS_LLVM),
areDiagsSelfContained()) {}
void ImplicitWideningOfMultiplicationResultCheck::registerPPCallbacks(
const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {

View File

@ -27,7 +27,8 @@ InitVariablesCheck::InitVariablesCheck(StringRef Name,
ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
IncludeInserter(Options.getLocalOrGlobal("IncludeStyle",
utils::IncludeSorter::IS_LLVM)),
utils::IncludeSorter::IS_LLVM),
areDiagsSelfContained()),
MathHeader(Options.get("MathHeader", "<math.h>")) {}
void InitVariablesCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {

View File

@ -306,10 +306,10 @@ void PreferMemberInitializerCheck::check(
NewInit, AddComma ? "), " : ")"});
Diag << FixItHint::CreateInsertion(InsertPos, Insertion,
FirstToCtorInits);
FirstToCtorInits = areDiagsSelfContained();
}
Diag << FixItHint::CreateRemoval(
CharSourceRange::getCharRange(S->getBeginLoc(), SemiColonEnd));
FirstToCtorInits = false;
}
}
}

View File

@ -22,7 +22,8 @@ ProBoundsConstantArrayIndexCheck::ProBoundsConstantArrayIndexCheck(
StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context), GslHeader(Options.get("GslHeader", "")),
Inserter(Options.getLocalOrGlobal("IncludeStyle",
utils::IncludeSorter::IS_LLVM)) {}
utils::IncludeSorter::IS_LLVM),
areDiagsSelfContained()) {}
void ProBoundsConstantArrayIndexCheck::storeOptions(
ClangTidyOptions::OptionMap &Opts) {

View File

@ -20,7 +20,8 @@ UniqueptrResetReleaseCheck::UniqueptrResetReleaseCheck(
StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
Inserter(Options.getLocalOrGlobal("IncludeStyle",
utils::IncludeSorter::IS_LLVM)) {}
utils::IncludeSorter::IS_LLVM),
areDiagsSelfContained()) {}
void UniqueptrResetReleaseCheck::storeOptions(
ClangTidyOptions::OptionMap &Opts) {

View File

@ -463,7 +463,8 @@ LoopConvertCheck::LoopConvertCheck(StringRef Name, ClangTidyContext *Context)
MinConfidence(Options.get("MinConfidence", Confidence::CL_Reasonable)),
NamingStyle(Options.get("NamingStyle", VariableNamer::NS_CamelCase)),
Inserter(Options.getLocalOrGlobal("IncludeStyle",
utils::IncludeSorter::IS_LLVM)),
utils::IncludeSorter::IS_LLVM),
areDiagsSelfContained()),
UseCxx20IfAvailable(Options.get("UseCxx20ReverseRanges", true)),
ReverseFunction(Options.get("MakeReverseRangeFunction", "")),
ReverseHeader(Options.get("MakeReverseRangeHeader", "")) {
@ -800,9 +801,12 @@ bool LoopConvertCheck::isConvertible(ASTContext *Context,
const ast_matchers::BoundNodes &Nodes,
const ForStmt *Loop,
LoopFixerKind FixerKind) {
// If we already modified the range of this for loop, don't do any further
// updates on this iteration.
if (TUInfo->getReplacedVars().count(Loop))
// In self contained diagnosics mode we don't want dependancies on other
// loops, otherwise, If we already modified the range of this for loop, don't
// do any further updates on this iteration.
if (areDiagsSelfContained())
TUInfo = std::make_unique<TUTrackingInfo>();
else if (TUInfo->getReplacedVars().count(Loop))
return false;
// Check that we have exactly one index variable and at most one end variable.

View File

@ -44,7 +44,8 @@ MakeSmartPtrCheck::MakeSmartPtrCheck(StringRef Name, ClangTidyContext *Context,
StringRef MakeSmartPtrFunctionName)
: ClangTidyCheck(Name, Context),
Inserter(Options.getLocalOrGlobal("IncludeStyle",
utils::IncludeSorter::IS_LLVM)),
utils::IncludeSorter::IS_LLVM),
areDiagsSelfContained()),
MakeSmartPtrFunctionHeader(
Options.get("MakeSmartPtrFunctionHeader", "<memory>")),
MakeSmartPtrFunctionName(

View File

@ -190,7 +190,8 @@ collectParamDecls(const CXXConstructorDecl *Ctor,
PassByValueCheck::PassByValueCheck(StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
Inserter(Options.getLocalOrGlobal("IncludeStyle",
utils::IncludeSorter::IS_LLVM)),
utils::IncludeSorter::IS_LLVM),
areDiagsSelfContained()),
ValuesOnly(Options.get("ValuesOnly", false)) {}
void PassByValueCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {

View File

@ -41,7 +41,8 @@ ReplaceAutoPtrCheck::ReplaceAutoPtrCheck(StringRef Name,
ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
Inserter(Options.getLocalOrGlobal("IncludeStyle",
utils::IncludeSorter::IS_LLVM)) {}
utils::IncludeSorter::IS_LLVM),
areDiagsSelfContained()) {}
void ReplaceAutoPtrCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "IncludeStyle", Inserter.getStyle());

View File

@ -24,8 +24,8 @@ ReplaceRandomShuffleCheck::ReplaceRandomShuffleCheck(StringRef Name,
ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
IncludeInserter(Options.getLocalOrGlobal("IncludeStyle",
utils::IncludeSorter::IS_LLVM)) {
}
utils::IncludeSorter::IS_LLVM),
areDiagsSelfContained()) {}
void ReplaceRandomShuffleCheck::registerMatchers(MatchFinder *Finder) {
const auto Begin = hasArgument(0, expr());

View File

@ -32,8 +32,8 @@ TypePromotionInMathFnCheck::TypePromotionInMathFnCheck(
StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
IncludeInserter(Options.getLocalOrGlobal("IncludeStyle",
utils::IncludeSorter::IS_LLVM)) {
}
utils::IncludeSorter::IS_LLVM),
areDiagsSelfContained()) {}
void TypePromotionInMathFnCheck::registerPPCallbacks(
const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {

View File

@ -69,7 +69,8 @@ UnnecessaryValueParamCheck::UnnecessaryValueParamCheck(
StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
Inserter(Options.getLocalOrGlobal("IncludeStyle",
utils::IncludeSorter::IS_LLVM)),
utils::IncludeSorter::IS_LLVM),
areDiagsSelfContained()),
AllowedTypes(
utils::options::parseStringList(Options.get("AllowedTypes", ""))) {}

View File

@ -36,8 +36,9 @@ private:
IncludeInserter *Inserter;
};
IncludeInserter::IncludeInserter(IncludeSorter::IncludeStyle Style)
: Style(Style) {}
IncludeInserter::IncludeInserter(IncludeSorter::IncludeStyle Style,
bool SelfContainedDiags)
: Style(Style), SelfContainedDiags(SelfContainedDiags) {}
void IncludeInserter::registerPreprocessor(Preprocessor *PP) {
assert(PP && "PP shouldn't be null");
@ -73,7 +74,9 @@ IncludeInserter::createIncludeInsertion(FileID FileID, llvm::StringRef Header) {
return llvm::None;
// We assume the same Header will never be included both angled and not
// angled.
if (!InsertedHeaders[FileID].insert(Header).second)
// In self contained diags mode we don't track what headers we have already
// inserted.
if (!SelfContainedDiags && !InsertedHeaders[FileID].insert(Header).second)
return llvm::None;
return getOrCreate(FileID).createIncludeInsertion(Header, IsAngled);

View File

@ -59,7 +59,8 @@ public:
/// using \code
/// Options.getLocalOrGlobal("IncludeStyle", <DefaultStyle>)
/// \endcode
explicit IncludeInserter(IncludeSorter::IncludeStyle Style);
explicit IncludeInserter(IncludeSorter::IncludeStyle Style,
bool SelfContainedDiags);
/// Registers this with the Preprocessor \p PP, must be called before this
/// class is used.
@ -93,6 +94,7 @@ private:
llvm::DenseMap<FileID, llvm::StringSet<>> InsertedHeaders;
const SourceManager *SourceMgr{nullptr};
const IncludeSorter::IncludeStyle Style;
const bool SelfContainedDiags;
friend class IncludeInserterCallback;
};

View File

@ -30,8 +30,8 @@ static void verifyRule(const RewriteRuleWith<std::string> &Rule) {
TransformerClangTidyCheck::TransformerClangTidyCheck(StringRef Name,
ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
Inserter(
Options.getLocalOrGlobal("IncludeStyle", IncludeSorter::IS_LLVM)) {}
Inserter(Options.getLocalOrGlobal("IncludeStyle", IncludeSorter::IS_LLVM),
areDiagsSelfContained()) {}
// This constructor cannot dispatch to the simpler one (below), because, in
// order to get meaningful results from `getLangOpts` and `Options`, we need the

View File

@ -412,6 +412,7 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
CTContext->setDiagnosticsEngine(&Clang->getDiagnostics());
CTContext->setASTContext(&Clang->getASTContext());
CTContext->setCurrentFile(Filename);
CTContext->setSelfContainedDiags(true);
CTChecks = CTFactories.createChecks(CTContext.getPointer());
llvm::erase_if(CTChecks, [&](const auto &Check) {
return !Check->isLanguageVersionSupported(CTContext->getLangOpts());

View File

@ -678,6 +678,67 @@ TEST(DiagnosticTest, ElseAfterReturnRange) {
Diag(Main.range(), "do not use 'else' after 'return'"))));
}
TEST(DiagnosticTest, ClangTidySelfContainedDiags) {
Annotations Main(R"cpp($MathHeader[[]]
struct Foo{
int A, B;
Foo()$Fix[[]] {
$A[[A = 1;]]
$B[[B = 1;]]
}
};
void InitVariables() {
float $C[[C]]$CFix[[]];
double $D[[D]]$DFix[[]];
}
)cpp");
TestTU TU = TestTU::withCode(Main.code());
TU.ClangTidyProvider =
addTidyChecks("cppcoreguidelines-prefer-member-initializer,"
"cppcoreguidelines-init-variables");
clangd::Fix ExpectedAFix;
ExpectedAFix.Message =
"'A' should be initialized in a member initializer of the constructor";
ExpectedAFix.Edits.push_back(TextEdit{Main.range("Fix"), " : A(1)"});
ExpectedAFix.Edits.push_back(TextEdit{Main.range("A"), ""});
// When invoking clang-tidy normally, this code would produce `, B(1)` as the
// fix the `B` member, as it would think its already included the ` : ` from
// the previous `A` fix.
clangd::Fix ExpectedBFix;
ExpectedBFix.Message =
"'B' should be initialized in a member initializer of the constructor";
ExpectedBFix.Edits.push_back(TextEdit{Main.range("Fix"), " : B(1)"});
ExpectedBFix.Edits.push_back(TextEdit{Main.range("B"), ""});
clangd::Fix ExpectedCFix;
ExpectedCFix.Message = "variable 'C' is not initialized";
ExpectedCFix.Edits.push_back(TextEdit{Main.range("CFix"), " = NAN"});
ExpectedCFix.Edits.push_back(
TextEdit{Main.range("MathHeader"), "#include <math.h>\n\n"});
// Again in clang-tidy only the include directive would be emitted for the
// first warning. However we need the include attaching for both warnings.
clangd::Fix ExpectedDFix;
ExpectedDFix.Message = "variable 'D' is not initialized";
ExpectedDFix.Edits.push_back(TextEdit{Main.range("DFix"), " = NAN"});
ExpectedDFix.Edits.push_back(
TextEdit{Main.range("MathHeader"), "#include <math.h>\n\n"});
EXPECT_THAT(
*TU.build().getDiagnostics(),
UnorderedElementsAre(
AllOf(Diag(Main.range("A"), "'A' should be initialized in a member "
"initializer of the constructor"),
withFix(equalToFix(ExpectedAFix))),
AllOf(Diag(Main.range("B"), "'B' should be initialized in a member "
"initializer of the constructor"),
withFix(equalToFix(ExpectedBFix))),
AllOf(Diag(Main.range("C"), "variable 'C' is not initialized"),
withFix(equalToFix(ExpectedCFix))),
AllOf(Diag(Main.range("D"), "variable 'D' is not initialized"),
withFix(equalToFix(ExpectedDFix)))));
}
TEST(DiagnosticsTest, Preprocessor) {
// This looks like a preamble, but there's an #else in the middle!
// Check that:

View File

@ -53,6 +53,7 @@ Inlay hints
Diagnostics
^^^^^^^^^^^
- Improved Fix-its of some clang-tidy checks when applied with clangd.
Semantic Highlighting
^^^^^^^^^^^^^^^^^^^^^

View File

@ -30,8 +30,10 @@ class IncludeInserterCheckBase : public ClangTidyCheck {
public:
IncludeInserterCheckBase(StringRef CheckName, ClangTidyContext *Context,
utils::IncludeSorter::IncludeStyle Style =
utils::IncludeSorter::IS_Google)
: ClangTidyCheck(CheckName, Context), Inserter(Style) {}
utils::IncludeSorter::IS_Google,
bool SelfContainedDiags = false)
: ClangTidyCheck(CheckName, Context),
Inserter(Style, SelfContainedDiags) {}
void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
Preprocessor *ModuleExpanderPP) override {
@ -85,6 +87,19 @@ public:
}
};
class MultipleHeaderSingleInserterCheck : public IncludeInserterCheckBase {
public:
MultipleHeaderSingleInserterCheck(StringRef CheckName,
ClangTidyContext *Context)
: IncludeInserterCheckBase(CheckName, Context,
utils::IncludeSorter::IS_Google,
/*SelfContainedDiags=*/true) {}
std::vector<StringRef> headersToInclude() const override {
return {"path/to/header.h", "path/to/header2.h", "path/to/header.h"};
}
};
class CSystemIncludeInserterCheck : public IncludeInserterCheckBase {
public:
CSystemIncludeInserterCheck(StringRef CheckName, ClangTidyContext *Context)
@ -246,6 +261,41 @@ void foo() {
PreCode, "clang_tidy/tests/insert_includes_test_input2.cc"));
}
TEST(IncludeInserterTest, InsertMultipleIncludesNoDeduplicate) {
const char *PreCode = R"(
#include "clang_tidy/tests/insert_includes_test_header.h"
#include <list>
#include <map>
#include "path/to/a/header.h"
void foo() {
int a = 0;
})";
// FIXME: ClangFormat bug - https://bugs.llvm.org/show_bug.cgi?id=49298
// clang-format off
const char *PostCode = R"(
#include "clang_tidy/tests/insert_includes_test_header.h"
#include <list>
#include <map>
#include "path/to/a/header.h"
#include "path/to/header.h"
#include "path/to/header2.h"
#include "path/to/header.h"
void foo() {
int a = 0;
})";
// clang-format on
EXPECT_EQ(PostCode,
runCheckOnCode<MultipleHeaderSingleInserterCheck>(
PreCode, "clang_tidy/tests/insert_includes_test_input2.cc"));
}
TEST(IncludeInserterTest, InsertBeforeFirstNonSystemInclude) {
const char *PreCode = R"(
#include "clang_tidy/tests/insert_includes_test_header.h"