ament / ament_lint

Apache License 2.0
38 stars 107 forks source link

clang_tidy XML to SARIF conversion doesn't match the SARIF spec #418

Open Ronoman opened 2 years ago

Ronoman commented 2 years ago

I ran colcon test --packages-select rclcpp and examined the clang_tidy output. Below is the XML reported by static analysis, as well as the SARIF generated by ament_clang_tidy on the spaceros branch.

XML output:

<testcase
    name="/home/spaceros-user/src/spaceros/build/rclcpp/include/rclcpp/node_interfaces/get_node_base_interface.hpp:97:65"
    classname="rclcpp.clang_tidy">
      <failure message="non-const reference parameter 'node_interface', make it const or use a pointer [google-runtime-references]"><![CDATA[/home/spaceros-user/src/spaceros/build/rclcpp/include/rclcpp/node_interfaces/get_node_base_interface.hpp:97:65]]></failure>
</testcase>

SARIF output:

...
"rules": [
    {
        "id": "google-runtime-references",
        "shortDescription": {
            "text": "non-const reference parameter 'node_interface', make it const or use a pointer"
        },
        "helpUri": "https://clang.llvm.org/extra/clang-tidy/checks/list.html"
    },
...
"results": [
    {
        "ruleId": "google-runtime-references",
        "level": "warning",
        "kind": "review",
        "message": {
            "text": "  std::shared_ptr<rclcpp::node_interfaces::NodeBaseInterface> & node_interface)\n                                                                ^\n"
        },
        "locations": [
        {
            "physicalLocation": {
            "artifactLocation": {
                "uri": "/home/spaceros-user/src/spaceros/build/rclcpp/include/rclcpp/node_interfaces/get_node_base_interface.hpp",
                "index": 0
            },
            "region": {
                "startLine": "97",
                "startColumn": "65"
            }
            }
        }
        ]
    },
...

What isn't right:

  1. The rules.shortDescription.text references code-specific issues, instead of the general issue being reported. This ends up duplicating rules when converting XML to SARIF.
  2. results.message.text is not sufficient to describe the issue (related to 1 above).
  3. startLine and startColumn should be integers, not strings.
  4. results.message.text has extraneous characters that could throw off the SARIF dashboard. Not high priority, and may be difficult to intelligently strip from the result.
  5. google-runtime-references is deprecated, it was removed from Google's style guide in May 2020: https://www.mail-archive.com/cfe-commits@lists.llvm.org/msg203119.html