Proposal to enable clang-tidy as a required CI for Autoware

[veqcc]

Introduction

To improve the code quality of Autoware, we are currently using clang-tidy on GitHub CI.
However, the results of clang-tidy are always discarded and all PRs are merged with warnings, which leads to 15,000 warnings of clang-tidy for Autoware.
My proposal is narrowing down the clang-tidy check features (written in .clang-tidy file) and making it required to pass all checks in each PR.

This is a similar proposal with https://github.com/orgs/autowarefoundation/discussions/4827.

Current progress

We have analyzed the current 15,000 clang-tidy warnings and classified them based on codechecker criteria.

"C" is "CRITICAL" level warnings, which means it failed to compile with clang.

"H" is "HIGH" level warnings. Out of 107 "HIGH" features prepared in clang-tidy, the current Autoware enables 18 features, and 6 of them offer some warnings. In other words, Autoware passes 12 features of them.

"M" is "MIDIUM" level warnings. Out of 1098 "MIDIUM" features prepared in clang-tidy, the current Autoware enables 40 features, and 13 of them offer some warnings. In other words, Autoware passes 27 features of them.

"L", "S" and "U" are "LOW", "STYLE" and "UNKNOWN", respectively. I will ignore these low level warnings this time.

You can find all clang-tidy warnings and the features currencly enabled in Autoware.

Proposal

We propose the following two things:

• reduce the clang-tidy check features so that the check results are useful for developers.
• make the clang-tidy-differential workflow a required check in our CI pipeline to ensure the code quality to some extent.

I recommend to enable the following features, based on the analysis above:

• 12 features which have "HIGH" level warnings and the current Autoware passes
• 27 features which have "MIDIUM" level warnings and the current Autoware passes

Future Plan

We will first solve the "CRITICAL" level warnings since these prevent clang-tidy from working well.
After that, we will move on to removing "HIGH" and "MIDIUM" level warnings which the current Autoware cannot pass.
When we succeed to remove all the warnings related a feature among them, we want to add the feature to check list and make it required.

Request for Feedback

If you have any comments or suggestions regarding this proposal, please feel free to provide your feedback!

[mitsudome-r]

@veqcc Could you provide some examples on what kind of critical level warning we have to give us some idea?

[veqcc]

@mitsudome-r

We have 40~50 CRITICAL level warnings, and they are classified as the following 9 type warnings:

• unknown type name 'nullptr_t'; did you mean 'std::nullptr_t'?
• constexpr variable 'hoge' must be initialized by a constant expression
• variable 'hoge' cannot be implicitly captured in a lambda with no capture-default specified
• no viable conversion from 'hoge' to 'fuga'
• reference to local binding 'hoge' declared in enclosing function 'fuga'
• chosen constructor is explicit in copy-initialization
• no matching function for call to 'hoge'
• expected ';' after struct
• 'hoge' attribute only applies to variables, data members and tag types

hoge and fuga are used to abstract actual function or variable names.

[veqcc]

@mitsudome-r @xmfcx
Hi!
We have completed removing almost all CIRITICAL severity warnings (which corresponds to clang compile errors) from Autoware!

So I created a PR to reduce the clang-tidy checkings so that clang-tidy-differential workflow can be required for each PR in autoware.universe.
I would be happy if you review the PR! Thank you.

[amadeuszsz]

@veqcc
Great idea with these changes, actual clang configuration needs to be reviewed. Some check currently are too strict but I just wonder if suppressing most of checks is the best way. Personally, I think with these checks we encourage developers for future code improvements. Another thing is our code style (readability key) - we could come up with one convention and just follow it.

[veqcc]

@amadeuszsz

Thank you for your comment!

Some check currently are too strict but I just wonder if suppressing most of checks is the best way. Personally, I think with these checks we encourage developers for future code improvements.

Agreed.
As @.mojomex mentions below, it seems better enabling all the warnings, but not as errors, so that the clang-tidy can provide useful infomation for developers.

Another thing is our code style (readability key) - we could come up with one convention and just follow it.

