Page MenuHomePhabricator

[prototype] include-cleaner library
Needs ReviewPublic

Authored by sammccall on Mar 29 2022, 11:55 AM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

See RFC
https://discourse.llvm.org/t/rfc-lifting-include-cleaner-missing-unused-include-detection-out-of-clangd/61228

This includes

  • a prototype library for IWYU analysis (include-cleaner/{lib,include})
  • a standalone tool (include-cleaner/tool)
  • replacing clangd's unused-include warning with the library
  • adding a new clangd hover interaction
  • a clang-tidy unused-include check

This squashes together quite a few patches worth of scope, has no tests, and
designs are half baked. Still it demonstrates that such a library could be
reused across tools & across unused vs missing warnings.

Some significant things that are missing:

  • IWYU pragma support and other non-self-contained-header patterns
  • ability to sensibly rank suggested includes. (Probably requires a design change to preserve the Node->Symbol->Location->Header DAG)
  • better standard library support (macros)
  • general accuracy
  • missing-include integration for clang-tidy and clangd (mechanical really)

Diff Detail

Event Timeline

sammccall created this revision.Mar 29 2022, 11:55 AM
sammccall requested review of this revision.Mar 29 2022, 11:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 29 2022, 11:55 AM

oops, remove "include is used" diagnostic from clang-tidy!

sammccall updated this revision to Diff 418971.Mar 29 2022, 1:50 PM

doc and clean up

sammccall updated this revision to Diff 419723.Apr 1 2022, 5:16 AM

Add "hints", used for ranking candidates for suggestions.

Not totally happy with this design:

  • its intrusive to a bunch of the types
  • computing/sorting these eagerly seems wasteful when they're rarely needed

However it is nice to be able to share the code easily.

Herald added a project: Restricted Project. · View Herald TranscriptApr 1 2022, 5:16 AM
phosek added a subscriber: phosek.Wed, Jun 1, 9:43 AM

Hey Sam, any update on this one? How can I help?

hi Serge, sorry for the silence here. we'll be moving this towards a production ready version over the next months (also releasing some comments i've forgotten to send, we've already discussed these offline so no need for action).
we'll probably be leaving some todos/fixmes as we go, if you want to help it'd be great to keep an eye on changes going into the clang-tools-extra/include-cleaner directory (i'll also try to add you as a subscriber to them) and mention things you'd be interested in picking up.

clang-tools-extra/clang-tidy/misc/UnusedIncludesCheck.cpp
60

i guess one annyoing thing we're missing here is communication of IWYU export / keep like pragmas. It feels like handling of such will now be distributed across all users, which feels unfortunate.
maybe we can address that by either completely deleting such headers from RecordedPP->Includes.all or having some helper like shouldKeep in RecordedPP->Includes (similar to match).

clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h
50

i think we might want to have some pointers to AnalysisContext owning these DefinedMacros, it's pretty surprising for the user otherwise.

clang-tools-extra/include-cleaner/lib/Headers.cpp
37

feels like we might need some language-based hints here. like stdio.h is C-like.

for the multiple header cases, i suppose it's fine to put them in an order that we think is "common", while still providing the others. so that unused includes usecase won't annoy folks and missing includes can provide sane defaults.

i am not sure if we care much about "guaranteed umbrella headers" case. but in such a scenario, i guess we'd want a policy, that way missing includes can be suppressed but unused includes can still fire (basically a flip that returns only the specific header vs both specific and umbrella header).