This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add a way to enable IncludeCleaner through config
ClosedPublic

Authored by kbobyrev on Oct 15 2021, 2:37 AM.

Details

Summary

This is useful for dogfooding the feature and spotting bugs.

Diff Detail

Event Timeline

kbobyrev created this revision.Oct 15 2021, 2:37 AM
kbobyrev requested review of this revision.Oct 15 2021, 2:37 AM
kbobyrev updated this revision to Diff 379944.Oct 15 2021, 2:46 AM

Delete debugging leftovers.

kbobyrev updated this revision to Diff 380426.Oct 18 2021, 8:52 AM

Fix the warning range.

sammccall added inline comments.Oct 18 2021, 10:01 AM
clang-tools-extra/clangd/Config.h
89–90

you've accidentally split this comment from its decl

90

structure, as discussed offline...

"include cleaner" refers to a set of features to do with header hygiene, related but I think we still want to configure them individually.

"Unused headers" is the feature we're trying to configure, so I think it's a key rather than a value. Its value could be a boolean, (and configure other aspects separately), or we could make it an enum so we can choose a policy at the same time.
My suspicion is that the major knob here is strictness: do we warn when a header doesn't *directly* provide any used symbols, or also indirectly?

So I'd probably call this UnusedInclude: Strict

94

don't group with the generic suppression bits

clang-tools-extra/clangd/ConfigFragment.h
213

This is where the documentation for the feature goes :-)

clang-tools-extra/clangd/IncludeCleaner.cpp
215

Source should be Clangd I think (our first?).

Name should be something like "unused-include": this doesn't need to echo the tool name.

216

remove FIXEDME

221

This doesn't handle hitting EOF correctly, and also gets encodings wrong (should be UTF-16 code units, unless we're in another mode).

I think you want:

D.range.end.character = lspLength(Code.drop_front(Inc->HashOffset).take_until([](char C) { return c == '\n' || c == '\r'; }));

You could make this a function in SourceCode.h if you like.
(I'm fairly sure we're not clean when it comes to using \r being a line separator in the rest of the code BTW)

226

No need to explain the ranges thing, this is ubiquitous and documented on the structs.

229

no-op

230

don't do this in the loop

235

D.Tags.push_back(Unneccesary)

This produces nice rendering in vscode

clang-tools-extra/clangd/IncludeCleaner.h
67

nit: llvm::StringRef

clang-tools-extra/clangd/ParsedAST.cpp
380

You're bypassing this logic, which is OK, but you'll need to duplicate it somewhere. (Probably wherever is looking at cfg anyway)

519

You're missing tests for this functionality

524

If/when we support other policies, the issueUnusedIncludesDiagnostics function will need to look at the current policy, so you might want to move this check there already. (We also need to check for suppression, as mentioned above)

525

you're copying all the diags here

I don't mind the temp vector to keep the signature nice, but Result.Diags->insert(Result.Diags->end(), make_move_iterator(NewDiags.begin()), etc)?

kbobyrev updated this revision to Diff 380665.Oct 19 2021, 5:58 AM
kbobyrev marked 16 inline comments as done.

Address review comments.

kbobyrev updated this revision to Diff 380702.Oct 19 2021, 8:27 AM

Rebase on top of main, revert unwanted formatting change.

kbobyrev updated this revision to Diff 381157.Oct 21 2021, 12:10 AM

Elaborate a bit more on Strict mode for UnusedHeaders.

kbobyrev updated this revision to Diff 381190.Oct 21 2021, 3:21 AM

[clangd] IncludeCleaner: Implement more complicated rules for considering enums forward declarations "used"

kbobyrev updated this revision to Diff 381192.Oct 21 2021, 3:28 AM

Prevent accidental changes from getting into this patch.

Mostly dumb nitpicks because this is user-facing magic strings... let's chat before doing another round so I'm not wasting your time.


You've done some renaming from UnusedInclude -> UnusedHeaders.

I feel bad because I did say in the last round

"Unused headers" is the feature we're trying to configure

but I wasn't intending to be precise and actually think the old name is better (some discussion below)

clang-tools-extra/clangd/ConfigFragment.h
215

