entt icon indicating copy to clipboard operation
entt copied to clipboard

Feature request: include-what-you-use pragmas, or mapping file

Open jonesmz opened this issue 4 years ago • 11 comments

I am trying to integrate include-what-you-use into my project's continuous integration pipeline, and address all of the complaints that its' giving me.

Background

include-what-you-use use will attempt to evaluate a header or implementation file, and determine which symbols are actually used in that header / implementation file. Then it tries to find the header file where those symbols are declared, and determine if the list of headers being included matches.

What I've found is that IWYU can frequently help cut a project's compile times down by even as much as half, depending on the project in question. That level of savings requires the project to have terrible include discipline before using IWY, of course. It does this by helping a project get to the "true" list of headers needed for inclusion on a per .h/.cpp basis.

However, IWYU is not without it's problems, and sometimes it has a hard time determining if a symbol is intended to be provided by a particular header, or if it's just incidental.

But to my request: I'm noticing that IWYU is complaining about missing includes of <type_traits> all over my code, even for places where MY code does not use <type_traits> related stuff. Why would it be doing that? Because I'm using functions from other projects (EnTT is only one culprit) that are including <type_traits> and then using them in functions that I'm calling. Thanks to SFINAE being particularly difficult for IWYU to understand, this results in IWYU asking me to include <type_traits>

Why should EnTT care?

Reason for caring is not because EnTT itself is including the wrong list of headers. As far as I can tell, this project does a good job of only including the headers that are needed.

Reason for caring would be to help projects that consume EnTT get a good out-of-the-box experience with header analyses tools like IWYU.

What can EnTT do?

EnTT can do one of two things:

The best out of the box experience would be to annotate #include statements in the EnTT source code with the pragmas defined here: https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/IWYUPragmas.md

For example, https://github.com/skypjack/entt/blob/master/src/entt/entt.hpp is intended to "export" all of the headers that it includes, and a project that chooses to use entt.hpp, instead of individual includes, wouldn't want IWYU to suggest the underlying include files instead, that would just be noise for the sake of noise.

Whether the whole <type_traits> situation that originally prompted me to post about this is something EnTT can or should address is unknown to me. I kind of feel like that's an IWYU bug, but it's still kind of a pain :-)

Alternatively, EnTT can provide an IWYU "map" file that describes the "correct" header for various symbols for situations where IWYU get's it wrong. That map file can then be used by consuming projects to fix IWYU's suggestions.

Personally, I'm ambivalent either way, since most projects have to use map files to deal with compiler built-in and/or standard library headers, so whichever.

jonesmz avatar Sep 04 '21 18:09 jonesmz

I don't know IWYU, therefore sorry if my questions are particularly stupid. :) What's exactly an IWYU map? My concern is that this request risks to add a maintenance burden and I'd like to avoid it or to reduce it to a minimum at least.

skypjack avatar Sep 04 '21 19:09 skypjack

Ah, sorry, I should have clarified that for you.

Link: https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/IWYUMappings.md

A "map" file is basically a file that override's the built in rules for IWYU.

For example, most compilers / standard libraries actually implement things like allocators inside of a collection of headers such as <ext/alloc_traits.h> and <ext/allocator_blahblahblah.h>. A map file can tell IWYU "Even though these types are ACTUALLY defined in <ext/blahblah> instead, if you encounter a use of them, only complain if the include that this mapping file says to use is not present, and then if you do complain, say to include instead.

For example, this is my current IWYU mapping file

[
    { ref: /usr/lib/llvm/12/share/include-what-you-use/gcc.libc.imp }
  , { ref: /usr/lib/llvm/12/share/include-what-you-use/gcc.symbols.imp }
  , { ref: /usr/lib/llvm/12/share/include-what-you-use/gcc.stl.headers.imp }
  , { ref: /usr/lib/llvm/12/share/include-what-you-use/stl.c.headers.imp }
  , { include: [ "<ext/alloc_traits.h>", private, "<memory>", public ] }
  , { include: [ "<ext/alloc_traits.h>", private, "<string>", public ] }
  , { include: [ "<ext/alloc_traits.h>", private, "<vector>", public ] }
  , { include: [ "<ext/alloc_traits.h>", private, "<map>", public ] }
  , { include: [ "<ext/alloc_traits.h>", private, "<set>", public ] }
]

Which says to use all the built-ins that are listed, as well as re-map <ext/alloc_traits.h> -> <memory> and similar. Mappings are ordered first-preference to last preference, but if any of the mappings are satisified, no warning is emitted. E.g. including <set> satisfies any situation where IWYU would recommend including <ext/alloc_traits.h>, but if IWYU wants to recommend, it'll always recommend the first recommendation (<memory>)

Note that most of the time, a mapping file should not try to reference the IWYU shipped map files like that. I'm only doing that because I'm the "Top Level" project in my IWYU usage.

For sake of this example, lets assume that EnTT has some type entt::Example with entt::Example having a hard dependency on entt::Dependency.

entt::Example comes from "example.h", and Dependency from "dependency.h". One would say that in never makes sense for a consumer of "example.h" to not also include "dependency.h", and that entt::Dependency must be always fully visable to any code that is using entt::Example

So to accomplish this, in your "example.h" file, you would annotate your include as

#include "dependency.h" // IWYU pragma: export

Or you would do a mapping file like

[
   { include: [ "dependency.h", private, "example.h", public ] }
]

Note though that just because "dependency.h" is exported by "example.h", doesn't mean that IWYU will say "dependency.h" should be replaced with "example.h". But IWYU will recommend removing "dependency.h" if "example.h" is also included.

Like I tried to indicate, I don't know that there's a whole lot that EnTT can practically do beyond having good header file discipline (which EnTT seems to have).

