[clang-tidy] Add readability test for not allowing relative includes

[ErezAmihud]

Summary

This pr adds a check that warns if it finds a relative include. This is to allow people to easily conform to the canonical project structure - https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1204r0.html

tl;dr The check issue a warning for every relative include (e.g. any include that uses regular quotes).

Github issue:
https://github.com/llvm/llvm-project/issues/63226

[ErezAmihud]

Remove a line in absolute-includes-only.cpp (fix formatting)

[Eugene.Zelenko]

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

17

Excessive newline.

clang-tools-extra/docs/ReleaseNotes.rst

187

Please make it as first statement in documentation.

clang-tools-extra/docs/clang-tidy/checks/readability/absolute-includes-only.rst

9

Please highlight "" and <> with double back-ticks. Please follow 80-characters limit.

18

Please split example on right and wrong blocks.

clang-tools-extra/test/clang-tidy/checkers/readability/absolute-includes-only.cpp

14

Excessive newline.

18

Ditto.

[PiotrZSL]

First this looks like "misc" check, not as "readability" one. So please move it to proper module. There is no "readability" aspect of it.
Second thing this is not an "absolute", absolute would be #include "/usr/includes/stdio", with this style you can still use relative paths like #include <../something>
Proper names then would be one of: misc-no-quote-includes or misc-use-angle-bracket-includes (this one looks to sound better), choose one, any.

and again naming, those are not relative and absolute, those are quote and angle-bracket, avoid confusion with path put into include.

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

43

inline this method in AbsoluteIncludesOnlyPPCallbacks no need to keep it out of class.

48

i think that Imported!=nullptr should also do a trick, but this way shoud be fine. Check if getIdentifierInfo is not null to be sure

51

getLocWithOffset(0) does nothing, remove

53–54

1. You could provide auto-fix here, but if you do not want to do that its fine.
2. Warning is a default, no need to specify it.
3. put included header name into message, it will make way easier for reader to find out whats going on
4. consider excluding includes included from system headers on this level, otherwise check may generate lot of includes that going to be filter out later in process, but that still may be costly.

clang-tools-extra/docs/ReleaseNotes.rst

188

sounds like "do not eat cheese, eat cheese", align later this documentation with check name

clang-tools-extra/docs/clang-tidy/checks/readability/absolute-includes-only.rst

11

format this url properly, look into other checks how they do it in documentation

11

80 characters limit

17

uncomment those includes

18

whats wrong here ? check wont raise warning here

[PiotrZSL]