I'm not sure we need a separate block for this. The main cost is an extra line in the config file, and it makes the "include cleaner" name user-facing, so it's an extra concept people have to learn.

Do you think we'll need a lot more than UnusedHeaders/MissingHeaders?

216

s/IncludeCleaner/clangd

225

I wonder whether "UnusedIncludes" would be clearer, especially given the pairing with "MissingHeaders" (consider confusion with pp_file_not_found) vs "MissingIncludes".

Conceptually it's an unused edge rather than node, but mostly i'm just thinking "MissingIncludes" more clearly describes the nature of the fix.

clang-tools-extra/clangd/IncludeCleaner.cpp
136

maybe worth a comment that we don't bother to handle escaped newlines etc

143

This code is making me a bit dizzy. I get the idea that we're using the precise HashOffset while exploiting the fact that we know the line number to avoid counting lines. But I'm not sure it's worthwhile, we're near the top of the file after all...

What about just

Result.end = result.start = offsetToPosition(code, HashOffset)
Result.end.character += lspLength(Code.drop_front(HashOffset).take_until(...));
210

config is usually passed by TLS rather than explicitly

215

clangd-unused-header -> unused-header

(there's a test, but it's stale)

225

include at least the basename in the message - this also gets listed in e.g. diagnostics view

233

again, we're not removing the header, we're removing the inclusion
"remove #include directive"?

kbobyrev marked 9 inline comments as done.Oct 26 2021, 1:00 AM
kbobyrev added inline comments.
clang-tools-extra/clangd/IncludeCleaner.cpp
215

what do you mean by "stale"?

kbobyrev updated this revision to Diff 382214.Oct 26 2021, 1:00 AM
kbobyrev marked an inline comment as done.

Resolve review comments.

kbobyrev updated this revision to Diff 382215.Oct 26 2021, 1:03 AM

Use IncludeCleaner to get rid of unused include in IncludeCleaner.cpp 🆒

sammccall accepted this revision.Oct 26 2021, 1:59 AM

Awesome, let's ship it!
Only nits on the text, feel free to ignore any you don't like

clang-tools-extra/clangd/ConfigFragment.h
213

brevity: consider "correct" or "diagnose" instead of "treat and warn users on".
specificity: consider "unnecessary #include directives" instead of "suboptimal include usage".

217

We're glossing over the key idea ("come from") here, and mixing what the Strict policy is about with what the feature is about. I think we can be a bit more precise.

I'd add a second sentence to the first paragraph, elaborating a little:

clangd can warn if a header is `#included` but not used, and suggest removing it.

And then we can define the Strict policy, and mention its limitations.

Strict means a header is unused if it does not *directly* provide any symbol used in the file.
Removing it may still break compilation if it transitively includes headers that are used.
This should be fixed by including those headers directly.
220

I think at least the "fixme" part isn't suitable for public docs.
We also don't mention what we want to do instead.

I'd suggest:

  • move this to the part of the code that generates fixes
  • mention that we if it's transitively used, we could suggest replacing it with the needed #includes instead (I think this is actually pretty feasible...)
  • while there, maybe also add a FIXME that we should suggest adding IWYU pragma export/keep as an alternative fixes (once we honor that)
clang-tools-extra/clangd/IncludeCleaner.cpp
215

You'd updated the diagnostic name, but not the suppression check. I was going to suggest a test for suppression, and realized you had one, it was also testing the old name though.

LG now!

224

This is totally up to you, but wanted to be explicit with what I meant about basename...

If the path is long, this can be: included header this/is/a/long/path/to/some/header.h is not used
Depending how you feel about having to visually scan past the path to get to the end of the sentence, you might choose to trade precision for brevity: included header header.h is not used.

This is llvm::sys::path::filename(stripHeaderQuotes(Inc->Written), posix), with stripHeaderQuotes to be written.

Again, I don't feel strongly, your call

This revision is now accepted and ready to land.Oct 26 2021, 1:59 AM
kbobyrev updated this revision to Diff 382242.Oct 26 2021, 2:53 AM
kbobyrev marked 4 inline comments as done.

Resolve the review comments.