[clang] [clang][ThreadSafety] Check trylock function success and return types (PR #95290)

[Dan McArdle via cfe-commits]

================
@@ -1359,17 +1361,18 @@ void ThreadSafetyAnalyzer::getMutexIDs(CapExprSet &Mtxs, AttrType *Attr,
                                        const Expr *Exp, const NamedDecl *D,
                                        const CFGBlock *PredBlock,
                                        const CFGBlock *CurrBlock,
-                                       Expr *BrE, bool Neg) {
-  // Find out which branch has the lock
-  bool branch = false;
-  if (const auto *BLE = dyn_cast_or_null<CXXBoolLiteralExpr>(BrE))
-    branch = BLE->getValue();
-  else if (const auto *ILE = dyn_cast_or_null<IntegerLiteral>(BrE))
-    branch = ILE->getValue().getBoolValue();
----------------
dmcardle wrote:

Thanks for the feedback, @aaronpuchert! I've reverted the change and I'll work through these comments for a reland.

My instinct is to explicitly disallow enumerator success expressions. Currently, the analysis can produce both false negatives and false positives when enumerator success expressions are used: <https://godbolt.org/z/MPYdK6hYb>. To my knowledge, enumerators were never actually supported in success expressions, so in some sense this will only break code that was already broken.  However, I do want to be sensitive to downstream breakage based on my experience with this PR!

One suggestion was to add a flag that disables any new errors, the goal being to enabling large code bases to incrementally adapt to the new behavior. I'm just not sure how this flag would actually work.

* If we suppress any new type errors, but otherwise apply the attribute, the analysis is now off the rails, operating on unexpected inputs.
* If we simply ignore any attributes that would cause an error to be emitted, now the analysis can produce different kinds of incorrect results, e.g. it might complain that you're unlocking a lock that was never acquired.
* I suppose we could mimic the current/incorrect behavior when the flag is present. Is it normally expected to keep incorrect behavior around for compatibility?

@asmok-g, do you have any thoughts? How are breaking changes such as this typically handled?

https://github.com/llvm/llvm-project/pull/95290