Agreed.
However, code style warnings tend to be low severity.
It's OK to make them enabled as warnings, but do you think we should make them required by assuming them as errors?

[amadeuszsz]

@veqcc Warnings are OK. I assumed no style in this proposal since PR removed all the style checks.

[veqcc]

Underdtood, thank you!!
I have restored all the original configurations so that this change will not break developers local environments.
And I made a new confiration clang-tidy-ci for CI, which checks the same warnings as is, while severity HIGH and MEDIUM warnings are treated as errors and prevent PR from being merged.

[mojomex]

@veqcc
Hi!
Is this a temporary fix to 'just to make CI pass' or will the new clang-tidy file remain for longer?
There are a few points I would like to address regarding your proposed approach:

• People (me included) rely on this file for code analysis in their IDE and thus I would want these checks to remain:
  • checks like modernize-
  • style checks
  • style guidelines
• It would be better in my opinion to leave all checks in, and just mark the non-HIGH and non-CRITICAL ones as warnings. Then, people would still see what needs to be improved, but Autoware CI would still pass

I would be happy for your thoughts on this!

[xmfcx]

We can keep the default .clang-tidy file as is (for default IDE configurations).

And keep a copy of it under a different name like .clang-tidy-ci to run with the CI check.

And for this .clang-tidy-ci file, we can do as @mojomex suggested to make transition smoother.

What do you think @veqcc @mojomex ?

[xmfcx]

Also overall, reducing the passing changes to minimum and adding them gradually is a good way to proceed.

For example one of the open bounties in the comma.ai/openpilot project for panda repository is about finishing the MISRA coverage:

• Finish MISRA coverage commaai/panda#1794
• Complete MISRA coverage commaai/panda#1954

They enable them rule by rule over time.

[veqcc]

@mojomex @xmfcx

Thank you for your comments!

As a conclusion, I basically agree to your opinions.
I will add a commit to restore the original .clang-tidy and create .clang-tidy-ci configuration.

just mark the non-HIGH and non-CRITICAL ones as warnings.

One thing I want to discuss is, which warnings should be required in each PR. In other words, which warnings should be assumed as errors.
My first proposal is to make CRITIACL, HIGH, MEDIUM severity warings should be errors, excluding those which occur in the current autoware.universe.
What do you think?

[xmfcx]

My first proposal is to make CRITICAL, HIGH, MEDIUM severity warnings should be errors, excluding those which occur in the current autoware.universe.

I think that's alright. Especially, considering that we have no checks right now. Anything will be better than what we have now.

[veqcc]

I have restored the original .clang-tidy configuration here
5192d5a

And I made MEDIUM and HIGH severity required for CI, while others are treated as warnings here
3431c51

[veqcc]

@xmfcx
Another problem was reported. 🥹

In the current clang-tidy configuration, /usr and /opt directories are ignored here:

autoware/.clang-tidy

Line 203 in ada7b8c

HeaderFilterRegex: ^(?!\/usr)(?!\/opt)

However, it seems not working propoerly.
For example, in this PR, clang-tidy reports errors in /usr directories.

I'll make a deeper investivation later.

[xmfcx]

@VRichardJP how can I reach you through direct messaging? I've sent you a message/friend request from discord.

We only have ccache though and it shouldn't affect anything but I don't know.

CI also uses ccache too btw.

[VRichardJP]

This was my modified build/map_loader/compile_commands.json

