A7-1-2: Query proposes to add constexpr to non-static data members (and other problems)

[rak3-sh]

Affected rules

• A7-1-2

Description

1. constexpr on non-static data members

This query should exclude variables that are non-static data members. This is because the compiler raises an error in case a non-static data member is marked as constexpr.

Example

class a_class
{
  int y;
  public:
  // CodeQL says to make 'x' as constexpr. 
  // But when we do so, there is a compiler error: "error: non-static data member ‘x’ declared ‘constexpr’"
  int x = 10; 
  a_class():y(x)
  {
  }
  int getY()
  {
    return y;
  } 
};

int main()
{
  a_class o;
  return o.getY();
}

Fixing as per the alerts raised by CodeQL in the test case of this query (e.g. here or here) also raises the error.

test.cpp:55:3: error: non-static data member ‘m2’ declared ‘constexpr’
   55 |   constexpr int m2 = 0; // NON_COMPLIANT

test.cpp:130:3: error: non-static data member ‘m1’ declared ‘constexpr’
  130 |   constexpr int m1 = 0;

2. constexpr on member variables updated in un-instantiated templates

This query alerts to add constexpr to member variables which are only used in uninstantiated template functions. At first glance, it seems to be correct because since the template function is not instantiated, the member variable isn't getting accessed anywhere and hence becomes a candidate for adding constexpr. However, when the user attempts to fix it, the compiler raises an error and points to the modification in the uninstantiated template function. So should we avoid member variables of templates?

Example

#include <vector>
#include <memory>

template <typename T>
class a_class
{
  // CodeQL says to make "size_" as constexpr because the "add()" function below isn't instantiated and hence, 
  // there is no access of "size_", but if we do this, the compiler complains that "size_" is updated in add.
  // "test.cpp:12:7: error: increment of read-only variable ‘a_class<T>::size_’"
  std::size_t size_ = 0;   
  std::vector<T> values_{};
  public:
  void add(T arg) {
    values_[size_] = arg;
    ++size_;
  }
};

int main()
{
  a_class<int> o;
  return 0;
}

Note: The error about size_ being a non-static data member and constexpr also exists but even if we make it static, we cant fix the error because compiler complains about the update on size_ in the uninstantiated template function add().

3. constexpr for range-based for loop variables

I'm not able to provide a reproducible case but this query alerts on variables used in range-based for loop.

Example

void a_function()
{
  auto origin = std::vector<int> {1, 2, 3, 4, 5, 6, 7, 8, 9};
  auto values = std::vector<std::unique_ptr<int>>{};
  for (auto& value : origin) { // value can be constexpr
    values.emplace_back(std::make_unique<int>(value));
  }
}

The reason is that when the range based for loop is de-sugared there is an assignment like the following:

for (..)
auto &value = *(__begin) // operator call to * with __begin variable generated by the compiler front end.
...

We have an operator* as an initializer expression and it has a variable access to the __begin variable. The line; “any(Call call | isCompileTimeEvaluatedCall(call))...” here is True for this case because the operator * is marked as constexpr by CodeQL and the getAnArgument() here for this call doesn’t return any arguments. Please note that, the qualifier for this call expression is the __begin variable which is compiler generated. So should we avoid such calls which has a qualifier that has VariableAccess to variables that are compiler generated?

[rak3-sh]

I've a fix for (1) and (3). Requesting @lcartey to provide valuable comments!

[lcartey]

For (1), I think we should report non-static members, but only if they are genuinely never modified. This is because the rule states:

The constexpr specifier shall be used for values that can be determined at compile time.

And the value can be determined at compile time if it's not genuinely modified. You are correct in saying that the user will also need to make the variable static in this case, in order to get the program to compile. We could potentially update the message and/or help file to explain this.

For (2), CodeQL doesn't generate template members unless it needs to, so we don't get visibility into that code unless add gets called. The change I think we should make here is not reporting member variables as constexpr candidates if there exists a member function for which we don't have a definition - as we don't have the information in these cases to make the decision on whether it can be constexpr or not.

For (3), I think we discussed this separately, but the for-range-loop generated variable is wrongly flagged because we don't check that qualifiers of calls are isCompileTimeEvaluatedExpressions, which leads us to the wrong conclusion. There's a separate issue that these variables aren't marked as compiler generated, which I have been unable to replicate in unit tests to date.

[rak3-sh]

Thanks @lcartey for the excellent feedback! I'll modify the PR as per your suggestions!

[rak3-sh]

Kindly check the updated PR: #794