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. |
|
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. |
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.
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
|
alright merging this now. @HighCommander4 please take a look at #140594 (comment) when you get a chance though. |
|
@Harald-R Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
|
Apologies for not having had a chance to circle back to this sooner. I read over the discussion of the original patch that added support for the @sam-mccall commented during that review:
and @kleinesfilmroellchen responded:
This is what the patch ended up implementing, and documenting in /// Matching is performed against the header text, not its absolute path
/// within the project.So, currently there is an inconsistency between the documentation and the In addition to the matter of accommodating project styles, one potential challenge I see with using the resolved path is that the documentation of
Having to write regexes that match an "either/or" seems like a bit of a headache for users. @kadircet, thoughts on how to proceed? |
|
@HighCommander4 thank you for the background! To me it sounds like going back to matching against the spelling makes sense. I can submit a patch later today if that's okay with @kadircet as well. Otherwise, this PR could be reverted and I can resubmit it later without the spelling change. |
|
Created #143411 to do the filtering based on the spelling instead of the resolved path. |
|
I am rather worried that we'll lose ability to filter effectively when we match against spelled headers. Maybe the actual use cases in the wild and what I have mind are different, but I would expect users to have a situation like;
Hence I'd still lean towards matching against resolved path instead, I'd also like to highlight that we're matching against a As for P.S, when matching against spellings, we're also matching against a suffix of those spellings today. It might be worthwhile to re-consider that too if we're going down that path. |
…lvm#140594) 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 `.clangd` file. 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 llvm#138740. This PR fixes this gap, so that the style configuration is followed.
…lvm#140594) 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 `.clangd` file. 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 llvm#138740. This PR fixes this gap, so that the style configuration is followed.
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.