Suppress cppcheck unknownMacro

[rotu]

cppcheck creates an unknownMacro error when it cannot resolve a macro.
Since we don't pass in all dependent headers, we don't expect all macros to be discoverable by cppcheck.

[rotu]

I think this will fully fix ros2/ros2#942 and obviates the need for ros2/rosidl_typesupport_connext#57, ros2/rosidl_typesupport_fastrtps#49, ros2/rosidl#505, ros2/rosidl_typesupport#78

[ivanpauno]

LGTM

[ivanpauno]

@dirk-thomas @jacobperron What do you think about using this approach instead of the one suggested in ros2/rclpy#577?

[dirk-thomas]

I don't think this is a good approach since it removes a valid check and therefore reduces "coverage" of the linter. Not having the headers available which contain the macros also indicates that the linter can't do a complete / correct check.

[rotu]

I don't think this is a good approach since it removes a valid check and therefore reduces "coverage" of the linter. Not having the headers available which contain the macros also indicates that the linter can't do a complete / correct check.

That's also true of excluding headers outside the project, which we do for performance reasons, which cripples the linter's ability to check for mistakes even more.

I do think we should be adding ROS headers to the include path, but that the performance issues we're encountering with cppcheck are due to other misconfiguration of cppcheck, in particular not passing preprocessor definitions (since cppcheck checks every preprocessor branch when none is specified).

[ivanpauno]

That's also true of excluding headers outside the project, which we do for performance reasons, which cripples the linter's ability to check for mistakes even more.

I think that's true.

I do think we should be adding ROS headers to the include path, but that the performance issues we're encountering with cppcheck are due to other misconfiguration of cppcheck, in particular not passing preprocessor definitions (since cppcheck checks every preprocessor branch when none is specified).

Not passing definitions might be a source of bad performance in cppcheck, though I'm not completely sure if that's the problem here. Maybe we should revisit the decision of not adding the includes of dependencies by default ...

I don't think this is a good approach since it removes a valid check and therefore reduces "coverage" of the linter. Not having the headers available which contain the macros also indicates that the linter can't do a complete / correct check.

That's similar to the coverage we were having with cppcheck < 2.0, as this warning wasn't shown in those versions (it was shown sometimes, but a lot less frequently than with cppcheck >=2.0).

[jacobperron]

Maybe we should revisit the decision of not adding the includes of dependencies by default

FWIW, I tried to add headers of all dependencies back when we introduced passing headers to cppcheck in #117. The problem I ran into at the time was performance; it could take upwards of 300 seconds for cppcheck to run for some packages. It might be worth trying again to see if cppcheck performance has improved 🤷‍♂️

I agree that it would be preferable to not have to suppress another check for ROS core packages.
For projects that want to suppress the check they can always do so in a config file.

[rotu]

Maybe we should revisit the decision of not adding the includes of dependencies by default

FWIW, I tried to add headers of all dependencies back when we introduced passing headers to cppcheck in #117. The problem I ran into at the time was performance; it could take upwards of 300 seconds for cppcheck to run for some packages. It might be worth trying again to see if cppcheck performance has improved 🤷‍♂️

Why did it take so long for some packages? If cppcheck just has unavoidably bad performance, we should be looking for alternatives.

I suspect our root performance issue is the missing compile definitions, which I did my best to write up in #270

I agree that it would be preferable to not have to suppress another check for ROS core packages.

That argument cuts both ways. On one hand, you could be talking about suppressing unknownMacro as per this PR. on the other hand you could be talking about including headers of individual dependencies to suppress each instance of unknownMacro as per ros2/ros2#942.

But we do tolerate missing functions and missing classes, so tolerating missing macros doesn't seem like throwing out a lot of bathwater with this particular baby.

For projects that want to suppress the check they can always do so in a config file.

You should be able to consume upstream packages' interfaces without pulling in linter false positives. A lint error this fragile and confusing has no business being enabled by default.

I think we can re-enable unknownMacro in the future. But not while it's so easily tripped by benign code.

[ivanpauno]

But we do tolerate missing functions and missing classes, so tolerating missing macros doesn't seem like throwing out a lot of bathwater with this particular baby.

I second that.
As cppcheck doesn't have the include files of the dependencies, it cannot check anything meaningful when you call some_method_in_another_package(arg1, arg2, ..., argN).
The same applies to macros, but the only difference is that cppcheck is showing an "unknown macro" warning in that case.

So, I think we should either figure out why it takes so long, or ignore the unknown macro warning.

[norro]

What is the status of this PR?
I still think it's a good idea!

[jacobperron]

IMO, it's worth pursuing a solution that can automatically collect dependency headers and set the correct preprocessor values to cppcheck (as described in #270).

As a workaround, I think individual projects can suppress the unknownMacro error either by passing preprocessor values (directly to cppcheck or by including headers via ament_cmake_cppcheck_ADDITIONAL_INCLUDE_DIRS) or passing an explicit suppression flag. If configuring cppcheck via a separate cfg file is considered too cumbersome, we could add options to ament_cmake_cppcheck for convenience (e.g. on option to opt-in to suppressing the unknownMacro error).

[cottsay]

Newer versions of cppcheck (such as cppcheck-2.3, which is the current version in Fedora and EPEL) are triggering unknownMacro exceptions far more often than cppcheck-1.90, which is the current version in Ubuntu Focal.

Until we can pursue a more programmatic solution like @jacobperron is describing, my vote is to suppress.

[mm318]

Can cppcheck take in a compile_commands.json to help it locate all dependent headers?

I think all C++ linters under ament_lint should be provided a compile_commands.json and used this way. (For example, it is absolutely necessary to do so for ament_clang_tidy. When all linters are provided with a compile_commands.json, it would make the infrastructure consistent.)

[jacobperron]

Can cppcheck take in a compile_commands.json to help it locate all dependent headers?

This is kind of what is described in #270. I think an approach like this could work, it just needs someone to spend some time on it. @rotu Started some work towards this in #269.

[jacobperron]

With three approvals, I'll move forward with merging this change. If a patch lands for #270, we can try reverting this change.

[jacobperron]

Linter tests only:

• Linux
• Linux-aarch64
• macOS
• Windows

Edit: both failures are due to hitting a timeout of 180 seconds, which was bumped on master in #271.

[ivanpauno]

This is kind of what is described in #270. I think an approach like this could work, it just needs someone to spend some time on it. @rotu Started some work towards this in #269.

Yes, the only blocker was the increased test time (which was kind of unacceptable).
If we pass macro definitions to cppcheck the amount of combinations it tries will be reduced, that will probably fix the issue.