Can a constructor affect other fields of an enclosing object, or is this a static analysis false positive?

Question

Consider this C++ code:

struct SomeStruct {
  SomeStruct() noexcept;
};

//SomeStruct::SomeStruct() noexcept {}

class SomeClass {
  const bool b;
  const SomeStruct s;

public:
  SomeClass() : b(true) {}
  operator bool() const { return b; }
};

void f() {
  int *p = new int;
  if (SomeClass())
    delete p;
}

When I run clang --analyze -Xanalyzer -analyzer-output=text on it, I get this:

q72007867.cpp:20:1: warning: Potential leak of memory pointed to by 'p' [cplusplus.NewDeleteLeaks]
}
^
q72007867.cpp:17:12: note: Memory is allocated
  int *p = new int;
           ^~~~~~~
q72007867.cpp:18:7: note: Assuming the condition is false
  if (SomeClass())
      ^~~~~~~~~~~
q72007867.cpp:18:3: note: Taking false branch
  if (SomeClass())
  ^
q72007867.cpp:20:1: note: Potential leak of memory pointed to by 'p'
}
^
1 warning generated.

Uncommenting the definition of SomeStruct's constructor makes the warning go away, though. Swapping the order of const bool b; and const SomeStruct s; also makes it go away. In the original program, is there actually some other definition of SomeStruct's constructor that would lead to the false branch being taken there, or is this a false positive in Clang's static analyzer?

This is a valid question; just noting that even if it is a false positive, the warning could be useful by pointing out `f` isn't following best practices for exception safety. Any of several minor changes could introduce a more concrete danger of leak. — aschepler, Apr 26 '22 at 03:08
As the code stands it's a false positive -- clearly the analyzer logic doesn't extend to determining that the condition always evaluates to `true` — M.M, Apr 26 '22 at 03:26
The warning goes away [if you make the constructors and boolean conversion operator `constexpr`](https://gcc.godbolt.org/z/1EMqqb8Eq). Without `constexpr`, the compiler is not required to do constant propagation at compile time. — Raymond Chen, Apr 30 '22 at 16:13
@RaymondChen No it doesn't. The warning is only went away in your godbolt link because you changed the `;` after it to `{}`. Undoing that brings the warning back, and the warning stays gone if you leave that change but get rid of the `constexpr`s. — Joseph Sible-Reinstate Monica, May 02 '22 at 13:55
@JosephSible-ReinstateMonica Naturally the compiler cannot do compile-time evaluation of a constructor it cannot see. — Raymond Chen, May 02 '22 at 16:09
@RaymondChen: Even with your code, compiler is not required to do constant propagation... you use nothing in constant expression/context. — Jarod42, May 03 '22 at 11:36
@Jarod42 True. But this is about static analysis, and making everything visible helps the static analyzer. — Raymond Chen, May 03 '22 at 13:42

Yakk - Adam Nevraumont · Accepted Answer · 2022-04-30T15:20:22.533

There is no standard compliant way for a const member to be changed after initialization; any mechanism is going to be UB.

Like

struct foo{
  const bool b=true;
  foo(){ b=false; }
};

is illegal, as is code that const_casts b to edit it like:

struct foo{
  const bool b=true;
  foo(){ const_cast<bool&>(b)=false; }
};

(this second version compiles, but produces UB).

Such UB is sadly not that rare. For example, I could implement the constructor of SomeStruct to fiddle with memory before the this pointer address. It would be doubly illegal (modifying a const value after construction, and violating reachability rules), but depending on optimization settings it could work.

On the other hand, the compiler is free to notice that the only constructor assigns true to b and then convert operator bool to just return true.

But instead, the static code analyzer gives up on provong the state of b once a call to a function body outside of the visible source code occurs. This is a pretty reasonable thing to give up on. Here, the function even gets a pointer into the same temporary object; doing a full proof that said pointer cannot change some state regardless of what code is run is possible, but failing to do that seems also reasonable.

Style wise, the code is also a bit of a mess. A provably true branch either should not exist, or the failure branch should semantically make sense. Neither occurs here; anyone reading this code cannot determine correctness from code structure; the code structure looks misleading.

I thought the code snippet you post doesn't compile. Or do you mean a more complex way to fiddle with `this->somewhereToBool`? [Demo](https://godbolt.org/z/K3neMqj7s) — Louis Go, Apr 26 '22 at 06:04
@LouisGo Yes, it is an invalid program. I mentioned using `const_cast` t make it compile, which also isn't legal to do in C++ (results in undefined behavior) like https://godbolt.org/z/fso3qWqzq -- the point is any way you try to change the bool isn't valid. (Manually destroying and recreating the object "changes the bool" but not in any intuitive sense). — Yakk - Adam Nevraumont, Apr 26 '22 at 13:20
The code style here is only a mess as a result of minimizing the example. The original code I found this in actually made sense to write. — Joseph Sible-Reinstate Monica, Apr 29 '22 at 17:19

Can a constructor affect other fields of an enclosing object, or is this a static analysis false positive?

1 Answers1