[clang] [clang-format] Fix a bug in annotating angles containing FatArrow (PR #108671)

[Owen Pan via cfe-commits]

================
@@ -243,14 +244,16 @@ class AnnotatingParser {
       // operator that was misinterpreted because we are parsing template
       // parameters.
       // FIXME: This is getting out of hand, write a decent parser.
-      if (InExpr && !Line.startsWith(tok::kw_template) &&
+      if (InExpr && !SeenFatArrow && !Line.startsWith(tok::kw_template) &&
           Prev.is(TT_BinaryOperator)) {
         const auto Precedence = Prev.getPrecedence();
         if (Precedence > prec::Conditional && Precedence < prec::Relational)
           return false;
       }
       if (Prev.isOneOf(tok::question, tok::colon) && !Style.isProto())
         SeenTernaryOperator = true;
+      else if (Prev.is(TT_FatArrow))
----------------
owenca wrote:

> > In your [#108671 (comment)](https://github.com/llvm/llvm-project/pull/108671#discussion_r1762725421) above, it doesn't seem that your C++ example return FixedInt<N | M>(foo); is from an existing codebase.
> 
> Sorry if I gave that impression, but I was delibaretly trying to make it more concrete as you were asking for more specifics, here's some real world examples: https://github.com/search?q=lang%3Ac%2B%2B+%2FFixedInt%3C.*%5C%7C.*%3E%2F&type=code

The GitHub examples you listed here have zero arguments and thus are unambiguous to clang-format. (See #110408.) As I mentioned or implied above, a pair of empty parentheses after the `>` would help. We can add another patch (if needed) for multiple arguments such as `return FixedInt<N | M>(foo, bar);` , which is also unambiguous.

However, the initial version of your C++ example `return FixedInt<N | M>(foo);` has exactly one argument and can be also interpreted as a non-template expression similar to `Foo < A || B > (C ^ D);`.

> > The fact that it happened to get formatted that way before doesn't necessarily mean that we should always format another similar pattern e.g. return Foo < A || B > (C ^ D); the same way, especially if the latter is much more likely.
> 
> No I totally get that. That's how clang-format evolves over time. But as I explained above, my understanding is this is done in a way that it doesn't regress behavior for already formatted code. As otherwise it becomes really troublesome to adopt new versions of clang-format. Hence the change in #100980, didn't simply improve formatting for some mishandled pattern, it also regressed handling of some existing pattern.

It goes without saying that we should strive to keep clang-format regression free. Nevertheless, not all behavior changes are regressions. Also, some past regressions can't simply be reverted as it would cause new regressions. The change in #100980 is an example of the latter.

> I am totally OK if your verdict here is this is a cost we're willing to pay, in the end you're the maintainer. I am just trying to make sure this doesn't go unnoticed and someone is making a call deliberately. Even if it isn't aligned with the outcome that I might like.

With #109916 we can cover both `return Foo < A || B > (C ^ D);` and `return Foo<A | B>(C ^ D);`. It may require changing the existing configuration for the latter case, but the alternative (i.e. not supporting the former case or having a `NonTemplateNames` option instead) would be worse IMO.

This patch is to fix the TypeScript issue as initially reported in #108536. As #109925 also fixes that issue (probably incidentally), I'll close this one if you think special casing the fat arrow for JavaScript/TypeScript is unnecessary.

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