-Wpointer-bool-conversion/-Waddress misses an obvious case

[r-barnes]

The following pattern is often used to evaluate an expensive function only once, cache its result, and return that on subsequent calls.

int foo() {
    static bool is_true = [](){ return expensive_helper_function; };

    return is_true;
}

But this code is subtly wrong. It should be

static bool is_true = [](){ return expensive_helper_function; }(); // Note the parens!

In the bad case it looks as though the lambda is being evaluated to a pointer(?) and this is being evaluated always to true when it is assigned to is_true.

It feels as though -Wpointer-bool-conversion should flag this, but it does not (godbolt).

[r-barnes]

This reproduces:

bool isSupported() {
  static bool is_supported = []() {
    return true;
  };
  return is_supported;
}

int main() {
    return isSupported();
}

• LLVM 17 - no warning (godbolt)
• GCC 11.1 - first GCC version to warn about this on -Waddress (godbolt)

[MatzeB]

I think I filed/proposed this in an issue years ago #25841 ...

[MatzeB]

FWIW this seems to be covered by clang-tidy https://clang.llvm.org/extra/clang-tidy/checks/readability/implicit-bool-conversion.html but unfortunately this rule also applies to other forms of conversions like int -> bool which may make it less suitable to enable in all situations...

[shafik]

So we end up here:

llvm-project/clang/lib/Sema/SemaChecking.cpp

Lines 15628 to 15632 in 004c197

	if (Source->isPointerType() || Source->canDecayToPointerType()) {
	// Warn on pointer to bool conversion that is always true.
	S.DiagnoseAlwaysNonNullPointer(E, Expr::NPCK_NotNull, /*IsEqual*/ false,
	SourceRange(CC));
	}

and then we bail out here:

llvm-project/clang/lib/Sema/SemaChecking.cpp

Lines 16463 to 16465 in 004c197

	// Weak Decls can be null.
	if (!D || D->isWeak())
	return;

I think what we need is a extra check in Sema::DiagnoseAlwaysNonNullPointer(...)

check for CXXMemberCallExpr and then if getObjectType() gives us a RecordType then check isLambda() and diagnose.

@AaronBallman seems like a good first issue.

[AaronBallman]

This does seem like a reasonable first issue and those steps are what I'd recommend as well. I don't think there are any surprises lurking in here that would trip someone up.

[llvmbot]

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

1. In the comments of the issue, request for it to be assigned to you.
2. Fix the issue locally.
3. Run the test suite locally. Remember that the subdirectories under test/ create fine-grained testing targets, so you can e.g. use make check-clang-ast to only run Clang's AST tests.
4. Create a Git commit.
5. Run git clang-format HEAD~1 to format your changes.
6. Open a pull request to the upstream repository on GitHub. Detailed instructions can be found in GitHub's documentation.

If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.

[llvmbot]

@llvm/issue-subscribers-good-first-issue

Author: Richard Barnes (r-barnes)

The following pattern is often used to evaluate an expensive function only once, cache its result, and return that on subsequent calls. ``` int foo() { static bool is_true = [](){ return expensive_helper_function; };

return is_true;

}

But this code is subtly wrong. It should be

static bool is_true = { return expensive_helper_function; }(); // Note the parens!

In the bad case it looks as though the lambda is being evaluated to a pointer(?) and this is being evaluated always to `true` when it is assigned to `is_true`.

It feels as though `-Wpointer-bool-conversion` should flag this, but it does not ([godbolt](https://godbolt.org/z/v9qhze9e3)).
</details>

[vinayakdsci]

@AaronBallman Hello! I would love to work on this issue. Could it be assigned to me?

[EugeneZelenko]

@vinayakdsci: Just create pull request and mention it on this page.

[vinayakdsci]

@AaronBallman I was trying to follow the instructions provided by Shafik above, and I seem to be getting an assertion failure DiagnosticsEngine::ArgumentKind clang::Diagnostic::getArgKind(unsigned int) const: Assertion Idx < getNumArgs() && "Argument index out of range!"' failed.

This is triggered when I try to emit a diagnostic with DiagID diag::warn_impcast_pointer_to_bool.

Do you have any ideas on how to fix this? I am unable to get it to work, and have tried a number of ways to avoid this assertion failure.

This diagnostic I was able to locate in the diagnostic tablegen file DiagnosticSemaKinds.td.

Thanks!

[dwblaikie]

you might not be passing the right number of/enough parameters to the diagnostic? In the .td file you'll see the diagnostic text uses a number of placeholders (%0, etc I think) and you'll need to supply just as many to the diagnostic - mabye check another use of that diagnostic to see what/how the parameters are passed?

[vinayakdsci]

@dwblaikie thanks for the advice, I was able to fix the errors. However, after the fixes, some of the existing tests are failing.
One is in clang/test/CXX/drs/dr18xx.cpp where a lambda expression is assigned to bool and three in CXX/expr/expr.prim/expr.prim.lamda/blocks.mm where a struct inherits from a lambda function. While I think that the former can fixed with a warning, I am not sure if it would be alright to change any tests in expr.prim.lambda/?

How do you think I should proceed? Should I open a PR and wait for review, and then fix any errors?

Thanks!

[shafik]

I think the best way forward is to open a PR and then folks can see the code changes and verify they match the intent and we can see the CI failures for ourselves and that should help identify better unexpected outcomes.