One more comment, according to this whitepaper you should actually check if include is relative, simply if current file directory + include exists and it's actually a file that is included, then its relative include.
Currently you checking only "" and its not a thing that should be done, because include like this `#include "hello/utility.hpp" is also valid.
Therefor you should be checking included path, not an include style, and this is main problem of this check.
In such case proper name for it would be misc-no-relative-includes, and entire implementation of the check should be updated.
Also note that some projects put header files in same folder as source file, and uses "" include for them, this is one of the ways to deal with private/public includes.

[PiotrZSL]

Even with that for me for example, I would consider #include "xyz" be fine, but #include "../xyz" would be a red flag, so please consider adding StrictMode option to check so if StrictMode is not set then includes from same directory, or subdirectory would be allowed, but includes that contain .. would not.
With that settings, I could use that check, and probably many other users could.

[ErezAmihud]

In D152589#4410930, @PiotrZSL wrote:

Even with that for me for example, I would consider #include "xyz" be fine, but #include "../xyz" would be a red flag, so please consider adding StrictMode option to check so if StrictMode is not set then includes from same directory, or subdirectory would be allowed, but includes that contain .. would not.
With that settings, I could use that check, and probably many other users could.

I want to make sure I understand.
The ideal situation would be to create misc-no-relative-includes check that checks for .. in paths (meaning - relative paths), and have a StrictMode which does the check for angled-bracket includes?

BTW The reason I initially checked for angled-bracket includes is that according to this quote includes search in paths relative to the current file, and the angled-bracket includes check only in the defined include directories.
In this case there is no risk that the include would get a file that is not relative to the include directory the user specified in the compile flags.

[PiotrZSL]

In D152589#4418649, @ErezAmihud wrote:

I want to make sure I understand.
The ideal situation would be to create misc-no-relative-includes check that checks for .. in paths (meaning - relative paths), and have a StrictMode which does the check for angled-bracket includes?

BTW The reason I initially checked for angled-bracket includes is that according to this quote includes search in paths relative to the current file, and the angled-bracket includes check only in the defined include directories.
In this case there is no risk that the include would get a file that is not relative to the include directory the user specified in the compile flags.

But according to paper you can also do -I<path to .cpp directory> and in .cpp you can do #include <header.hpp> and if that header.hpp exist near .cpp then this is still relative include.
For me we should do something like this:

path_to_cpp_directory = absolutepath(dirname(main-file))
include_file = argument to #include <> or #include ""
if (actually_included_file_is(path_to_cpp_directory  + include_file))
  if (StrictMode || !absolutepath(path_to_cpp_directory  + include_file).starts_with(path_to_cpp_directory))
       // relative include, generate warning

[ErezAmihud]

Closed, because I don't have time to work on it

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

+//===--- AbsoluteIncludesOnlyCheck.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_ABSOLUTEINCLUDESONLYCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_ABSOLUTEINCLUDESONLYCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::readability {
+
+/// A check to not allow relative includes (includes using quotes).
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability/absolute-includes-only.html
+class AbsoluteIncludesOnlyCheck : public ClangTidyCheck {
+public:
+  AbsoluteIncludesOnlyCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+                           Preprocessor *ModuleExpanderPP) override;
+};
+
+} // namespace clang::tidy::readability
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_ABSOLUTEINCLUDESONLYCHECK_H

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

+//===--- AbsoluteIncludesOnlyCheck.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 "AbsoluteIncludesOnlyCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Lex/PPCallbacks.h"
+#include "clang/Lex/Preprocessor.h"
+
+namespace clang::tidy::readability {
+
+namespace {
+class AbsoluteIncludesOnlyPPCallbacks : public PPCallbacks {
+public:
+  explicit AbsoluteIncludesOnlyPPCallbacks(AbsoluteIncludesOnlyCheck &Check,
+                                           const SourceManager &SM)
+      : Check(Check) {}
+
+  void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
+                          StringRef FileName, bool IsAngled,
+                          CharSourceRange FilenameRange,
+                          OptionalFileEntryRef File, StringRef SearchPath,
+                          StringRef RelativePath, const Module *Imported,
+                          SrcMgr::CharacteristicKind FileType) override;
+
+private:
+  AbsoluteIncludesOnlyCheck &Check;
+};
+} // namespace
+
+void AbsoluteIncludesOnlyCheck::registerPPCallbacks(
+    const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
+  PP->addPPCallbacks(
+      ::std::make_unique<AbsoluteIncludesOnlyPPCallbacks>(*this, SM));
+}
+
+void AbsoluteIncludesOnlyPPCallbacks::InclusionDirective(
+    SourceLocation HashLoc, const Token &IncludeTok, StringRef FileName,
+    bool IsAngled, CharSourceRange FilenameRange, OptionalFileEntryRef File,
+    StringRef SearchPath, StringRef RelativePath, const Module *Imported,
+    SrcMgr::CharacteristicKind FileType) {
+  if (IncludeTok.getIdentifierInfo()->getPPKeywordID() == tok::pp_import)
+    return;
+
+  SourceLocation DiagLoc = FilenameRange.getBegin().getLocWithOffset(0);
+  if (!IsAngled) {
+    Check.diag(DiagLoc, "relative include found, use only absolute includes",
+               DiagnosticIDs::Warning);
+  }
+}
+
+} // namespace clang::tidy::readability

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

   )
 
 add_clang_library(clangTidyReadabilityModule
+  AbsoluteIncludesOnlyCheck.cpp
   AvoidConstParamsInDecls.cpp
   AvoidUnconditionalPreprocessorIfCheck.cpp
   BracesAroundStatementsCheck.cpp

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

 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "AbsoluteIncludesOnlyCheck.h"
 #include "AvoidConstParamsInDecls.h"
 #include "AvoidUnconditionalPreprocessorIfCheck.h"
 #include "BracesAroundStatementsCheck.h"

 class ReadabilityModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<AbsoluteIncludesOnlyCheck>(
+        "readability-absolute-includes-only");
     CheckFactories.registerCheck<AvoidConstParamsInDecls>(
         "readability-avoid-const-params-in-decls");
     CheckFactories.registerCheck<AvoidUnconditionalPreprocessorIfCheck>(

clang-tools-extra/docs/ReleaseNotes.rst

 
   Finds uses of ``std::endl`` on streams and replaces them with ``'\n'``.
 
+- New :doc:`readability-absolute-includes-only
+  <clang-tidy/checks/readability/absolute-includes-only>` check.
+
+Finds relative includes in your code and warn about them. for example
+don't use "readability/absolute-includes-only", use <clang-tidy/checks/readability/absolute-includes-only>
+
 - New :doc:`readability-avoid-unconditional-preprocessor-if
   <clang-tidy/checks/readability/avoid-unconditional-preprocessor-if>` check.
 

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

    `portability-restrict-system-includes <portability/restrict-system-includes.html>`_, "Yes"
    `portability-simd-intrinsics <portability/simd-intrinsics.html>`_,
    `portability-std-allocator-const <portability/std-allocator-const.html>`_,
+   `readability-absolute-includes-only <readability/absolute-includes-only.html>`_,
    `readability-avoid-const-params-in-decls <readability/avoid-const-params-in-decls.html>`_, "Yes"
    `readability-avoid-unconditional-preprocessor-if <readability/avoid-unconditional-preprocessor-if.html>`_,
    `readability-braces-around-statements <readability/braces-around-statements.html>`_, "Yes"

clang-tools-extra/docs/clang-tidy/checks/readability/absolute-includes-only.rst

+.. title:: clang-tidy - readability-absolute-includes-only
+
+readability-absolute-includes-only
+==================================
+
+Finds relative includes inside of the code and warn about them.
+
+Meaning this check disallow the use of quote includes (""), and allow only angular brackets includes (<>).
+
+This is relevant for the canonical project structure specified in paper p1204r0.
+The relevant part is the src-dir where relative includes are discussed: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1204r0.html#src-dir
+
+Example (taken from the paper):
+
+.. code-block:: c++
+
+  // #include "utility.hpp"           // Wrong.
+  // #include <utility.hpp>           // Wrong.
+  // #include "../hello/utility.hpp"  // Wrong.
+
+  #include <hello/utility.hpp>

clang-tools-extra/test/clang-tidy/checkers/readability/absolute-includes-only.cpp

+// RUN: %check_clang_tidy %s readability-absolute-includes-only %t -- -- -isystem %clang_tidy_headers -fmodules
+
+// clang-format off
+
+// CHECK-MESSAGES: [[@LINE+1]]:10: warning: relative include found, use only absolute includes
+#include "j.h"
+
+// CHECK-MESSAGES: [[@LINE+1]]:10: warning: relative include found, use only absolute includes
+#include "llvm/a.h"
+
+// CHECK-MESSAGES: [[@LINE+1]]:10: warning: relative include found, use only absolute includes
+#include "clang/../b.h"
+
+
+#include <s.h>
+
+#include <string>
+