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. |
| @@ -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.
There was a problem hiding this comment.
TU.ExtraArgs.push_back("-I" + testPath("."));should be sufficient to get #include <b_angled.h> to compile.
Will leave the naming up to you as it's a minor point. (Possible suggestion: b_angled --> angled, and a_angled --> angled_wrapper?)
There was a problem hiding this comment.
I like the proposed suggestion. Made the changes: 8f56879
|
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
+1, it makes sense to check both filters. Thanks Kadir for catching that. |
| @@ -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.
TU.ExtraArgs.push_back("-I" + testPath("."));should be sufficient to get #include <b_angled.h> to compile.
Will leave the naming up to you as it's a minor point. (Possible suggestion: b_angled --> angled, and a_angled --> angled_wrapper?)
Harald-R
force-pushed
the
clangd/include_cleaner/fix_angled_config
branch
from
4896e03 to
7564e9c
Compare
|
@HighCommander4 @kadircet many thanks for the reviews! Added a commit which takes the There is some duplication with the logic in the include inserter; not sure if this should be solved now or via a dedicated refactoring of the inclusion logic in all places, as was proposed here. |
| @@ -141,16 +143,41 @@ std::vector<Diag> generateMissingIncludeDiagnostics( | |||
| AST.getPreprocessor().getHeaderSearchInfo(), MainFile}); | |||
|
|
|||
| llvm::StringRef HeaderRef{Spelling}; | |||
| bool Angled = HeaderRef.starts_with("<"); | |||
|
|
|||
| bool IsAngled = false; | |||
There was a problem hiding this comment.
we shouldn't be matching against HeaderRef, but instead matching against ResolvedPath and only when SymbolWithMissingInclude.Providers.front().kind() == Physical
nit:
bool Angled = HeaderRef.starts_with("<");
if (SymbolWithMissingInclude.Providers.front().kind() == ..Physical) {
for (auto &Filter : Angled ? QuotedHeaders : AngledHeaders) {
if (Filter(ResolvedPath)) {
Angled = !Angled;
break;
}
}
}
....
std::optional<tooling::Replacement> Replacement = HeaderIncludes.insert(
HeaderRef.trim("\"<>"), Angled, tooling::IncludeDirective::Include);logic in the include inserter seem to have the same bug (we should match against paths, not spellings).
There was a problem hiding this comment.
Made the change: 6739bd8
If this PR gets merged, I will submit another one for fixing the other instance in Headers.cpp.
Harald-R
force-pushed
the
clangd/include_cleaner/fix_angled_config
branch
from
7564e9c to
6739bd8
Compare
There was a problem hiding this comment.
thanks, mostly LGTM, please wait on a final look from @HighCommander4 though, he has more context about this feature overall, maybe the discrepancy on path vs spelling based match has a story.
| @@ -151,6 +164,11 @@ std::vector<Diag> generateMissingIncludeDiagnostics( | |||
| if (!Replacement.has_value()) | |||
| continue; | |||
|
|
|||
| if (Angled && Spelling.front() == '\"') { | |||
There was a problem hiding this comment.
yikes, we need to do this transformation in the other way as well :/
Might be easier to:
if (Angled != (Spelling.front() == '<')) {
Spelling.front() = Angled ? '<' : '"';
Spelling.back() = Angled ? '>' : '"';
}(I guess we're also lacking some tests for the quoted direction)
There was a problem hiding this comment.
@kadircet you are right. Forgot to update the spelling fix when I changed the implementation to include the quoted filter. Fixed and added a test that forces a system include to use the quoted notation.
Harald-R
force-pushed
the
clangd/include_cleaner/fix_angled_config
branch
from
6739bd8 to
eaedbef
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Harald-R
force-pushed
the
clangd/include_cleaner/fix_angled_config
branch
from
fa38248 to
41a5055
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.