[
{
  "directory": "/home/sig/autoware/build/map_loader",
  "command": "/usr/bin/c++ -Wno-enum-constexpr-conversion -I/home/sig/autoware/src/universe/autoware.universe/map/map_loader/include  -I/opt/ros/humble/include/rosidl_runtime_c -I/opt/ros/humble/include/rcutils -I/opt/ros/humble/include/rosidl_typesupport_interface -I/opt/ros/humble/include/fastcdr -I/opt/ros/humble/include/rosidl_typesupport_fastrtps_cpp -I/opt/ros/humble/include/rmw -I/opt/ros/humble/include/rosidl_typesupport_fastrtps_c -I/opt/ros/humble/include/rosidl_typesupport_introspection_c -I/opt/ros/humble/include/rosidl_typesupport_introspection_cpp -I/opt/ros/humble/include/ament_index_cpp -I/opt/ros/humble/include/libstatistics_collector -I/opt/ros/humble/include/rcl_interfaces -I/opt/ros/humble/include/rcl_logging_interface -I/opt/ros/humble/include/rcl_yaml_param_parser -I/opt/ros/humble/include/libyaml_vendor -I/opt/ros/humble/include/tracetools -I/opt/ros/humble/include/rcpputils -I/opt/ros/humble/include/statistics_msgs -I/opt/ros/humble/include/rosgraph_msgs -I/opt/ros/humble/include/rosidl_typesupport_cpp -I/opt/ros/humble/include/rosidl_typesupport_c -I/opt/ros/humble/include/class_loader -I/opt/ros/humble/include/composition_interfaces -I/home/sig/autoware/install/autoware_common_msgs/include/autoware_common_msgs -I/opt/ros/humble/include/diagnostic_msgs -I/opt/ros/humble/include/shape_msgs -I/opt/ros/humble/include/action_msgs -I/opt/ros/humble/include/rclcpp_action -I/opt/ros/humble/include/rcl_action -I/opt/ros/humble/include/tf2_msgs -I/opt/ros/humble/include/tf2_ros -I/usr/include/vtk-9.1 -I/usr/include/jsoncpp -I/usr/include/freetype2 -I/usr/include/x86_64-linux-gnu/qt5 -I/usr/include/x86_64-linux-gnu/qt5/QtOpenGL -I/usr/include/x86_64-linux-gnu/qt5/QtWidgets -I/usr/include/x86_64-linux-gnu/qt5/QtGui -I/usr/include/x86_64-linux-gnu/qt5/QtCore -I/usr/lib/x86_64-linux-gnu/qt5/mkspecs/linux-g++ -isystem /usr/include/eigen3 -isystem /opt/ros/humble/include -isystem /opt/ros/humble/src/gmock_vendor/include -isystem /opt/ros/humble/src/gtest_vendor/include -isystem /home/sig/autoware/install/tier4_map_msgs/include/tier4_map_msgs -isystem /home/sig/autoware/install/autoware_map_msgs/include/autoware_map_msgs -isystem /opt/ros/humble/include/geometry_msgs -isystem /opt/ros/humble/include/rclcpp -isystem /opt/ros/humble/include/rclcpp_components -isystem /opt/ros/humble/include/visualization_msgs -isystem /home/sig/autoware/install/geography_utils/include -isystem /home/sig/autoware/install/component_interface_specs/include -isystem /home/sig/autoware/install/tier4_vehicle_msgs/include/tier4_vehicle_msgs -isystem /home/sig/autoware/install/component_interface_utils/include -isystem /home/sig/autoware/install/autoware_lanelet2_extension/include -isystem /home/sig/autoware/install/tier4_system_msgs/include/tier4_system_msgs -isystem /home/sig/autoware/install/tier4_planning_msgs/include/tier4_planning_msgs -isystem /home/sig/autoware/install/tier4_localization_msgs/include/tier4_localization_msgs -isystem /home/sig/autoware/install/tier4_control_msgs/include/tier4_control_msgs -isystem /home/sig/autoware/install/autoware_vehicle_msgs/include/autoware_vehicle_msgs -isystem /home/sig/autoware/install/autoware_utils/include -isystem /home/sig/autoware/install/autoware_planning_msgs/include/autoware_planning_msgs -isystem /home/sig/autoware/install/autoware_perception_msgs/include/autoware_perception_msgs -isystem /home/sig/autoware/install/autoware_adapi_v1_msgs/include/autoware_adapi_v1_msgs -isystem /opt/ros/humble/include/builtin_interfaces -isystem /opt/ros/humble/include/unique_identifier_msgs -isystem /opt/ros/humble/include/tf2 -isystem /opt/ros/humble/include/tf2_geometry_msgs -isystem /opt/ros/humble/include/nav_msgs -isystem /opt/ros/humble/include/rcl -isystem /opt/ros/humble/include/rosidl_runtime_cpp -isystem /opt/ros/humble/include/geographic_msgs -isystem /opt/ros/humble/include/message_filters -isystem /opt/ros/humble/include/pcl_msgs -isystem /opt/ros/humble/include/sensor_msgs -isystem /opt/ros/humble/include/std_msgs -isystem /usr/include/pcl-1.12 -isystem /usr/include/ni -isystem /usr/include/openni2 -fdiagnostics-color -O2 -g -DNDEBUG -Wall -Wextra -Wpedantic -Werror -fopenmp -fPIC -std=c++17 -o CMakeFiles/test_pointcloud_map_loader_module.dir/test/test_pointcloud_map_loader_module.cpp.o -c /home/sig/autoware/src/universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp",
  "file": "/home/sig/autoware/src/universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp"
}
]

