github / codeql-coding-standards

This repository contains CodeQL queries and libraries which support various Coding Standards.
MIT License
122 stars 54 forks source link

`M0-1-10`: Generates lot of false positives for used functions #711

Open rak3-sh opened 3 hours ago

rak3-sh commented 3 hours ago

Affected rules

Description

The query M0-1-10 generates a significant amount of false positives while analyzing "unused functions". These are noticed especially in the following scenarios.

This results from the fact that CodeQL cannot track the usages of such functions in compile time constructs like constexpr, static_asserts, templates etc.

Example

test.hpp

template <class T>
constexpr T to_underlying(T value) noexcept { // M0-1-3 violation reported here
  return static_cast<T>(value);
}

test.cpp

#include <type_traits>
#include "test.hpp"

template <typename T1, typename T2>
constexpr bool StaticAssertTypeEq() noexcept {
  static_assert(std::is_same<T1, T2>::value, "T1 and T2 are not the same type");
  return true;
}

template <typename T, int val>
class X
{
  T arr[val];
};

void foo()
{
  struct dummy {
    dummy() noexcept(false) { static_cast<void>(0); } // M0-1-3 violation reported here
  };
  // usage of dummy
  static_assert(!std::is_nothrow_default_constructible<X<dummy, 5>>::value,
                "Must not be nothrow default constructible");
}

int main()
{
  int a;
  StaticAssertTypeEq<decltype(to_underlying(a)), int>(); // usage of to_underlying
  foo();
  return 0;
}
rak3-sh commented 3 hours ago

Proposed Fix(es)

Hi @lcartey, kindly let me know your valuable insights whether the following can be acceptable or not.

The MISRA2008 standard doesn't highlight any specific exception for this rule. However, considering CodeQL's limitation and to make it more practical to check the results of this query, can we propose to restrict this query in the following way? I would like to seek opinions about the same.

lcartey commented 2 hours ago

Hi Rakesh. Thanks again for reporting this. To take each suggestion in turn:

Avoid analyzing functions that are constexpr because CodeQL has known limitations about tracking the usages of constexpr functions/expressions.

Yes, I think that would be reasonable.

Avoid analyzing explicitly defaulted functions. Since the user has marked it as "default", should we consider these as candidates for unused functions?

default was added to the language after MISRA C++ 2008, so wasn't considered when the rule was written. While you could argue that a default function that is never called is unused in the strict sense of the word, I think

Avoid analyzing functions which result from template instantiations. Since the function has been instantiated, there should be an explicit call.

Functions from template instantiations should already be excluded (but contribute to whether the uninstantiated version of the function is considered used or not). Do you have a specific example where they are not?

Avoid analyzing special member functions (constructors, destructors, operators, conversion operators). This is proposed mainly because their usages in compile time constructs are not tracked properly by CodeQL.

I think we can exclude special member functions which are marked constexpr - because those I would expect we may not be able to report accurately, due to the automatic folding of constants. In other cases, I would expect to have enough information in the database to judge whether the function is or isn't used.

I note that the equivalent rule in MISRA C++ 2023 (Rule 0.2.4) does exclude special member functions, so another option would be to split the rule into two queries (one for special member functions, one without) so as to enable deviations to be applied to this case.

Treating test stubs from GoogleTest framework as a likely entry point. E.g. a function named "TestBody" can be treated as a GoogleTest function and considered as an entry point. We can think of making it more specific by checking GoogleTest's stub function's characteristics

Yes, the query already has a concept of an EntryPoint abstract class that can be extended to support other types of entry points.