[PATCH] D66830: Consumed checker: various improvements

[Nicholas Allegra via Phabricator via cfe-commits]

comex created this revision.
comex added reviewers: delesley, dblaikie, rsmith.
Herald added subscribers: cfe-commits, kristof.beyls, javed.absar.
Herald added a project: clang.

- Treat arguments to constructor calls the same way as arguments to other calls (fixes 42856).  Previously, arguments to constructors were completely ignored – even if explicitly marked `param_typestate` or `return_typestate` – except for one special case to mark the argument to a move constructor as consumed on return.  Even that special case was broken, as it wouldn't apply if a move constructor happened to be marked with `return_typestate`.
  - Constructors are still special-cased when determining the default typestate for the newly constructed object, unless the constructor is marked `return_typestate`.

- Stop ignoring `return_typestate` on rvalue reference parameters.

- When a function takes a consumable object by value, treat that as consuming by default, same as with an rvalue reference.

  Note that the object being consumed is always a temporary.  If you write:

  `void a(X); a((X &&)x);`

  ...this first constructs a temporary X by moving from `x`, then calls `a` with a pointer to the temporary.  Before and after this change, the move copies `x`'s typestate to the temporary and marks `x` itself as consumed.  But before this change, if the temporary started out unconsumed (because `x` was unconsumed before the move), it would still be unconsumed when its destructor was run after the call.  Now it will be considered consumed at that point.

  Note that currently, only parameters explicitly marked `return_typestate` have their state-on-return checked on the callee side.  Parameters which are implicitly consuming due to being rvalue references – or, after this patch, by-value – are checked only on the caller side.  This discrepancy was very surprising to me as a user.  I wrote a fix, but in my codebase it broke some code that was using `std::forward`.  Therefore, I plan to hold off on submitting the fix until a followup patch, which will generalize the current `std::move` special case into an attribute that can be applied to any 'cast' function like `std::move` and `std::forward`.

- Diagnose if a constructor or a static method is marked `set_typestate`, instead of (respectively) ignoring it or crashing.

- Diagnose if a `param_typestate` or `return_typestate` attribute is attached to a parameter of non-consumable type.  The wording I came up with is a bit fuzzy; suggestions welcome.

- Fix broken check that was trying to `dyn_cast` a `CXXOperatorCallExpr` to a sibling class, `CXXMemberCallExpr` (that will never work).  Instead, we need to check whether the declaration is a `CXXMethodDecl`.
  - This was harmless in the existing code, for somewhat complicated reasons, but caused problems after I refactored `handleCall` to take the arguments as a separate parameter.

Some of these changes have the potential to 'break' code (in the sense of creating warnings where there were none before) because they check things that were previously ignored.

One notable case, which showed up in the existing test code (warn-consumed-analysis.cpp), is a copy constructor that takes its argument by non-const reference.  Previously, the argument would be ignored and thus implicitly treated as leaving the state alone – just like all other constructor arguments, except for the argument to a move constructor.  Now it's given the usual treatment for non-const reference parameters, which is to mark the original object as being in `unknown` state, unless overridden with `return_typestate`.  I think the new behavior is more correct, and this isn't removing a special case *per se*.  However, since it is a "copy" constructor, there might be some argument that the argument should be treated as non-consuming instead.

Also, if anyone wants to test this on their code, note that I have a separate patch pending to make the CFG more accurate with respect to temporary object destructors (D66404 <https://reviews.llvm.org/D66404>).


Repository:
  rC Clang

https://reviews.llvm.org/D66830

Files:
  include/clang/Analysis/Analyses/Consumed.h
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Analysis/Consumed.cpp
  lib/Sema/AnalysisBasedWarnings.cpp
  lib/Sema/SemaDeclAttr.cpp
  test/SemaCXX/warn-consumed-analysis.cpp
  test/SemaCXX/warn-consumed-parsing.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D66830.217492.patch
Type: text/x-patch
Size: 18134 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190827/023f3200/attachment-0001.bin>