This is an archive of the discontinued LLVM Phabricator instance.

[clang][Tooling] Add support for generating #import edits
ClosedPublic

Authored by dgoldman on Jun 27 2022, 12:41 PM.

Details

Summary

And make use of this from clangd's CodeComplete and IncludeFixer, although currently they are both restricted only to #include symbols.

Diff Detail

Event Timeline

dgoldman created this revision.Jun 27 2022, 12:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 27 2022, 12:41 PM
dgoldman requested review of this revision.Jun 27 2022, 12:41 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 27 2022, 12:41 PM
dgoldman updated this revision to Diff 479343.Dec 1 2022, 10:09 AM

Run clang-format

dgoldman retitled this revision from [clangd] Support #import insertions to [clangd] Add support for generating #import edits.Dec 1 2022, 10:10 AM
dgoldman edited the summary of this revision. (Show Details)

we should have tests in clang/unittests/Tooling/HeaderIncludesTest.cpp and the commit itself should be tagged as [clang][Tooling] rather than [clangd].

clang-tools-extra/clangd/Headers.h
250

again it's better to have an enum here.

clang-tools-extra/clangd/IncludeFixer.cpp
252

again can we rather pass the directive enum around?

clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
74

rather than a boolean flag, can you introduce an enum IncludeDirective { Include, Import }; and default it to Include here?

83

we also need to preserve includedirective here now. as the behaviour of insert is to return none iff it's exactly the same spelling. so if there's a #include "foo.h" and we want to insert #import "foo.h" it shouldn't fail.

dgoldman updated this revision to Diff 479659.Dec 2 2022, 9:33 AM
dgoldman marked 4 inline comments as done.

Fixes for review

dgoldman retitled this revision from [clangd] Add support for generating #import edits to [clang][Tooling] Add support for generating #import edits.Dec 2 2022, 9:34 AM
dgoldman edited the summary of this revision. (Show Details)

thanks, mostly LG. some small changes.

clang-tools-extra/clangd/IncludeFixer.cpp
260

nit: llvm::StringLiteral DirectiveSpelling = Directive == tooling::IncludeDirective::Include ? "Include" : "Import"; and use that inside formatv calls

clang-tools-extra/clangd/unittests/HeadersTests.cpp
337

nit: ASSERT_TRUE, here and above.

clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
47

this will leak enum values to the whole clang::tooling namespace. can you make this an enum class IncludeDirective instead (sorry for forgetting that in the initial comment)

75

i don't think we have many callers here. can we just update them instead of defaulting to Include here?

76

s/#includes/#includes and #imports/

clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
299

i know this is not possible today, but just to be safe, can we actually check for import here and default to include ?

351

nit: can you put some braces around the outermost if, as the statements inside are getting long.

374

nit:

llvm::StringLiteral DirectiveSpelling = Directive == IncludeDirective::Include ? "include" : "import";
std::string NewInclude = llvm::formatv("#{0} {1}\n", DirectiveSpelling, QuotedName); // this already has an implicit conversion operator to std::string
clang/unittests/Tooling/HeaderIncludesTest.cpp
65 ↗(On Diff #479665)

can you also add a removal test?

i know we didn't change the implementation, but today there's nothing checking the behaviour. so explicitly encoding the expectation here, especially for the case where we have both #include and #import with same spelling is useful. btw is it likely that #include "foo.h" and #import "foo.h" can refer to different physical headers (e.g. due to framework handling and whatnot)?

we're not changing the behaviour today, but if there's a difference, this is likely a bug (that already exists), so we should leave a fixme (or address it here if you got time, by introducing a IncludeDirective parameter into remove. updating users is likely harder in this case, as everyone needs to propagate/store the directive during their analysis).

dgoldman updated this revision to Diff 480482.Dec 6 2022, 7:20 AM
dgoldman marked 9 inline comments as done.

Fixes for review + fix diffbase

clang/unittests/Tooling/HeaderIncludesTest.cpp
65 ↗(On Diff #479665)

No, I don't think #import and #include can refer to different headers.

kadircet accepted this revision.Dec 6 2022, 7:54 AM

thanks!

clang/unittests/Tooling/HeaderIncludesTest.cpp
77 ↗(On Diff #480482)

nit: feel free to use raw string literals here and in other tests (or angled (<) includes), e.g:

llvm::StringRef Code = R"cpp(
  #include <abc.h>
  #import <abc.h>
  int x;)cpp";
EXPECT_EQ("int x;", remove(Code, "<abc.h>"));
81 ↗(On Diff #480482)

nit: just inline "int x" to the LHS.

This revision is now accepted and ready to land.Dec 6 2022, 7:54 AM
dgoldman updated this revision to Diff 480524.Dec 6 2022, 9:28 AM
dgoldman marked 2 inline comments as done.

Use raw string literal

This revision was landed with ongoing or failed builds.Dec 6 2022, 10:48 AM
This revision was automatically updated to reflect the committed changes.