[clang-tidy] Check request: misc-constexpr-non-static-in-scope

[denzor200]

Many developers mistakenly believe that constexpr variables inside functions automatically have a static lifetime, like static variables. This common misconception leads to dangerous code that:

• Compiles without errors, but causes undefined behavior (UB) at runtime.
• Hard to detect in code reviews and testing.
• Misleading because constexpr is associated with "compile-time safety".

Need a check that will find constexpr non-static variables inside a function scope. The check will provide fixit hint to add static keyword.

BEFORE:

void foo() {
   constexpr int x = 42;
   // ...
}

AFTER:

void foo() {
   static constexpr int x = 42;
   // ...
}

The check will not provide warning for constexpr variables outside a function scope:

namespace ns {
inline constexpr int MAX_SIZE = 1024;  // OK - no need static here
}

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: Denis Mikhailov (denzor200)

Many developers mistakenly believe that `constexpr` variables inside functions **automatically have a static lifetime**, like `static` variables. This common misconception leads to dangerous code that: - Compiles without errors, but causes undefined behavior (UB) at runtime. - Hard to detect in code reviews and testing. - Misleading because constexpr is associated with "compile-time safety".

Need a check that will find constexpr non-static variables inside a function scope. The check will provide fixit hint to add static keyword.

BEFORE:

void foo() {
   constexpr int x = 42;
   // ...
}

AFTER:

void foo() {
   static constexpr int x = 42;
   // ...
}

The check will not provide warning for variables outside a function scope:

namespace ns {
inline constexpr int MAX_SIZE = 1024;  // OK - no need static here
}

[denzor200]

Sample of problematic code here:

int* p = nullptr;
{
   constexpr int x = 100;
   p = &x;
}
use(*p); // UB - dangling pointer `p`

[Spaarsh]

I would like to work on this issue.

I plan to add a new .cpp and .h file to the clang-tools-extra/clang-tidy/misc/ directory. I think all that is needed by the checker is to look if the variable declaration is happening inside a function, is using constexpr and is not using static.

Please do tell me if there are any inaccuracies to my solution. Otherwise I plan to make a PR soon.

Thanks!

[vbvictor]

You should use script ./clang-tools-extra/clang-tidy/add_new_check.py to automatically set up all necessary files. Also, please read https://clang.llvm.org/extra/clang-tidy/Contributing.html.

I think it should be in bugprone category.

[vbvictor]

I hear for the first time that people think constexpr variables inside functions automatically have a static lifetime. I'd like to warn only when we have

constexpr int x = 100;
p = &x; // UnaryOperator(has(DeclRefExpr(to(varDecl(isConstexpr())))))

So we would only tell people: "you may think that constexpr variables are static but they are not!" (wording is informal)

Also, note that static inside constexpr functions is C++23 extension. https://godbolt.org/z/jzcGnKna6. I'd assume that people mostly like to have constexpr variables inside constexpr functions.

[carlosgalvezp]

I've only heard about this pattern in terms of performance actually, there's a Jason Turner episode about it.

Returning references to local objects is an anti-pattern, which could be hidden by this check.

Thus I'd be inclined to have this under performance-use-static-constexpr instead, but I don't have a strong opinion.

I didn't know about it being a C++23 extension as mentioned above, that's something people will probably care about.

[Spaarsh]

So I suppose I should write the check such that it warns a user trying to declare constexpr in a function with the same return datatype or warn the user if they try to reference constexpr without static?

And I also keep this version aware. So a static constexpr inside a function will raise a warning for versions older than C++23.

Or should I wait until this is more thoroughly discussed?

[vbvictor]

I didn't know about it being a C++23 extension as mentioned above, that's something people will probably care about.

Paper behind it https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2647r1.html

So a static constexpr inside a function will raise a warning for versions older than C++23.

We probably don't need a warning for that in clang-tidy because it's already a on-by-default compiler-warning -Wc++23-extensions. But we shouldn't advise users to write static in a constexpr function prior to C++23.

[carlosgalvezp]

Paper behind it

Thank you! I think this paragraph highlights what I mentioned before and could motivate putting this in performance:

We could make the variable non-static, but compilers have difficulty optimizing this, leading to much worse code-gen

So to summarize:

• Warn in non-constexpr functions for all standards.
• Warn also in constexpr functions in C++23 and onwards (possibly behind an option in case people want to use the extensions regardless).

Another aspect is that this is probably only useful when dealing with big objects, typically arrays. Warning on small objects like ints or floats might be too noisy in relation to the performance gains (if any). It could be behind an option too.

[vbvictor]

Or should I wait until this is more thoroughly discussed?

With carlosgalvezp's summary, I think you could implement first version of the check. The only uncertainty may be the check category, we could begin with performance and later change it using rename_check.py script fairly easy if needed.

[vbvictor]

Another aspect is that this is probably only useful when dealing with big objects, typically arrays. Warning on small objects like ints or floats might be too noisy in relation to the performance gains (if any). It could be behind an option too.

For this you can create an option to specify minimal size of object to warn (like 16 or 32 chars), you could use ASTContext::getTypeSizeInChars(QualType T) function

Upd, maybe it's a bad criteria for types like std::vector..

[carlosgalvezp]

minimal size of object to warn

Even if it can be detected reliably, I find it brittle, because changing one header file can require a change in many places where is used.

Maybe just an option to warn about arrays, and another one where the user can specify which types to warn about.

[vbvictor]

Even if it can be detected reliably, I find it brittle, because changing one header file can require a change in many places where is used.

Maybe that's why it isn't widely used, so we should go with plain list of types.

[denzor200]

Another aspect is that this is probably only useful when dealing with big objects, typically arrays. Warning on small objects like ints or floats might be too noisy in relation to the performance gains (if any). It could be behind an option too.

Small objects like ints or floats also relevant for this check. They may affect performance too being non-static:

Upd: the full snippet: https://quick-bench.com/q/lSl3JShDGP77aqliaadn5ukNL8M

[denzor200]

Could someone add check request label please?