[clang-tidy] `bugprone-exception-escape` behaves weird in language versions before C++17

[ledror]

clang-tidy's bugprone-exception-escape check doesn't work as expected on windows builds.
Consider the following file:

class Foo {};

Foo func() {
    throw 1;
}

class Bar {
public:
    ~Bar();
};

Bar::~Bar() {
    func()
}

When running clang-tidy with bugprone-exception-escape, we get, as expected, the following report:

> clang-tidy -checks="bugprone-exception-escape" good.cpp
...
good.cpp:12:6: warning: an exception may be thrown in function '~Bar' which should not throw exceptions [bugprone-exception-escape]
   12 | Bar::~Bar() {

Now consider the following file:

class Foo {};

Foo func() {
    throw 1;
}

class Bar {
public:
    ~Bar();
};

Bar::~Bar() {
    Foo foo = func();
    (void)foo;
}

When running the same command on windows we don't get any diagnostic.

On the other hand, with a macOS build we do get the expected message:

> clang-tidy -checks="bugprone-exception-escape" bad.cpp
...
bad.cpp:12:6: warning: an exception may be thrown in function '~Bar' which should not throw exceptions [bugprone-exception-escape]
   12 | Bar::~Bar() {

A weird case is that if the return type of func is an integral type (e.g int) and not a class, we do get the diagnostic on windows as well:

> clang-tidy -checks="bugprone-exception-escape" bad.cpp
...
bad.cpp:10:6: warning: an exception may be thrown in function '~Bar' which should not throw exceptions [bugprone-exception-escape]
   10 | Bar::~Bar() {
      |      ^
bad.cpp:2:5: note: frame #0: unhandled exception of type 'int' may be thrown in function 'func' here
    2 |     throw 1;
      |     ^
bad.cpp:11:15: note: frame #1: function '~Bar' calls function 'func' here
   11 |     int foo = func();

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: ledror (ledror)

clang-tidy's `bugprone-exception-escape` check doesn't work as expected on windows builds. Consider the following file:

class Foo {};

Foo func() {
    throw 1;
}

class Bar {
public:
    ~Bar();
};

Bar::~Bar() {
    func()
}

When running clang-tidy with bugprone-exception-escape, we get, as expected, the following report:

> clang-tidy -checks="bugprone-exception-escape" good.cpp
...
good.cpp:12:6: warning: an exception may be thrown in function '~Bar' which should not throw exceptions [bugprone-exception-escape]
   12 | Bar::~Bar() {

Now consider the following file:

class Foo {};

Foo func() {
    throw 1;
}

class Bar {
public:
    ~Bar();
};

Bar::~Bar() {
    Foo foo = func();
    (void)foo;
}

When running the same command on windows we don't get any diagnostic.

On the other hand, with a macOS build we do get the expected message:

> clang-tidy -checks="bugprone-exception-escape" bad.cpp
...
bad.cpp:12:6: warning: an exception may be thrown in function '~Bar' which should not throw exceptions [bugprone-exception-escape]
   12 | Bar::~Bar() {

A weird case is that if the return type of func is an integral type (e.g int) and not a class, we do get the diagnostic on windows as well:

> clang-tidy -checks="bugprone-exception-escape" bad.cpp
...
bad.cpp:10:6: warning: an exception may be thrown in function '~Bar' which should not throw exceptions [bugprone-exception-escape]
   10 | Bar::~Bar() {
      |      ^
bad.cpp:2:5: note: frame #<!-- -->0: unhandled exception of type 'int' may be thrown in function 'func' here
    2 |     throw 1;
      |     ^
bad.cpp:11:15: note: frame #<!-- -->1: function '~Bar' calls function 'func' here
   11 |     int foo = func();

[vbvictor]

Hm, if you write

Bar::~Bar() {
    func();
}

It works fine on Windows too.

[ledror]

Hm, if you write

Bar::~Bar() {
func();
}
It works fine on Windows too.

Yea, that's the first example I gave

[localspook]

Godbolt repro: https://godbolt.org/z/56xr56P3G
Neither -fdelayed-template-parsing nor -fms-extensions (the usual suspects for divergent behaviour on Windows) seems to be the culprit here

[vbvictor]

We should probably check if there are any differences in AST on Win vs Linux. But still idk how can be there major differences.

[localspook]

Ahhh, it's a problem with the language mode: https://godbolt.org/z/ajnqjsTEq. Looks like clang defaults to a different language version when compiling for Windows (probably to match MSVC), which changes the AST, and the check mishandles the AST before C++17 for some reason.

[localspook]

Looks like this is a symptom of a bigger issue: when the check sees a CallExpr or a CXXConstructExpr node, it just... ignores all of its children! Here are some ways this manifests (Godbolt):

int thrower() { throw 1; }
void f(int) {}
struct S { S(auto&&...) {}; };

// Zero warnings in this function.
void g() noexcept {
  f(thrower());
  f(false ? 1 : throw 1);
  (false ? []{} : throw 1)(); // Contrived example, I admit
  S(false ? 1 : throw 1);
}

How does this apply to the example in this issue? Well, before C++17, this line:

Foo foo = func();

Generates this AST:

DeclStmt <line:13:5, col:21>
`-VarDecl <col:5, col:20> col:9 used foo 'Foo' cinit
  `-ExprWithCleanups <col:15, col:20> 'Foo'
    // The check doesn't see anything below this CXXConstructExpr.
    `-CXXConstructExpr <col:15, col:20> 'Foo' 'void (Foo &&) noexcept' elidable
      `-MaterializeTemporaryExpr <col:15, col:20> 'Foo' xvalue
        `-CallExpr <col:15, col:20> 'Foo' // Throw happens here!
          `-ImplicitCastExpr <col:15> 'Foo (*)()' <FunctionToPointerDecay>
            `-DeclRefExpr <col:15> 'Foo ()' lvalue Function 0xf3cdb58 'func' 'Foo ()'

The AST changes in C++17 (because of deferred temporary materialization, I think?) to this:

DeclStmt <line:13:5, col:21>
`-VarDecl <col:5, col:20> col:9 used foo 'Foo' cinit
  `-CallExpr <col:15, col:20> 'Foo' // Throw happens here.
    // The check still ignores all the following nodes.
    `-ImplicitCastExpr <col:15> 'Foo (*)()' <FunctionToPointerDecay>
      `-DeclRefExpr <col:15> 'Foo ()' lvalue Function 0x112cbe38 'func' 'Foo ()'

There's no longer an intervening CXXConstructExpr, so the check happens to work.