This is my colcon build command:

colcon build --symlink-install --event-handlers=console_cohesion+ --cmake-args -DCMAKE_EXPORT_COMPILE_COMMANDS=1 -DCMAKE_BUILD_TYPE=RelWithDebInfo -DBUILD_TESTING=True -DCMAKE_C_FLAGS='-fdiagnostics-color' -DCMAKE_CXX_FLAGS='-fdiagnostics-color -Wno-enum-constexpr-conversion' -DCMAKE_C_COMPILER_LAUNCHER=ccache -DCMAKE_CXX_COMPILER_LAUNCHER=ccache -GNinja --profiling-format=google-trace --profiling-output=cmake_profile.json

Feel free to ping me on discord

[mojomex]

What! there are compile_commands for every package in addition to the overall one!?
@veqcc so we would have had to replace -I/usr and -I/opt there as well....

[VRichardJP]

@mojomex Yes and no.
Each package is build individually, thus generates its own compile_commands.json. But then colcon concatenates all packages compile_commands.json into a single one: build/compile_commands.json (see colcon/colcon-cmake#69)

I just modified and used build/map_loader/compile_commands.json because the original file is smaller. You will find the same content in build/compile_commands.json

[VRichardJP]

@xmfcx
I just realized I was building autoware using an old docker image (base image name has changed recently)
I have re-created my docker dev image, installed llvm 19 tools (./llvm.sh 19 all) and libomp-19-dev (to fix omp.h header not found). Then I built Autoware with the same command than above and got exactly the same output from clang-tidy-19.

[veqcc]

@xmfcx @mitsudome-r
Hi, it's been a long time since we have struggled to enable clang-tidy a required CI.

After that, I made some investigations on how to solve the problems we have encoutnered, and found some solutions

• Some of the errors/warnings to /opt and /usr are actually caused by Autoware, so fixing Autoawre leads to solving those system layer warnings.
• The differences between CI and local runs are due to missing headers, so it could be solved by adding --extra-arg=-I/usr/include/c++/11 or --extra-arg=-I/usr/include/x86_64-linux-gnu/c++/11 in local runs.
• Erros in opencv were solved by changing how to include opencv in the following PRs
  • chore(autoware_behavior_velocity_intersection_module): include opencv as system autoware.universe#9330
  • chore(autoware_traffic_light_visualization): include opencv as system autoware.universe#9331

Although not all the problems are solved yet, it is also true that several Autowarwe packages already pass clang-tidy checks.
Thus, I want to make a progress in the following steps:

1. Create a minimum .clang-tidy-ci file in which only CRITICAL warnings are enabled
2. Create clang-tidy-required-package.yaml file which specifies the packages to be checked by clang-tidy. This is proposed in feat(build-and-test-differential): modify clang-tidy to run only on specified packages autoware.universe#9016 by @yhisaki
3. Enable clang-tidy CI as required for the packages wtitten in clang-tidy-required-package.yaml
4. Aims to enable all the packages, by adding packages to the file one by one (First Step in the figure)
5. Add a new warning to .clang-tidy-ci which we want to make required. (Second Step in the figure)

The whole picture of this proposal look like the following:

If you have any comments or suggestions on this proposal, please provide your feedback by 25th of November (next Monday).