clang-analyzer-cplusplus.Move false positive due to incompletely tracked data dependency

[bbannier]

Bugzilla Link	42433
Version	trunk
OS	All
CC	@devincoughlin,@haoNoQ

Extended Description

We are seeing a false positive from clang-analyzer-cplusplus.Move in our code which looks like it is due to incomplete tracking of data dependencies.

A somewhat simplified example:

// file test.cpp
#include <string>
#include <utility>

enum class Flow { BREAK, CONTINUE };

struct F {
  template <typename T> F(T, Flow f) : _flow(f) {}
  Flow _flow;
};

template <typename T> F break_(T &&t) {
  return F{std::forward<T>(t), Flow::BREAK};
}

F continue_() { return F{0, Flow::CONTINUE}; }

template <typename Body> void while_(Body &&body) {
  auto flow = body();

  switch (flow._flow) {
  case Flow::CONTINUE: {
    body();
    break;
  }
  case Flow::BREAK:
    return;
  }

  return while_(body);
}

int main() {
    std::string s("foo");

    while_([&]() -> F {
      if (!s.empty()) {
        return break_(std::move(s));
      } else {
        s = std::string();
        return continue_();
      }
    });
}

When analyzed this gives

$ clang-tidy -checks='-*,clang-analyzer-cplusplus.Move' test.cpp -- --std=c++11
1 warning generated.
/test.cpp:12:12: warning: Moved-from object 's' of type 'std::__cxx11::basic_string' is moved [clang-analyzer-cplusplus.Move]
  return F{std::forward<T>(t), Flow::BREAK};
           ^
/test.cpp:35:5: note: Calling 'while_<(lambda at /test.cpp:35:12)>'
    while_([&]() -> F {
    ^
/test.cpp:20:3: note: Control jumps to 'case CONTINUE:'  at line 21
  switch (flow._flow) {
  ^
/test.cpp:22:5: note: Calling 'operator()'
    body();
    ^
/test.cpp:36:11: note: Assuming the condition is true
      if (!s.empty()) {
          ^
/test.cpp:36:7: note: Taking true branch
      if (!s.empty()) {
      ^
/test.cpp:37:16: note: Calling 'break_<std::__cxx11::basic_string<char>>'
        return break_(std::move(s));
               ^
/test.cpp:12:12: note: Object 's' of type 'std::__cxx11::basic_string' is left in a valid but unspecified state after move
  return F{std::forward<T>(t), Flow::BREAK};
           ^
/test.cpp:37:16: note: Returning from 'break_<std::__cxx11::basic_string<char>>'
        return break_(std::move(s));
               ^
/test.cpp:22:5: note: Returning from 'operator()'
    body();
    ^
/test.cpp:23:5: note:  Execution continues on line 29
    break;
    ^
/test.cpp:29:10: note: Calling 'while_<(lambda at /test.cpp:35:12) &>'
  return while_(body);
         ^
/test.cpp:18:15: note: Calling 'operator()'
  auto flow = body();
              ^
/test.cpp:36:11: note: Assuming the condition is true
      if (!s.empty()) {
          ^
/test.cpp:36:7: note: Taking true branch
      if (!s.empty()) {
      ^
/test.cpp:37:16: note: Calling 'break_<std::__cxx11::basic_string<char>>'
        return break_(std::move(s));
               ^
//test.cpp:12:12: note: Moved-from object 's' of type 'std::__cxx11::basic_string' is moved
  return F{std::forward<T>(t), Flow::BREAK};
           ^

I am seeing this with clang version 44019e8c500fba0cece458d2e44700c967f247c9 (git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@364630 91177308-0d34-0410-b5e6-96231b3b80d8).

[haoNoQ]

Yeah, i see, we always break on the first iteration, nvm. We simply don't realize that a string constructed with a non-empty string literal is non-empty.

The while_ thingy is still suspicious though, i think you should double-check your actual implementation.

[haoNoQ]

Oh, wait, i see, the string is also cleared. Let me think a bit more.

[haoNoQ]

Hmm, are you sure this while_ construct works as expected? It looks as if the return value of body() in case Flow::CONTINUE: is entirely discarded. Therefore it's not surprising that when the loop yields "break" on its second iteration it keeps going on instead of actually breaking. Therefore it looks like a true positive on this reduced example.

[bbannier]

assigned to @haoNoQ