[PATCH] D138777: [clang-tidy] Add check bugprone-multiple-new-in-one-expression.

[Balázs Kéri via Phabricator via cfe-commits]

balazske marked an inline comment as done.
balazske added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/MultipleNewInOneExpressionCheck.cpp:91
+                                      ExceptionType, ExceptionReferenceType)));
+  auto BadAllocCatchingTryBlock = cxxTryStmt(hasHandlerFor(CatchBadAllocType));
+
----------------
PiotrZSL wrote:
> what about: ```catch(...)```
`hasHandlerFor` checks for it (and it is used in the test).


================
Comment at: clang-tools-extra/clang-tidy/bugprone/MultipleNewInOneExpressionCheck.cpp:102
+          hasAnyArgument(
+              expr(HasNewExpr1, unless(equalsBoundNode("arg2"))).bind("arg1")),
+          hasAnyArgument(
----------------
PiotrZSL wrote:
> this doesnt look valid, arg2 isn't known at this point yet, so this could be removed.
> and this may not work for case like this:
> 
> ```callSomething({new A,  new B});```  
> Most probably this equalsBoundNode should be on HasNewExpr2 level, to avoid duplicated newExpr, not arguments of call
> 
> and image situation like this:
> 
> ```
> try {
>   something(std::shared_ptr<int>(new int), std::shared_ptr<int>(new int));
> } catch(const std::bad_alloc&) {}
> ```
> this in theory could also produce false-positive.
> 
> other issue is that first call could be anything, maloc, new, calloc, wzalloc, operator new(), it doesn't need to be just new.
> You could try to use some simple way of checking this like, isBeforeInTransationUnit....
> 
> And this will also produce false-positive if we use `new(std::nothrow)` on second.
> There are some "utils" to check sequence order, maybe would be good to investigate them.
I am not an expert in how AST matchers work exactly, but this code works with the provided tests and the results look correct. I did not experience that two `new` in the same argument is matched, this is why the `unless(equalsBoundNode(...))` is added. The "arg1" and "arg2" nodes are the direct expressions in the function call, not descendants, and a `new` in the same argument (any descendant) is not matched twice in this way. Otherwise this would show up in failed tests.

The code `something(std::shared_ptr<int>(new int), std::shared_ptr<int>(new int))` should produce warning (it may be possible that result of the first `new int` is not passed to `shared_ptr` before the other `new int` is called that fails, good solution is use of `std::make_shared` in such case). The test code `(void)f(g(new A), new B);` is somewhat similar AST, the `new A` should be found in all cases because `hasDescendant` is used at `HasNewExpr1` and 2.

Probably `unless(equalsBoundNode("arg2"))` can be removed, it is enough to have one of these checks.

`InitListExpr` is not handled by the current code, I need to add this case (it has fixed evaluation order).


================
Comment at: clang-tools-extra/clang-tidy/bugprone/MultipleNewInOneExpressionCheck.cpp:102
+          hasAnyArgument(
+              expr(HasNewExpr1, unless(equalsBoundNode("arg2"))).bind("arg1")),
+          hasAnyArgument(
----------------
balazske wrote:
> PiotrZSL wrote:
> > this doesnt look valid, arg2 isn't known at this point yet, so this could be removed.
> > and this may not work for case like this:
> > 
> > ```callSomething({new A,  new B});```  
> > Most probably this equalsBoundNode should be on HasNewExpr2 level, to avoid duplicated newExpr, not arguments of call
> > 
> > and image situation like this:
> > 
> > ```
> > try {
> >   something(std::shared_ptr<int>(new int), std::shared_ptr<int>(new int));
> > } catch(const std::bad_alloc&) {}
> > ```
> > this in theory could also produce false-positive.
> > 
> > other issue is that first call could be anything, maloc, new, calloc, wzalloc, operator new(), it doesn't need to be just new.
> > You could try to use some simple way of checking this like, isBeforeInTransationUnit....
> > 
> > And this will also produce false-positive if we use `new(std::nothrow)` on second.
> > There are some "utils" to check sequence order, maybe would be good to investigate them.
> I am not an expert in how AST matchers work exactly, but this code works with the provided tests and the results look correct. I did not experience that two `new` in the same argument is matched, this is why the `unless(equalsBoundNode(...))` is added. The "arg1" and "arg2" nodes are the direct expressions in the function call, not descendants, and a `new` in the same argument (any descendant) is not matched twice in this way. Otherwise this would show up in failed tests.
> 
> The code `something(std::shared_ptr<int>(new int), std::shared_ptr<int>(new int))` should produce warning (it may be possible that result of the first `new int` is not passed to `shared_ptr` before the other `new int` is called that fails, good solution is use of `std::make_shared` in such case). The test code `(void)f(g(new A), new B);` is somewhat similar AST, the `new A` should be found in all cases because `hasDescendant` is used at `HasNewExpr1` and 2.
> 
> Probably `unless(equalsBoundNode("arg2"))` can be removed, it is enough to have one of these checks.
> 
> `InitListExpr` is not handled by the current code, I need to add this case (it has fixed evaluation order).
The check only works with memory allocation failures that throw exceptions, but really only with `new`, this is the most often used. Functions like `malloc` do not throw exceptions.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/MultipleNewInOneExpressionCheck.cpp:105
+              expr(HasNewExpr2, unless(equalsBoundNode("arg1"))).bind("arg2")),
+          hasAncestor(BadAllocCatchingTryBlock)),
+      this);
----------------
PiotrZSL wrote:
> To be honest, I don't see any reason, how this try-catch would change anything, there can be one in parent function, and we going to heave a leak if we have try-catch or not.
The problem is that it is difficult to check if the parent function has a try-catch block, and the function can be called in different ways. This can results in false negatives. But without any try-catch block the program may not use exception handling at all (a failed `new` terminates the program), and it can be false positive to emit any warnings.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/MultipleNewInOneExpressionCheck.cpp:125
+      this);
+}
+
----------------
PiotrZSL wrote:
> other issue I see is that same new could be matched multiple times by those Matchers
This may be possible but I think that if the same warning is found multiple times it is shown only once, otherwise every case found by the matchers is a valid warning case even if the same `new` is involved.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138777/new/

https://reviews.llvm.org/D138777