llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
27.57k stars 11.33k forks source link

clang-tidy misc-include-cleaner does not detect `chrono` header #98122

Open andrea-cassioli-maersk opened 1 month ago

andrea-cassioli-maersk commented 1 month ago

I have noticed that when running with misc-include-cleaner option, clang-tidy seems not to detect that the chrono header is provided and used.

In one of my project file you can find

#include <algorithm>
#include <chrono>
#include <functional>
#include <set>
#include <string>

// some other code

 const auto time_residual = rotation_time - std::chrono::round<std::chrono::weeks>(t);
  constexpr std::chrono::minutes kThreshold{30};
  if (std::chrono::abs(time_residual) > kThreshold) {
    results << ErrorMessage("FOO");
  }

// and more code goes here

Running clang-tidy I get

/Users/<my-path>/foo.cpp:241:68: error: no header providing "std::chrono::round" is directly included [misc-include-cleaner,-warnings-as-errors]
    2 |   const auto time_residual = rotation_time - std::chrono::round<std::chrono::weeks>(t);
      |   

and the header #include <__chrono/duration.h> is added to the file.

Notice it is not only std::chrono::round that is not found, but std::chrono::weeks as well.

I am running on

➜  $ clang-tidy --version
Homebrew LLVM version 18.1.8
  Optimized build.
firewave commented 1 month ago

Related to #68213 which shows the same issue using libstdc++.

llvmbot commented 1 month ago

@llvm/issue-subscribers-clang-include-cleaner

Author: Andrea Cassioli (andrea-cassioli-maersk)

I have noticed that when running with `misc-include-cleaner` option, clang-tidy seems not to detect that the `chrono` header is provided and used. In one of my project file you can find ``` #include <algorithm> #include <chrono> #include <functional> #include <set> #include <string> // some other code const auto time_residual = rotation_time - std::chrono::round<std::chrono::weeks>(t); constexpr std::chrono::minutes kThreshold{30}; if (std::chrono::abs(time_residual) > kThreshold) { results << ErrorMessage("FOO"); } // and more code goes here ``` Running clang-tidy I get ``` /Users/<my-path>/foo.cpp:241:68: error: no header providing "std::chrono::round" is directly included [misc-include-cleaner,-warnings-as-errors] 2 | const auto time_residual = rotation_time - std::chrono::round<std::chrono::weeks>(t); | ``` and the header `#include <__chrono/duration.h>` is added to the file. Notice it is not only `std::chrono::round` that is not found, but `std::chrono::weeks` as well. I am running on ``` ➜ $ clang-tidy --version Homebrew LLVM version 18.1.8 Optimized build. ```
mordante commented 1 month ago

Is there anything libc++ can do or is this a clang-include-cleaner issue? Based on that it also happens with libstdc++ I expect it's a clang-include-cleaner issue.

firewave commented 1 month ago

I checked some fixed issues and based on 586e497f7985ddb0ca481b45f82e202d3b6f23ea I assume it is caused by missing mappings in clang/lib/Tooling/Inclusions/Stdlib/StdSymbolMap.inc.

firewave commented 1 month ago

Actually it looks like a shortcoming of the script which generates the mapping. Using std::chrono as example it seems it omits symbols which have a parameter specified in the documentation at https://en.cppreference.com/w/cpp/header/chrono (e.g. ceil(std::chrono::duration)). Those include ceil and round which are the culprits in the related tickets.

CC @hokein

mordante commented 1 month ago

It's indeed documented as not supported in llvm/clang/tools/include-mapping/gen_std.py. Since this is not an issue in libc++ I'll remove the label.

andrea-cassioli-maersk commented 1 month ago

If it can help, we have seen this happening also for chrono types like weeks

mordante commented 1 month ago

Based on the comment in the script it happens to everything in the namespace std::foo. Where foo is chrono, ranges, filesystem, etc.

hokein commented 1 month ago

The gen_std.py script does support generating std subnamespace symbols. It appears that the FIXME comment is stale.

The script parses all symbol lists from symbol index pages, e.g., https://en.cppreference.com/w/cpp/symbol_index/chrono, so only symbols listed there will be included. The current StdSymbolMap.inc file was generated from an older cppreference offline book, which might miss some newly-added symbols (I guess std::chrono::round). I think we should consider rerunning the script on a newer cppreference offline book.

firewave commented 1 month ago

According to the Wayback Machine the chrono page has not changed since December 4, 2021. The missing symbols have <>() at the end, so maybe that is causing them to be left out.

firewave commented 1 month ago

I took a look at the script using the previously used version of the reference.

It skips these symbols because there are variants for these functions - e.g. std::chrono::ceil() can take either std::chrono::duration or std::chrono::time_point.