[clang-tidy] Add readability-duplicate-include check

[LegalizeAdulthood]

Summary

Looks for duplicate includes and removes them.

Every time an include directive is processed, check a vector of filenames
to see if the included file has already been included. If so, it issues
a warning and a replacement to remove the entire line containing the
duplicated include directive.

When a macro is defined or undefined, the vector of filenames is cleared.
This enables including the same file multiple times, but getting
different expansions based on the set of active macros at the time of
inclusion. For example:

#undef NDEBUG
#include "assertion.h"
// ...code with assertions enabled

#define NDEBUG
#include "assertion.h"
// ...code with assertions disabled

Since macros are redefined between the inclusion of assertion.h,
they are not flagged as redundant.

[LegalizeAdulthood]

Add doxygen comments to check's .h file

[alexfh]

I'd better put the effort in making the llvm-include-order check extensible (or configurable) enough to support custom include order, and then implement this inside the include order check. There's a TODO, BTW ;)

Will this suit your needs?

[LegalizeAdulthood]

I think there is benefit to checking for redundant includes separately from include order.

On many code bases where I've worked the include order is all over the place. The main barrier in my experience to having a consistent include order hasn't been one of tools, but getting agreement from the team. For the llvm/clang code bases the team has already decided on the order of includes, but for many teams that can start a big political battle.

However, noone is going to disagree with eliminating redundant includes.

[alexfh]

In D7982#132666, @LegalizeAdulthood wrote:

I think there is benefit to checking for redundant includes separately from include order.

On many code bases where I've worked the include order is all over the place. The main barrier in my experience to having a consistent include order hasn't been one of tools, but getting agreement from the team. For the llvm/clang code bases the team has already decided on the order of includes, but for many teams that can start a big political battle.

However, noone is going to disagree with eliminating redundant includes.

Makes sense. See a couple of initial comments below.

I have a feeling that some common machinery could be pulled from the llvm-include-order check which could benefit this check as well. I'll try to come up with a specific idea later.

clang-tidy/readability/CMakeLists.txt

10 ↗

