Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
rak3-sh opened this issue Oct 30, 2024 · 4 comments · May be fixed by #794
Open

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

rak3-sh opened this issue Oct 30, 2024 · 4 comments · May be fixed by #794
Labels
Difficulty-Medium A false positive or false negative report which is expected to take 1-5 days effort to address false positive/false negative An issue related to observed false positives or false negatives. Impact-Medium Standard-AUTOSAR

Comments

@rak3-sh
Copy link
Contributor

rak3-sh commented Oct 30, 2024

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 rak3-sh added the false positive/false negative An issue related to observed false positives or false negatives. label Oct 30, 2024
@rak3-sh
Copy link
Contributor Author

rak3-sh commented Oct 30, 2024

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

rak3-sh added a commit to rak3-sh/codeql-coding-standards that referenced this issue Nov 11, 2024
@lcartey
Copy link
Collaborator

lcartey commented Nov 15, 2024

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.

@lcartey lcartey moved this from Reported to Review in progress in Coding Standards Public Development Board Nov 15, 2024
@lcartey lcartey added Difficulty-Medium A false positive or false negative report which is expected to take 1-5 days effort to address Impact-Medium labels Nov 15, 2024
@rak3-sh
Copy link
Contributor Author

rak3-sh commented Nov 18, 2024

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

rak3-sh added a commit to rak3-sh/codeql-coding-standards that referenced this issue Nov 18, 2024
@rak3-sh
Copy link
Contributor Author

rak3-sh commented Nov 18, 2024

Kindly check the updated PR: #794

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Difficulty-Medium A false positive or false negative report which is expected to take 1-5 days effort to address false positive/false negative An issue related to observed false positives or false negatives. Impact-Medium Standard-AUTOSAR
Projects
Status: Review in progress
3 participants