Follow style configuration in clangd when inserting missing includes #140594
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-clangd Author: None (Harald-R) ChangesThe missing include diagnostic has the capability to introduce the necessary headers into the source file. However, it does not currently follow the inclusion style found in the Full diff: https://github.com/llvm/llvm-project/pull/140594.diff 4 Files Affected:
diff --git a/clang-tools-extra/clangd/IncludeCleaner.cpp b/clang-tools-extra/clangd/IncludeCleaner.cpp
index dc4c8fc498b1f..ce16d060a1f06 100644
--- a/clang-tools-extra/clangd/IncludeCleaner.cpp
+++ b/clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -117,7 +117,8 @@ bool mayConsiderUnused(const Inclusion &Inc, ParsedAST &AST,
std::vector<Diag> generateMissingIncludeDiagnostics(
ParsedAST &AST, llvm::ArrayRef<MissingIncludeDiagInfo> MissingIncludes,
- llvm::StringRef Code, HeaderFilter IgnoreHeaders, const ThreadsafeFS &TFS) {
+ llvm::StringRef Code, HeaderFilter IgnoreHeaders,
+ HeaderFilter AngledHeaders, const ThreadsafeFS &TFS) {
std::vector<Diag> Result;
const SourceManager &SM = AST.getSourceManager();
const FileEntry *MainFile = SM.getFileEntryForID(SM.getMainFileID());
@@ -142,6 +143,13 @@ std::vector<Diag> generateMissingIncludeDiagnostics(
llvm::StringRef HeaderRef{Spelling};
bool Angled = HeaderRef.starts_with("<");
+ for (auto Filter : AngledHeaders) {
+ if (Filter(HeaderRef)) {
+ Angled = true;
+ break;
+ }
+ }
+
// We might suggest insertion of an existing include in edge cases, e.g.,
// include is present in a PP-disabled region, or spelling of the header
// turns out to be the same as one of the unresolved includes in the
@@ -481,18 +489,17 @@ bool isPreferredProvider(const Inclusion &Inc,
return false; // no header provides the symbol
}
-std::vector<Diag>
-issueIncludeCleanerDiagnostics(ParsedAST &AST, llvm::StringRef Code,
- const IncludeCleanerFindings &Findings,
- const ThreadsafeFS &TFS,
- HeaderFilter IgnoreHeaders) {
+std::vector<Diag> issueIncludeCleanerDiagnostics(
+ ParsedAST &AST, llvm::StringRef Code,
+ const IncludeCleanerFindings &Findings, const ThreadsafeFS &TFS,
+ HeaderFilter IgnoreHeaders, HeaderFilter AngledHeaders) {
trace::Span Tracer("IncludeCleaner::issueIncludeCleanerDiagnostics");
std::vector<Diag> UnusedIncludes = generateUnusedIncludeDiagnostics(
AST.tuPath(), Findings.UnusedIncludes, Code, IgnoreHeaders);
std::optional<Fix> RemoveAllUnused = removeAllUnusedIncludes(UnusedIncludes);
std::vector<Diag> MissingIncludeDiags = generateMissingIncludeDiagnostics(
- AST, Findings.MissingIncludes, Code, IgnoreHeaders, TFS);
+ AST, Findings.MissingIncludes, Code, IgnoreHeaders, AngledHeaders, TFS);
std::optional<Fix> AddAllMissing = addAllMissingIncludes(MissingIncludeDiags);
std::optional<Fix> FixAll;
diff --git a/clang-tools-extra/clangd/IncludeCleaner.h b/clang-tools-extra/clangd/IncludeCleaner.h
index 3f6e3b2fd45b6..884feb69488d5 100644
--- a/clang-tools-extra/clangd/IncludeCleaner.h
+++ b/clang-tools-extra/clangd/IncludeCleaner.h
@@ -57,11 +57,10 @@ IncludeCleanerFindings
computeIncludeCleanerFindings(ParsedAST &AST,
bool AnalyzeAngledIncludes = false);
-std::vector<Diag>
-issueIncludeCleanerDiagnostics(ParsedAST &AST, llvm::StringRef Code,
- const IncludeCleanerFindings &Findings,
- const ThreadsafeFS &TFS,
- HeaderFilter IgnoreHeader = {});
+std::vector<Diag> issueIncludeCleanerDiagnostics(
+ ParsedAST &AST, llvm::StringRef Code,
+ const IncludeCleanerFindings &Findings, const ThreadsafeFS &TFS,
+ HeaderFilter IgnoreHeader = {}, HeaderFilter AngledHeaders = {});
/// Converts the clangd include representation to include-cleaner
/// include representation.
diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp
index 3f63daaf400db..a47bfafe8a92c 100644
--- a/clang-tools-extra/clangd/ParsedAST.cpp
+++ b/clang-tools-extra/clangd/ParsedAST.cpp
@@ -382,7 +382,8 @@ std::vector<Diag> getIncludeCleanerDiags(ParsedAST &AST, llvm::StringRef Code,
if (SuppressUnused)
Findings.UnusedIncludes.clear();
return issueIncludeCleanerDiagnostics(AST, Code, Findings, TFS,
- Cfg.Diagnostics.Includes.IgnoreHeader);
+ Cfg.Diagnostics.Includes.IgnoreHeader,
+ Cfg.Style.AngledHeaders);
}
tidy::ClangTidyCheckFactories
diff --git a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
index 0ee748c1ed2d0..fda1b3b1835ae 100644
--- a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -220,12 +220,13 @@ TEST(IncludeCleaner, ComputeMissingHeaders) {
TEST(IncludeCleaner, GenerateMissingHeaderDiags) {
Annotations MainFile(R"cpp(
#include "a.h"
+#include "a_angled.h"
#include "all.h"
$insert_b[[]]#include "baz.h"
#include "dir/c.h"
$insert_d[[]]$insert_foo[[]]#include "fuzz.h"
#include "header.h"
-$insert_foobar[[]]#include <e.h>
+$insert_foobar[[]]$insert_b_angled[[]]#include <e.h>
$insert_f[[]]$insert_vector[[]]
#define DEF(X) const Foo *X;
@@ -237,6 +238,7 @@ TEST(IncludeCleaner, GenerateMissingHeaderDiags) {
void foo() {
$b[[b]]();
+ $b_angled[[b_angled]]();
ns::$bar[[Bar]] bar;
bar.d();
@@ -262,6 +264,8 @@ TEST(IncludeCleaner, GenerateMissingHeaderDiags) {
TU.Filename = "main.cpp";
TU.AdditionalFiles["a.h"] = guard("#include \"b.h\"");
TU.AdditionalFiles["b.h"] = guard("void b();");
+ TU.AdditionalFiles["a_angled.h"] = guard("#include \"b_angled.h\"");
+ TU.AdditionalFiles["b_angled.h"] = guard("void b_angled();");
TU.AdditionalFiles["dir/c.h"] = guard("#include \"d.h\"");
TU.AdditionalFiles["dir/d.h"] =
@@ -297,7 +301,8 @@ TEST(IncludeCleaner, GenerateMissingHeaderDiags) {
Findings.UnusedIncludes.clear();
std::vector<clangd::Diag> Diags = issueIncludeCleanerDiagnostics(
AST, TU.Code, Findings, MockFS(),
- {[](llvm::StringRef Header) { return Header.ends_with("buzz.h"); }});
+ {[](llvm::StringRef Header) { return Header.ends_with("buzz.h"); }},
+ {[](llvm::StringRef Header) { return Header.contains("b_angled.h"); }});
EXPECT_THAT(
Diags,
UnorderedElementsAre(
@@ -306,6 +311,12 @@ TEST(IncludeCleaner, GenerateMissingHeaderDiags) {
withFix({Fix(MainFile.range("insert_b"), "#include \"b.h\"\n",
"#include \"b.h\""),
FixMessage("add all missing includes")})),
+ AllOf(Diag(MainFile.range("b_angled"),
+ "No header providing \"b_angled\" is directly included"),
+ withFix(
+ {Fix(MainFile.range("insert_b_angled"),
+ "#include <b_angled.h>\n", "#include \"b_angled.h\""),
+ FixMessage("add all missing includes")})),
AllOf(Diag(MainFile.range("bar"),
"No header providing \"ns::Bar\" is directly included"),
withFix({Fix(MainFile.range("insert_d"),
|
|
@llvm/pr-subscribers-clang-tools-extra Author: None (Harald-R) ChangesThe missing include diagnostic has the capability to introduce the necessary headers into the source file. However, it does not currently follow the inclusion style found in the Full diff: https://github.com/llvm/llvm-project/pull/140594.diff 4 Files Affected:
diff --git a/clang-tools-extra/clangd/IncludeCleaner.cpp b/clang-tools-extra/clangd/IncludeCleaner.cpp
index dc4c8fc498b1f..ce16d060a1f06 100644
--- a/clang-tools-extra/clangd/IncludeCleaner.cpp
+++ b/clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -117,7 +117,8 @@ bool mayConsiderUnused(const Inclusion &Inc, ParsedAST &AST,
std::vector<Diag> generateMissingIncludeDiagnostics(
ParsedAST &AST, llvm::ArrayRef<MissingIncludeDiagInfo> MissingIncludes,
- llvm::StringRef Code, HeaderFilter IgnoreHeaders, const ThreadsafeFS &TFS) {
+ llvm::StringRef Code, HeaderFilter IgnoreHeaders,
+ HeaderFilter AngledHeaders, const ThreadsafeFS &TFS) {
std::vector<Diag> Result;
const SourceManager &SM = AST.getSourceManager();
const FileEntry *MainFile = SM.getFileEntryForID(SM.getMainFileID());
@@ -142,6 +143,13 @@ std::vector<Diag> generateMissingIncludeDiagnostics(
llvm::StringRef HeaderRef{Spelling};
bool Angled = HeaderRef.starts_with("<");
+ for (auto Filter : AngledHeaders) {
+ if (Filter(HeaderRef)) {
+ Angled = true;
+ break;
+ }
+ }
+
// We might suggest insertion of an existing include in edge cases, e.g.,
// include is present in a PP-disabled region, or spelling of the header
// turns out to be the same as one of the unresolved includes in the
@@ -481,18 +489,17 @@ bool isPreferredProvider(const Inclusion &Inc,
return false; // no header provides the symbol
}
-std::vector<Diag>
-issueIncludeCleanerDiagnostics(ParsedAST &AST, llvm::StringRef Code,
- const IncludeCleanerFindings &Findings,
- const ThreadsafeFS &TFS,
- HeaderFilter IgnoreHeaders) {
+std::vector<Diag> issueIncludeCleanerDiagnostics(
+ ParsedAST &AST, llvm::StringRef Code,
+ const IncludeCleanerFindings &Findings, const ThreadsafeFS &TFS,
+ HeaderFilter IgnoreHeaders, HeaderFilter AngledHeaders) {
trace::Span Tracer("IncludeCleaner::issueIncludeCleanerDiagnostics");
std::vector<Diag> UnusedIncludes = generateUnusedIncludeDiagnostics(
AST.tuPath(), Findings.UnusedIncludes, Code, IgnoreHeaders);
std::optional<Fix> RemoveAllUnused = removeAllUnusedIncludes(UnusedIncludes);
std::vector<Diag> MissingIncludeDiags = generateMissingIncludeDiagnostics(
- AST, Findings.MissingIncludes, Code, IgnoreHeaders, TFS);
+ AST, Findings.MissingIncludes, Code, IgnoreHeaders, AngledHeaders, TFS);
std::optional<Fix> AddAllMissing = addAllMissingIncludes(MissingIncludeDiags);
std::optional<Fix> FixAll;
diff --git a/clang-tools-extra/clangd/IncludeCleaner.h b/clang-tools-extra/clangd/IncludeCleaner.h
index 3f6e3b2fd45b6..884feb69488d5 100644
--- a/clang-tools-extra/clangd/IncludeCleaner.h
+++ b/clang-tools-extra/clangd/IncludeCleaner.h
@@ -57,11 +57,10 @@ IncludeCleanerFindings
computeIncludeCleanerFindings(ParsedAST &AST,
bool AnalyzeAngledIncludes = false);
-std::vector<Diag>
-issueIncludeCleanerDiagnostics(ParsedAST &AST, llvm::StringRef Code,
- const IncludeCleanerFindings &Findings,
- const ThreadsafeFS &TFS,
- HeaderFilter IgnoreHeader = {});
+std::vector<Diag> issueIncludeCleanerDiagnostics(
+ ParsedAST &AST, llvm::StringRef Code,
+ const IncludeCleanerFindings &Findings, const ThreadsafeFS &TFS,
+ HeaderFilter IgnoreHeader = {}, HeaderFilter AngledHeaders = {});
/// Converts the clangd include representation to include-cleaner
/// include representation.
diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp
index 3f63daaf400db..a47bfafe8a92c 100644
--- a/clang-tools-extra/clangd/ParsedAST.cpp
+++ b/clang-tools-extra/clangd/ParsedAST.cpp
@@ -382,7 +382,8 @@ std::vector<Diag> getIncludeCleanerDiags(ParsedAST &AST, llvm::StringRef Code,
if (SuppressUnused)
Findings.UnusedIncludes.clear();
return issueIncludeCleanerDiagnostics(AST, Code, Findings, TFS,
- Cfg.Diagnostics.Includes.IgnoreHeader);
+ Cfg.Diagnostics.Includes.IgnoreHeader,
+ Cfg.Style.AngledHeaders);
}
tidy::ClangTidyCheckFactories
diff --git a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
index 0ee748c1ed2d0..fda1b3b1835ae 100644
--- a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -220,12 +220,13 @@ TEST(IncludeCleaner, ComputeMissingHeaders) {
TEST(IncludeCleaner, GenerateMissingHeaderDiags) {
Annotations MainFile(R"cpp(
#include "a.h"
+#include "a_angled.h"
#include "all.h"
$insert_b[[]]#include "baz.h"
#include "dir/c.h"
$insert_d[[]]$insert_foo[[]]#include "fuzz.h"
#include "header.h"
-$insert_foobar[[]]#include <e.h>
+$insert_foobar[[]]$insert_b_angled[[]]#include <e.h>
$insert_f[[]]$insert_vector[[]]
#define DEF(X) const Foo *X;
@@ -237,6 +238,7 @@ TEST(IncludeCleaner, GenerateMissingHeaderDiags) {
void foo() {
$b[[b]]();
+ $b_angled[[b_angled]]();
ns::$bar[[Bar]] bar;
bar.d();
@@ -262,6 +264,8 @@ TEST(IncludeCleaner, GenerateMissingHeaderDiags) {
TU.Filename = "main.cpp";
TU.AdditionalFiles["a.h"] = guard("#include \"b.h\"");
TU.AdditionalFiles["b.h"] = guard("void b();");
+ TU.AdditionalFiles["a_angled.h"] = guard("#include \"b_angled.h\"");
+ TU.AdditionalFiles["b_angled.h"] = guard("void b_angled();");
TU.AdditionalFiles["dir/c.h"] = guard("#include \"d.h\"");
TU.AdditionalFiles["dir/d.h"] =
@@ -297,7 +301,8 @@ TEST(IncludeCleaner, GenerateMissingHeaderDiags) {
Findings.UnusedIncludes.clear();
std::vector<clangd::Diag> Diags = issueIncludeCleanerDiagnostics(
AST, TU.Code, Findings, MockFS(),
- {[](llvm::StringRef Header) { return Header.ends_with("buzz.h"); }});
+ {[](llvm::StringRef Header) { return Header.ends_with("buzz.h"); }},
+ {[](llvm::StringRef Header) { return Header.contains("b_angled.h"); }});
EXPECT_THAT(
Diags,
UnorderedElementsAre(
@@ -306,6 +311,12 @@ TEST(IncludeCleaner, GenerateMissingHeaderDiags) {
withFix({Fix(MainFile.range("insert_b"), "#include \"b.h\"\n",
"#include \"b.h\""),
FixMessage("add all missing includes")})),
+ AllOf(Diag(MainFile.range("b_angled"),
+ "No header providing \"b_angled\" is directly included"),
+ withFix(
+ {Fix(MainFile.range("insert_b_angled"),
+ "#include <b_angled.h>\n", "#include \"b_angled.h\""),
+ FixMessage("add all missing includes")})),
AllOf(Diag(MainFile.range("bar"),
"No header providing \"ns::Bar\" is directly included"),
withFix({Fix(MainFile.range("insert_d"),
|
|
@kleinesfilmroellchen @HighCommander4 would it be possible to take a look when you have some time? Many thanks! |
Yep, will do. Added myself as reviewer. |
| @@ -142,6 +143,13 @@ std::vector<Diag> generateMissingIncludeDiagnostics( | |||
|
|
|||
| llvm::StringRef HeaderRef{Spelling}; | |||
| bool Angled = HeaderRef.starts_with("<"); | |||
| for (auto Filter : AngledHeaders) { | |||
There was a problem hiding this comment.
nit: iterate using auto &, the way we do here
| "No header providing \"b_angled\" is directly included"), | ||
| withFix( | ||
| {Fix(MainFile.range("insert_b_angled"), | ||
| "#include <b_angled.h>\n", "#include \"b_angled.h\""), |
There was a problem hiding this comment.
Shouldn't the message (third parameter to Fix()) also be #include <b_angled.h>?
There was a problem hiding this comment.
Good point. I was under the impression that the Message field referred to the original code (which in this test had an instance of quoted inclusion), but it seems that it refers to the fix itself, so it should be angled as well. I was mainly testing the code with the example here and missed this. Made some changes to the way the Message field is generated in the code to reflect the correct inclusion style: 4896e03
| @@ -262,6 +264,8 @@ TEST(IncludeCleaner, GenerateMissingHeaderDiags) { | |||
| TU.Filename = "main.cpp"; | |||
| TU.AdditionalFiles["a.h"] = guard("#include \"b.h\""); | |||
| TU.AdditionalFiles["b.h"] = guard("void b();"); | |||
| TU.AdditionalFiles["a_angled.h"] = guard("#include \"b_angled.h\""); | |||
There was a problem hiding this comment.
- if
a_angled.his not part of theAngledfilter, it should probably be calleda_quoted.h(orc.hor something else that doesn't include "angled") - to avoid confusion, let's make the include style of
b_angled.hangled here in the contents ofa_angled.has well
There was a problem hiding this comment.
if a_angled.h is not part of the Angled filter, it should probably be called a_quoted.h (or c.h or something else that doesn't include "angled")
I can rename a_angled.h to a_quoted.h or something similar, if that helps avoid confusion. Here I wanted to follow the example with a.h and b.h, where a.h only includes b.h and is used inside the test itself, as it is necessary for the TU.build() call to be successful; otherwise, the b symbol would be undeclared when this build() method is called during the test execution.
Here, the a_angled.h and b_angled.h headers follow the same pattern, albeit the names could be reworked. I something like c.h is preferred, I can replace it, if it doesn't bring confusion with the other dir/c.h header.
to avoid confusion, let's make the include style of b_angled.h angled here in the contents of a_angled.h as well
Using angled brackets here leads to an error:
TestTU failed to build (suppress with /*error-ok*/):
[2:9-2:21] in included file: 'b_angled.h' file not found with <angled> include; use "quotes" instead, notes: {[/clangd-test/a_angled.h:1:9-1:21] error occurred here}
I believe its parent directory would need to be added to the SYSTEM include path.
Let me know which rename makes most sense and whether this header should be in the SYSTEM path, and I will push a commit with the changes.
| @@ -297,7 +301,8 @@ TEST(IncludeCleaner, GenerateMissingHeaderDiags) { | |||
| Findings.UnusedIncludes.clear(); | |||
| std::vector<clangd::Diag> Diags = issueIncludeCleanerDiagnostics( | |||
| AST, TU.Code, Findings, MockFS(), | |||
| {[](llvm::StringRef Header) { return Header.ends_with("buzz.h"); }}); | |||
| {[](llvm::StringRef Header) { return Header.ends_with("buzz.h"); }}, | |||
There was a problem hiding this comment.
nit: a couple of parameter hint comments (/*IgnoreHeader=*/, /*AngledHeaders=*/) would aid readability here
|
just a high level question, I wasn't following the recent developments closely, but we seem to have both an I guess the signals for angled includes are much stricter (search paths need to be marked with |
Hmm, I was mainly focusing on the quoted -> angled case, hence why only the |
Harald-R
force-pushed
the
clangd/include_cleaner/fix_angled_config
branch
from
710ca26 to
4896e03
Compare
The missing include diagnostic has the capability to introduce the necessary headers into the source file. However, it does not currently follow the inclusion style found in the
.clangdfile. For example, if the file explicitly mentions that headers should be include with angled brackets, they could be included with quotes instead. More details in #138740. This PR fixes this gap, so that the style configuration is followed.