(On Diff #20953)

I'm not a native speaker, but I have a feeling that "duplicate" is a more suitable word here than "redundant". WDYT?

clang-tidy/readability/RedundantInclude.h

22 ↗

(On Diff #20953)

This description is somewhat confusing. Maybe something along the lines of "Only consecutive #include directives without any other preprocessor directives between them are analyzed"?

[LegalizeAdulthood]

clang-tidy/readability/CMakeLists.txt

10 ↗

(On Diff #20953)

Fixed.

clang-tidy/readability/RedundantInclude.h

22 ↗

(On Diff #20953)

Fixed.

[LegalizeAdulthood]

Update check description in header file.
Rename check class to DuplicateIncludeCheck
Rename check to readability-duplicate-include

[alexfh]

clang-tidy/readability/DuplicateIncludeCheck.cpp

62 ↗

(On Diff #21439)

What's the reason to limit the check to the main file only? I think, it should work on all headers as well. Also, sometimes it's fine to have duplicate includes even without macro definitions in between, e.g. when these #includes are in different namespaces.

I'd suggest using the same technique as in the IncludeOrderCheck: for each file collect all preprocessor directives sorted by SourceLocation. Then detect #include blocks (not necessarily the same way as its done in the IncludeOrderCheck. Maybe use the presense of any non-comment tokens between #includes as a boundary of blocks), and detect duplicate includes in each block.

[LegalizeAdulthood]

clang-tidy/readability/DuplicateIncludeCheck.cpp

62 ↗

(On Diff #21439)

If I remove the isInMainFile, how do I ensure that I don't attempt to modify system headers?

[alexfh]

clang-tidy/readability/DuplicateIncludeCheck.cpp

62 ↗

(On Diff #21439)

Using SourceLocation::isInSystemHeader().

[LegalizeAdulthood]

clang-tidy/readability/DuplicateIncludeCheck.cpp

62 ↗

(On Diff #21439)

Thanks, I'll try that. The next question that brings to mind is how do I distinguish between headers that I own and headers used from third party libraries?

For instance, suppose I run a check on a clang refactoring tool and it uses isInSystemHeader and starts flagging issues in the clang tooling library headers.

The compile_commands.json doesn't contain any information about headers in my project, only translation units in my build, so it doesn't know whether or not included headers belong to me or third-party libraries.

[LegalizeAdulthood]

clang-tidy/readability/DuplicateIncludeCheck.cpp

62 ↗

(On Diff #21439)

For the benefit of others reading this, Alex pointed out to me the -header-filter and -system-headers options to clang-tidy. I think this means I don't need any narrowing if isExpansinInMainFile() in any of my matchers. I will do some experimenting to verify that this doesn't introduce regressions.

[sbenza]

clang-tidy/readability/DuplicateIncludeCheck.cpp

23 ↗

(On Diff #21439)

Function names start with lowercase.

28 ↗

(On Diff #21439)

This seems very wasteful, given that the SourceManager could tell you (if there was an API) the location for the next line in O(1) time.
Don't know if it is worth enough to add this API to SourceManager.

53 ↗

(On Diff #21439)

These should not end with _

[LegalizeAdulthood]

clang-tidy/readability/DuplicateIncludeCheck.cpp

28 ↗

(On Diff #21439)

I thought it should have been easy to say "get me the source range that spans the line containing this #include directive", but I didn't find anything easier that worked properly for my test cases. I'm in the process of refactoring this check to share some common code with llvm-include-order and that might eliminate the need for this, but I haven't gotten that far yet.

[sbenza]

clang-tidy/readability/DuplicateIncludeCheck.cpp

28 ↗

(On Diff #21439)

I'm ok with the way it is.
It is only being used when making a warning so it should not cost anything on clean code.
It was just a general comment.
Maybe adding a comment here would be good in case someone comes looking for a speedup opportunity.

[alexfh]

What's the state of this patch?

[LegalizeAdulthood]

I need to update from review comments and upload a new diff.

[LegalizeAdulthood]

clang-tidy/readability/DuplicateIncludeCheck.cpp

62 ↗

(On Diff #21439)

If I remove the isInMainFile and replace it with a check to isInSystemHeader, then all my tests fail. I will have to investigate this further.

[Eugene.Zelenko]

Include What You Use detect duplicated include directives. I think will be good idea to use it instead of Clang-tidy for much deeper analysis.

[LegalizeAdulthood]

Review takes too long to make forward progress; abandoning.

[LegalizeAdulthood]

Revive review with updated diff on top-of-tree

[Eugene.Zelenko]

Functionality-wise this check is superseded by Include What You Use.

clang-tools-extra/docs/clang-tidy/checks/readability-duplicate-include.rst

7

Please fix double spaces. Same below.

[LegalizeAdulthood]

Move included headers to clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-duplicate-include
Stub out included headers to isolate from system headers
Run test for C++98 or later

[LegalizeAdulthood]

In D7982#3216332, @Eugene.Zelenko wrote:

Functionality-wise this check is superseded by Include What You Use.

That's not part of clang-tidy and has it's own set of problems (false positives) when you run it.

clang-tools-extra/docs/clang-tidy/checks/readability-duplicate-include.rst

7

Please point me to a LLVM coding standards section that requires this.

[LegalizeAdulthood]

clang-format

[LegalizeAdulthood]

Ping.

This review has been waiting for a week without any comment.

[LegalizeAdulthood]

2nd. ping.

Still waiting for a review on this check.

[LegalizeAdulthood]

Ping.

Still waiting for a review on this check.

The implementation isn't particularly long or complicated,
so it should not take much time to review.

Thanks.

[carlosgalvezp]

Looks good! I need to familiarize myself better with the Loc manipulation code to be able to review the free-standing function but otherwise looks reasonable.

clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp

23

Move out of anonymous namespace and use static, as per LLVM guidelines.

65

I'm not familiar with isInMainFile, will the check work if the duplicated include is in a header included by the main .cpp file? Should a test be added for that?

clang-tools-extra/docs/clang-tidy/checks/readability-duplicate-include.rst

35–48

As an external user that is not deeply involved in the details of LLVM, I find this example rather complicated. Would it be possible to use a simpler example? What about this one with cassert

#undef NDEBUG
#include <cassert>

// ... code with assertions enabled

#define NDEBUG
#include <cassert>

// ... code with assertions disabled

[carlosgalvezp]

There's also a negative vote from @alexfh , what about that?

[LegalizeAdulthood]

clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp

65

Good catch. I originally wrote this check in 2015 (yes, that's
how long it's been waiting for a review) and recently revived it.
At the time, I didn't understand that checks could apply fixits to
headers and that user headers could be differentiated from system
headers, so this is a mistake.

clang-tools-extra/docs/clang-tidy/checks/readability-duplicate-include.rst

35–48

Good idea and I like your example better.

[LegalizeAdulthood]

! In D7982#3258888, @carlosgalvezp wrote:
I need to familiarize myself better with the Loc manipulation
code to be able to review the free-standing function but otherwise
looks reasonable.

The "clang internals" documentation does a good job of explaining the
location classes:
https://clang.llvm.org/docs/InternalsManual.html#the-sourcelocation-and-sourcemanager-classes

This reminds me that I've been thinking we need to write some
documentation for clang-tidy check authors to help orient them
on the various parts involved in writing a check.

[LegalizeAdulthood]

• Properly apply fixes to header files
• Add tests for fixes in header files
• Move function to static outside anonymous namespace
• Improve documentation

[LegalizeAdulthood]

@carlosgalvezp I believe I've addressed your comments now.
Thanks for the review, it definitely improved this check.

[carlosgalvezp]

LGTM! Had some nits that can be fixed without review.

Can't see anything else major that needs change. As said I'm fairly new here so probably a more experienced reviewer can find some more improvements/optimizations, especially on the Loc aspects. On the other hand I think after 6 years it's about time to get this in, it can always be improved afterwards :)

clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp

85

Nit: as a n00b I would have appreciated a quick comment about why we can't directly use the HashLoc and FilenameRange as Start and End locations. Took me a while to understand this is to cover these cases:

#include "foo.h" -> needs updated Start
#include "bar.h" // Bar is needed! -> needs updated End

clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-duplicate-include/readability-duplicate-include.h

2

Nit: #ifndef

clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-duplicate-include/system/sys/types.h

2

Nit: write "This file is intentionally empty" for consistency with the other files you added? It's already clear from the folder structure that this file is an input for the readability-duplicate-include test.

clang-tools-extra/test/clang-tidy/checkers/readability-duplicate-include.cpp

20

Nit: it would be good to keep [readability-duplicate-include] part of the warning consistent - on Line 8 you have it but not on the other lines. So either keep it for all warnings or remove it from line 8.

[LegalizeAdulthood]

clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-duplicate-include/readability-duplicate-include.h

2

It's a style thing. I don't prefer #ifndef because it only differs from #ifdef by a single letter.

clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-duplicate-include/system/sys/types.h

2

Oops, yeah, meant to do that with all of them and missed this one.
When I first authored this check 7 years ago, they didn't have the Inputs
folder.

clang-tools-extra/test/clang-tidy/checkers/readability-duplicate-include.cpp

20

CHECK-MESSAGES will match whatever substring you provide. To make
the test file less noisy, I do a full match on the entire diagnostic for the
first warning and then just the necessary bits for the remaining checks.

[LegalizeAdulthood]

In D7982#3260403, @carlosgalvezp wrote:

LGTM! Had some nits that can be fixed without review.

I think you need to do "Accept Revision" in phabricator.
It's in the "Add Action..." dropdown on the bottom of the
review page.

[LegalizeAdulthood]

• Update from review comments

[carlosgalvezp]

clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-duplicate-include/readability-duplicate-include.h

2

You make a very valid point and I would definitely agree with that for "regular" code.

However we are talking about a very special case here - header guards. Header guards have a de-facto standard based on #ifndef/#define. It's actually less error prone to write it like that - a well-written header guard will have perfect visual alignment:

#ifndef FOO_H
#define FOO_H

If you missed the n in #ifndef, you would notice the misalignment immediately and know what to fix.

#ifdef FOO_H
#define FOO_H

This also helps making sure that the macro name is identical on both lines. This visual alignment is broken when using #if !defined, which makes it harder to detect these problems.

Furthermore, I have run a quick search in the whole repo and I cannot find one single instance where a header guard is written in the way that you propose here.

$ git grep -E "#ifndef[ ]+[A-Z_]+_H" | wc -l
9573

$ git grep -E "#if[ ]+\!defined\([A-Z_]+_H\)" | wc -l
7

All such 7 instances are exclusively used for error handling or other logic, not for defining header guards.

Therefore I don't see this as a style choice, but rather a repo-wide convention that shall be followed, and so I feel it's my duty as reviewer to request changes.

I understand this is annoying after 7 years waiting for a patch, but I think it's a very easy fix to do. I can approve the patch right away after this if fixed. Thank you for your patience! :)

[LegalizeAdulthood]

Update from review comments

[LegalizeAdulthood]

clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-duplicate-include/readability-duplicate-include.h

2

When it comes to matters that I consider a style preference, I
always prioritize the style of the team/project/code base over
my own style preferences unless I'm the only person working
on the existing code base :)

[carlosgalvezp]

Great patch, thanks!!

[LegalizeAdulthood]

In D7982#3263790, @carlosgalvezp wrote:

Great patch, thanks!!

Can you mark the patch as accepted in phabricator?
Thanks

[carlosgalvezp]

In D7982#3263920, @LegalizeAdulthood wrote:

In D7982#3263790, @carlosgalvezp wrote:

Great patch, thanks!!

Can you mark the patch as accepted in phabricator?
Thanks

I did do that and it shows that I have accepted the revision, what else should I do?

Or is it because of the negative vote from @alexfh ? Since he's not replying perhaps you can remove him from the reviewer list?

[LegalizeAdulthood]

In D7982#3264009, @carlosgalvezp wrote:

In D7982#3263920, @LegalizeAdulthood wrote:

In D7982#3263790, @carlosgalvezp wrote:

Great patch, thanks!!

Can you mark the patch as accepted in phabricator?
Thanks

I did do that and it shows that I have accepted the revision, what else should I do?

Or is it because of the negative vote from @alexfh ? Since he's not replying perhaps you can remove him from the reviewer list?

Ah yes, I see now, it's in the history that you accepted the patch.
I'm not sure how to clear the "needs revision" status from Alex
years ago, so I'm going to submit this.

[LegalizeAdulthood]

OK, removing Alex has cleared it.

[sylvestre.ledru]

Thanks for this, it found 74 occurrences of this in the firefox code base
https://hg.mozilla.org/mozilla-central/rev/b07f125d09fd

clang-tools-extra/clang-tidy/readability/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
   )
 
 add_clang_library(clangTidyReadabilityModule
   AvoidConstParamsInDecls.cpp
   BracesAroundStatementsCheck.cpp
   ConstReturnTypeCheck.cpp
   ContainerDataPointerCheck.cpp
   ContainerSizeEmptyCheck.cpp
   ConvertMemberFunctionsToStatic.cpp
   DeleteNullPointerCheck.cpp
+  DuplicateIncludeCheck.cpp
   ElseAfterReturnCheck.cpp
   FunctionCognitiveComplexityCheck.cpp
   FunctionSizeCheck.cpp
   IdentifierLengthCheck.cpp
   IdentifierNamingCheck.cpp
   ImplicitBoolConversionCheck.cpp
   InconsistentDeclarationParameterNameCheck.cpp
   IsolateDeclarationCheck.cpp

clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.h

+//===--- DuplicateIncludeCheck.h - clang-tidy -------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_DUPLICATE_INCLUDE_CHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_DUPLICATE_INCLUDE_CHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// \brief Find and remove duplicate #include directives.
+///
+/// Only consecutive include directives without any other preprocessor
+/// directives between them are analyzed.
+class DuplicateIncludeCheck : public ClangTidyCheck {
+public:
+  DuplicateIncludeCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+                           Preprocessor *ModuleExpanderPP) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_DUPLICATE_INCLUDE_CHECK_H

clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp

+//===--- DuplicateIncludeCheck.cpp - clang-tidy ---------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "DuplicateIncludeCheck.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Lex/Preprocessor.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallVector.h"
+#include <memory>
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+static SourceLocation advanceBeyondCurrentLine(const SourceManager &SM,
+                                               SourceLocation Start,
+                                               int Offset) {
+  const FileID Id = SM.getFileID(Start);
+  const unsigned LineNumber = SM.getSpellingLineNumber(Start);
+  while (SM.getFileID(Start) == Id &&
+         SM.getSpellingLineNumber(Start.getLocWithOffset(Offset)) == LineNumber)
+    Start = Start.getLocWithOffset(Offset);
+  return Start;
+}
+
+namespace {
+
+using FileList = SmallVector<StringRef>;
+
+class DuplicateIncludeCallbacks : public PPCallbacks {
+public:
+  DuplicateIncludeCallbacks(DuplicateIncludeCheck &Check,
+                            const SourceManager &SM)
+      : Check(Check), SM(SM) {
+    // The main file doesn't participate in the FileChanged notification.
+    Files.emplace_back();
+  }
+
+  void FileChanged(SourceLocation Loc, FileChangeReason Reason,
+                   SrcMgr::CharacteristicKind FileType,
+                   FileID PrevFID) override;
+
+  void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
+                          StringRef FileName, bool IsAngled,
+                          CharSourceRange FilenameRange, const FileEntry *File,
+                          StringRef SearchPath, StringRef RelativePath,
+                          const Module *Imported,
+                          SrcMgr::CharacteristicKind FileType) override;
+
+  void MacroDefined(const Token &MacroNameTok,
+                    const MacroDirective *MD) override;
+
+  void MacroUndefined(const Token &MacroNameTok, const MacroDefinition &MD,
+                      const MacroDirective *Undef) override;
+
+private:
+  // A list of included files is kept for each file we enter.
+  SmallVector<FileList> Files;
+  DuplicateIncludeCheck &Check;
+  const SourceManager &SM;
+};
+
+void DuplicateIncludeCallbacks::FileChanged(SourceLocation Loc,
+                                            FileChangeReason Reason,
+                                            SrcMgr::CharacteristicKind FileType,
+                                            FileID PrevFID) {
+  if (Reason == EnterFile)
+    Files.emplace_back();
+  else
+    Files.pop_back();
+}
+
+void DuplicateIncludeCallbacks::InclusionDirective(
+    SourceLocation HashLoc, const Token &IncludeTok, StringRef FileName,
+    bool IsAngled, CharSourceRange FilenameRange, const FileEntry *File,
+    StringRef SearchPath, StringRef RelativePath, const Module *Imported,
+    SrcMgr::CharacteristicKind FileType) {
+  if (llvm::find(Files.back(), FileName) != Files.back().end()) {
+    // We want to delete the entire line, so make sure that [Start,End] covers
+    // everything.
+    SourceLocation Start =
+        advanceBeyondCurrentLine(SM, HashLoc, -1).getLocWithOffset(-1);
+    SourceLocation End =
+        advanceBeyondCurrentLine(SM, FilenameRange.getEnd(), 1);
+    Check.diag(HashLoc, "duplicate include")
+        << FixItHint::CreateRemoval(SourceRange{Start, End});
+  } else
+    Files.back().push_back(FileName);
+}
+
+void DuplicateIncludeCallbacks::MacroDefined(const Token &MacroNameTok,
+                                             const MacroDirective *MD) {
+  Files.back().clear();
+}
+
+void DuplicateIncludeCallbacks::MacroUndefined(const Token &MacroNameTok,
+                                               const MacroDefinition &MD,
+                                               const MacroDirective *Undef) {
+  Files.back().clear();
+}
+
+} // namespace
+
+void DuplicateIncludeCheck::registerPPCallbacks(
+    const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
+  PP->addPPCallbacks(std::make_unique<DuplicateIncludeCallbacks>(*this, SM));
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp

 #include "../ClangTidyModuleRegistry.h"
 #include "AvoidConstParamsInDecls.h"
 #include "BracesAroundStatementsCheck.h"
 #include "ConstReturnTypeCheck.h"
 #include "ContainerDataPointerCheck.h"
 #include "ContainerSizeEmptyCheck.h"
 #include "ConvertMemberFunctionsToStatic.h"
 #include "DeleteNullPointerCheck.h"
+#include "DuplicateIncludeCheck.h"
 #include "ElseAfterReturnCheck.h"
 #include "FunctionCognitiveComplexityCheck.h"
 #include "FunctionSizeCheck.h"
 #include "IdentifierLengthCheck.h"
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolConversionCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
 #include "IsolateDeclarationCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ContainerDataPointerCheck>(
         "readability-container-data-pointer");
     CheckFactories.registerCheck<ContainerSizeEmptyCheck>(
         "readability-container-size-empty");
     CheckFactories.registerCheck<ConvertMemberFunctionsToStatic>(
         "readability-convert-member-functions-to-static");
     CheckFactories.registerCheck<DeleteNullPointerCheck>(
         "readability-delete-null-pointer");
+    CheckFactories.registerCheck<DuplicateIncludeCheck>(
+        "readability-duplicate-include");
     CheckFactories.registerCheck<ElseAfterReturnCheck>(
         "readability-else-after-return");
     CheckFactories.registerCheck<FunctionCognitiveComplexityCheck>(
         "readability-function-cognitive-complexity");
     CheckFactories.registerCheck<FunctionSizeCheck>(
         "readability-function-size");
     CheckFactories.registerCheck<IdentifierLengthCheck>(
         "readability-identifier-length");

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`misc-misleading-identifier <clang-tidy/checks/misc-misleading-identifier>` check.
   Reports identifier with unicode right-to-left characters.
 
 - New :doc:`readability-container-data-pointer
   <clang-tidy/checks/readability-container-data-pointer>` check.
 
   Finds cases where code could use ``data()`` rather than the address of the
   element at index 0 in a container.
 
+- New :doc:`readability-duplicate-include
+  <clang-tidy/checks/readability-duplicate-include>` check.
+
+  Looks for duplicate includes and removes them.  
+
 - New :doc:`readability-identifier-length
   <clang-tidy/checks/readability-identifier-length>` check.
 
   Reports identifiers whose names are too short. Currently checks local
   variables and function parameters only.
 
 - New :doc:`misc-misleading-bidirectional <clang-tidy/checks/misc-misleading-bidirectional>` check.
 

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `portability-simd-intrinsics <portability-simd-intrinsics.html>`_,
    `readability-avoid-const-params-in-decls <readability-avoid-const-params-in-decls.html>`_, "Yes"
    `readability-braces-around-statements <readability-braces-around-statements.html>`_, "Yes"
    `readability-const-return-type <readability-const-return-type.html>`_, "Yes"
    `readability-container-data-pointer <readability-container-data-pointer.html>`_, "Yes"
    `readability-container-size-empty <readability-container-size-empty.html>`_, "Yes"
    `readability-convert-member-functions-to-static <readability-convert-member-functions-to-static.html>`_, "Yes"
    `readability-delete-null-pointer <readability-delete-null-pointer.html>`_, "Yes"
+   `readability-duplicate-include <readability-duplicate-include.html>`_, "Yes"
    `readability-else-after-return <readability-else-after-return.html>`_, "Yes"
    `readability-function-cognitive-complexity <readability-function-cognitive-complexity.html>`_,
    `readability-function-size <readability-function-size.html>`_,
    `readability-identifier-length <readability-identifier-length.html>`_,
    `readability-identifier-naming <readability-identifier-naming.html>`_, "Yes"
    `readability-implicit-bool-conversion <readability-implicit-bool-conversion.html>`_, "Yes"
    `readability-inconsistent-declaration-parameter-name <readability-inconsistent-declaration-parameter-name.html>`_, "Yes"
    `readability-isolate-declaration <readability-isolate-declaration.html>`_, "Yes"

clang-tools-extra/docs/clang-tidy/checks/readability-duplicate-include.rst

+.. title:: clang-tidy - readability-duplicate-include
+
+readability-duplicate-include
+=============================
+
+Looks for duplicate includes and removes them.  The check maintains a list of
+included files and looks for duplicates.  If a macro is defined or undefined
+then the list of included files is cleared.
+
+Examples:
+
+.. code-block:: c++
+
+  #include <memory>
+  #include <vector>
+  #include <memory>
+
+becomes
+
+.. code-block:: c++
+
+  #include <memory>
+  #include <vector>
+
+Because of the intervening macro definitions, this code remains unchanged:
+
+.. code-block:: c++
+
+  #undef NDEBUG
+  #include "assertion.h"
+  // ...code with assertions enabled
+
+  #define NDEBUG
+  #include "assertion.h"
+  // ...code with assertions disabled

clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-duplicate-include/readability-duplicate-include.h

+#ifndef READABILITY_DUPLICATE_INCLUDE_H
+#define READABILITY_DUPLICATE_INCLUDE_H
+
+extern int g;
+#include "readability-duplicate-include2.h"
+extern int h;
+#include "readability-duplicate-include2.h"
+extern int i;
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: duplicate include
+// CHECK-FIXES:      {{^extern int g;$}}
+// CHECK-FIXES-NEXT: {{^#include "readability-duplicate-include2.h"$}}
+// CHECK-FIXES-NEXT: {{^extern int h;$}}
+// CHECK-FIXES-NEXT: {{^extern int i;$}}
+
+#endif

clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-duplicate-include/readability-duplicate-include2.h

+// This file is intentionally empty.

clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-duplicate-include/system/iostream

+// This file is intentionally empty.

clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-duplicate-include/system/string.h

+// This file is intentionally empty.

clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-duplicate-include/system/sys/types.h

+// This file is intentionally empty.

clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-duplicate-include/system/types.h

+// This file is intentionally empty.

clang-tools-extra/test/clang-tidy/checkers/readability-duplicate-include.cpp

+// RUN: %check_clang_tidy %s readability-duplicate-include %t -- -- -isystem %S/Inputs/readability-duplicate-include/system -I %S/Inputs/readability-duplicate-include
+
+int a;
+#include <string.h>
+int b;
+#include <string.h>
+int c;
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: duplicate include [readability-duplicate-include]
+// CHECK-FIXES:      {{^int a;$}}
+// CHECK-FIXES-NEXT: {{^#include <string.h>$}}
+// CHECK-FIXES-NEXT: {{^int b;$}}
+// CHECK-FIXES-NEXT: {{^int c;$}}
+
+int d;
+#include <iostream>
+int e;
+#include <iostream> // extra stuff that will also be removed
+int f;
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: duplicate include
+// CHECK-FIXES:      {{^int d;$}}
+// CHECK-FIXES-NEXT: {{^#include <iostream>$}}
+// CHECK-FIXES-NEXT: {{^int e;$}}
+// CHECK-FIXES-NEXT: {{^int f;$}}
+
+int g;
+#include "readability-duplicate-include.h"
+int h;
+#include "readability-duplicate-include.h"
+int i;
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: duplicate include
+// CHECK-FIXES:      {{^int g;$}}
+// CHECK-FIXES-NEXT: {{^#include "readability-duplicate-include.h"$}}
+// CHECK-FIXES-NEXT: {{^int h;$}}
+// CHECK-FIXES-NEXT: {{^int i;$}}
+
+#include <types.h>
+
+int j;
+#include <sys/types.h>
+int k;
+#include <sys/types.h>
+int l;
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: duplicate include
+// CHECK-FIXES:      {{^int j;$}}
+// CHECK-FIXES-NEXT: {{^#include <sys/types.h>$}}
+// CHECK-FIXES-NEXT: {{^int k;$}}
+// CHECK-FIXES-NEXT: {{^int l;$}}
+
+int m;
+        #          include             <string.h>  // lots of space
+int n;
+// CHECK-MESSAGES: :[[@LINE-2]]:9: warning: duplicate include
+// CHECK-FIXES:      {{^int m;$}}
+// CHECK-FIXES-NEXT: {{^int n;$}}
+
+// defining a macro in the main file resets the included file cache
+#define ARBITRARY_MACRO
+int o;
+#include <sys/types.h>
+int p;
+// CHECK-FIXES:      {{^int o;$}}
+// CHECK-FIXES-NEXT: {{^#include <sys/types.h>$}}
+// CHECK-FIXES-NEXT: {{^int p;$}}
+
+// undefining a macro resets the cache
+#undef ARBITRARY_MACRO
+int q;
+#include <sys/types.h>
+int r;
+// CHECK-FIXES:      {{^int q;$}}
+// CHECK-FIXES-NEXT: {{^#include <sys/types.h>$}}
+// CHECK-FIXES-NEXT: {{^int r;$}}