The fwd.hpp file probably should have that export pragma wrapped around all of it's includes, but I can't currently point to any other specific file and say "EnTT should absolutely be adding an IWYU pragma right here" just yet. But I'll point them out if I run into any.

Basically my reason for bringing this up is to bring the potential for issues to your attention.

I would consider a commit that adds the IWYU export pragma to fwd.hpp as sufficent to close this issue on github, as long as you're also willing to consider other areas for IWYU hand-holding in the future as a "back of the mind" thought.

Another idea might be to add IWYU to your continuous integration, and then if it complains, that's a good area to investigate the use of pragmas.

An example of using IWYU in a github action is here:

name: Analyzers

on:
  pull_request:
  push:
  release:
    types: published

jobs:
  build:
    runs-on: ubuntu-latest

    strategy:
      matrix:
        tool: [clang-analyzer, iwyu]
        config: [Release, Debug]

    steps:
    - uses: actions/checkout@v2
      with:
        fetch-depth: 1
        submodules: recursive

    - name: Install dependencies
      run: |
        sudo apt-get update
        sudo apt install -y libglfw3-dev libopenal-dev libglvnd-core-dev iwyu clang clang-tidy clang-tools
        # TODO: Really, this should only be fetching the build dependencies, so we can only use the in-tree version
        sudo apt install -y libsdl2-dev

    - name: Configure
      run: |
        cmake -B build -DCMAKE_BUILD_TYPE=R${{ matrix.config }}

    - name: Download Clang Analysis Driver Scripts
      run: |
        wget https://raw.githubusercontent.com/llvm/llvm-project/llvmorg-11.0.1/clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
        wget https://raw.githubusercontent.com/include-what-you-use/include-what-you-use/clang_11/iwyu_tool.py
        chmod +x run-clang-tidy.py iwyu_tool.py

    # Implicitly requires build/compile_commands.json to exist
    - name: Run Clang Analyzer
      if: ${{ matrix.tool == 'clang-analyzer' }}
      run: |
        ./run-clang-tidy.py -p build

    # Implicitly requires build/compile_commands.json to exist
    - name: Run IWYU
      if: ${{ matrix.tool == 'iwyu' }}
      run: |
        ./iwyu_tool.py -p build -- -Xiwyu --mapping_file=${GITHUB_WORKSPACE}/iwyu-ubuntu.imp

jonesmz avatar Sep 04 '21 19:09 jonesmz

As for maintenance burden: You're certainly right to be cautious about that.

I'm not recommending annotating every single file in every single place. Generally IWYU doesn't have too many false-positives.

I'm really only trying to say that anything that's low-hanging fruit should be addressed, and then only the most annoying of high-hanging fruit should be in scope for EnTT to care about.

jonesmz avatar Sep 04 '21 19:09 jonesmz

Oh, one thing to note:

My CI example above, requires

set_target_properties(${PROJECT_NAME} PROPERTIES EXPORT_COMPILE_COMMANDS TRUE)

Or CMAKE_EXPORT_COMPILE_COMMANDS to be set to true on the cmake configure step globally.

Just thought I should point that out.

The project that that github action was copied from sets the compile commands flag only on the 1st party target, so that 3rd party code doesn't get scanned by clang-analyser. So only set that in your CMake script if you're building your own unit tests, I guess, to avoid forcing consumers of EnTT to have it set.

jonesmz avatar Sep 04 '21 19:09 jonesmz

Ah, here's an example of where entt could make IWYU be less noisy.

<entt/registry.hpp> depends on <entt/sparse_set.hpp>, in the sense of the EnTT::Example and EnTT::Dependency that I described above.

Changing line 25 of <entt/registry.hpp> from

#include "sparse_set.hpp"

to

#include "sparse_set.hpp" // IWYU pragma: export

Would shut IWYU up quite a bit.

jonesmz avatar Sep 04 '21 20:09 jonesmz

Out of curiosity, do you think it's possible to add it to the CI on GH? Dunno if you've ever tried this. At a first glance, IWYU works fine with cmake, so it may be an option to keep things up-to-date over time.

skypjack avatar Sep 05 '21 20:09 skypjack

The YAML snippet i pasted in a previous comment is a full github action script that you can add to your repo.

See how i use it here https://github.com/TheOpenSpaceProgram/osp-magnum/blob/master/.github/workflows/analyzers.yml

jonesmz avatar Sep 05 '21 22:09 jonesmz

Oh, right, my fault. I should re-read a thread before asking. Completely forgotten. Sorry.

skypjack avatar Sep 05 '21 23:09 skypjack

So, I've created a workflow for iwyu (see here) and it spits out a lot of false positives from what I see. 😞 Even things like _don't include <gtest/gtest.h>, include "gtest/gtest.h"_. These are quite annoying for example. Is there any way to silence some of these warnings, like false positives on path or stuff from the standard library?

skypjack avatar Jan 31 '22 15:01 skypjack

I gave it a shot, not sure which parts of my changes are strictly needed, so some minimization might be worth doing.

https://github.com/jonesmz/entt/tree/iwyu

The gtest annoyance i think i fixed

Looks like there's also iwyu's typical annoyance with not considering double-quotes and angle-brackets to be the same thing, so some rules need to be duplicated to get it to shut up.

We're also dealing with a release of iwyu that's several old, so some of these might have been addressed since.

jonesmz avatar Feb 14 '22 01:02 jonesmz

We're also dealing with a release of iwyu that's several old, so some of these might have been addressed since.

Good point. I think it's easy to download a more updated version from the CI though. I'll give it a try. 👍

skypjack avatar Feb 15 '22 07:02 skypjack

Closing this as an iwyu workflow exists now and I'll refine the whole thing a little at a time when I've time. 👍

skypjack avatar Aug 31 '22 09:08 skypjack