[clang-format] [PR36294] AlwaysBreakAfterReturnType works incorrectly for some operator functions

[MyDeveloperDay]

Summary

https://bugs.llvm.org/show_bug.cgi?id=36294

Addressing bug related to returning after return type not being honoured for some operator types.

$ bin/clang-format --style="{BasedOnStyle: llvm, AlwaysBreakAfterReturnType: TopLevelDefinitions}" /tmp/foo.cpp
class Foo {
public:
  bool operator!() const;
  bool operator<(Foo const &) const;
  bool operator*() const;
  bool operator->() const;
  bool operator+() const;
  bool operator-() const;
  bool f() const;
};

bool Foo::operator!() const { return true; }
bool
Foo::operator<(Foo const &) const {
  return true;
}
bool Foo::operator*() const { return true; }
bool Foo::operator->() const { return true; }
bool
Foo::operator+() const {
  return true;
}
bool
Foo::operator-() const {
  return true;
}
bool
Foo::f() const {
  return true;
}

[sammccall]

clang/lib/Format/TokenAnnotator.cpp

1347–1349

This seems clearer as is semi || (is exclaim && !previous is operator), as operator; isn't relevant here.
&& !Current.is(TT_OverloadedOperator) would be even better of course

2092

This seems like it's supposed to be handled by the token annotator, and the same cause will result in bugs in other places - why aren't these tokens being marked as TT_OverloadedOperator?

I'd guess the deeper fix is in the kw_operator case in consumeToken(). The way it's written looks suspect, though I'm not an expert on any of this.

[MyDeveloperDay]

Trying to address review comments

[MyDeveloperDay]

clang/lib/Format/TokenAnnotator.cpp

1347–1349

rearranged the logic as suggested, unfortunately we can't use TT_LoverloadOperator at this point because its not yet been annotated (so ! has TT_Unknown still)

2092

The * from *() is already labelled as a TT_PointerOrRefernce so can't also be marked as an Overloaded operator so use that in the loop.

[sammccall]

clang/lib/Format/TokenAnnotator.cpp

2092

Right, the fact that it's incorrectly labeled as a PointerOrReference is a bug, is it not possible to fix that instead of working around it here?

In the consumeToken loop, it seems the * in operator * is labeled as PointerOrReference by logic that's trying to handle e.g. StringRef::operator char*(). However when the * is _immediately_ preceded by operator, it's always an overloaded operator name rather than a pointer, I think?

[MyDeveloperDay]

Extend this revision to cover additional https://bugs.llvm.org/show_bug.cgi?id=43783 issue (which has overlap)

New revision correctly formats the following code:

constexpr auto
operator*() const -> reference {
  bfexpects_when_compliant(m_i < m_t->size());
  return m_t->get()[m_i];
}

constexpr auto
operator->() const -> pointer {
  bfexpects_when_compliant(m_i < m_t->size());
  return &m_t->get()[m_i];
}

constexpr auto
operator[](index_type n) const -> reference {
  bfexpects_when_compliant(m_i < m_t->size());
  return m_t->get()[m_i];
}

constexpr auto
operator++() -> lra_iterator & {
  ++m_i;
  return *this;
}

constexpr auto
operator()() const -> reference {}

constexpr auto
operator++() const -> reference {}

constexpr auto
operator--() const -> reference {}

constexpr auto
operator*() const -> reference {}

constexpr auto
operator->() const -> reference {}

constexpr auto
operator>>() const -> reference {}

constexpr auto
operator<<() const -> reference {}

constexpr auto
operator[]() const -> reference {}

constexpr auto
operator void*() const -> reference {}

constexpr auto
operator char*() const -> reference {}

constexpr auto
operator Foo*() const -> reference {}

constexpr auto
operator!() const -> reference {}

class Foo {
public:
  bool operator!() const;
  bool operator<(Foo const &) const;
  bool operator*() const;
  bool operator->() const;
  bool operator+() const;
  bool operator-() const;
  bool f() const;
};

bool
Foo::operator!() const {
  return true;
}
bool
Foo::operator<(Foo const &) const {
  return true;
}
bool
Foo::operator*() const {
  return true;
}
bool
Foo::operator->() const {
  return true;
}
bool
Foo::operator+() const {
  return true;
}
bool
Foo::operator-() const {
  return true;
}
bool
Foo::f() const {
  return true;
}

[sammccall]

clang/lib/Format/TokenAnnotator.cpp

955

I'm confused about this: ISTM that where we were previously always treating star as a pointer, now we're always treating it as an operator name.
Whereas it's sometimes an operator name (immediately after the operator keyword) and sometimes a pointer (following a type name).

Maybe we should shift the OverloadedOperator labelling outside the loop (since AFAICT it should only apply to the first token) and then keep the loop to mark stars/amps elsewhere in the operator name as PointerOrReference?

2626

why?

clang/unittests/Format/FormatTest.cpp

6989

this looks like a regression at first glance, in LLVM style there's a space between type and *

[MyDeveloperDay]

clang/lib/Format/TokenAnnotator.cpp

955

Let me take another look..

2092

Treating * and -> as an TT_OverloadedOperator is probably ok, although this breaks unit test I highlight below, which I almost think is incorrect anyway.

I'm looking into a related issue https://bugs.llvm.org/show_bug.cgi?id=43783, which is changing this code again, I may roll both changes into the same fix

However this is made much more complex to deal with it just by using the TT_OverloadedOperator when the operator consists of 2 tokens say like [ and ] unless I combine them into a single token.

operator void*()
operator char*()
operator []()

hence the need for code to handle in this loop for handling.

operator new[]()
operator delete[]()

clang/unittests/Format/FormatTest.cpp

6989

Treating * as TT_OverloadedOperator will break this test, which perhaps is already wrong!

6989

yes, I was a little concerned about this change too, it's hard to know if operator void * was intended as it falls off the bottom of the spacesBetween function with a return true; but if you are concerned then perhaps its better to change it back.

[MyDeveloperDay]

Remove test regression

[MyDeveloperDay]

clang/lib/Format/TokenAnnotator.cpp

2626

LLVM style and google style is different based on pointer alignment

clang/unittests/Format/FormatTest.cpp

6989

reverted

[MyDeveloperDay]

clang/lib/Format/TokenAnnotator.cpp

955

@sammccall In trying to understand what you were suggesting I tried the following:

bool
Foo::operator*(void *) const {
  return true;
}

The second * will be as seen correctly below a PointerOrReference by the nature that we already hit the OverloadedOperatorLParen as the loop only goes until it see a ( ) or ;

I'm trying to think of a case where that perhaps wouldn't be the case? such that we might get 2 OverloadedOperators

M=0 C=0 T=Unknown S=1 B=0 BK=0 P=0 Name=bool L=4 PPK=2 FakeLParens= FakeRParens=0 II=0x1cea7c36c40 Text='bool'
 M=1 C=1 T=FunctionDeclarationName S=1 B=0 BK=0 P=80 Name=identifier L=84 PPK=2 FakeLParens= FakeRParens=0 II=0x1cea7c42298 Text='Foo'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=23 Name=coloncolon L=86 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='::'
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=520 Name=operator L=94 PPK=2 FakeLParens= FakeRParens=0 II=0x1cea7c36ed8 Text='operator'
 M=0 C=0 T=OverloadedOperator S=0 B=0 BK=0 P=34 Name=star L=95 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='*'
 M=0 C=0 T=OverloadedOperatorLParen S=0 B=0 BK=0 P=34 Name=l_paren L=96 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=140 Name=void L=100 PPK=2 FakeLParens= FakeRParens=0 II=0x1cea7c368e0 Text='void'
 M=0 C=1 T=PointerOrReference S=1 B=0 BK=0 P=230 Name=star L=102 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='*'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=54 Name=r_paren L=103 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
 M=0 C=1 T=TrailingAnnotation S=1 B=0 BK=0 P=170 Name=const L=109 PPK=2 FakeLParens= FakeRParens=0 II=0x1cea7c363e8 Text='const'
 M=0 C=0 T=FunctionLBrace S=1 B=0 BK=1 P=23 Name=l_brace L=111 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='{'

2626

I believe that now I'm no longer marking that * as a TT_PointerOrReference, one of the other spaceRequiredBetween rules is missing for this case and now we need to consider the pointer alignment.

Without adding this rule to handle this case one of the 2 items from the same UnderstandsOverloadedOperators test fail

verifyFormat("operator void *();");
...
verifyGoogleFormat("operator void*();");

its possible I could clarify this further make ensuring the star is also an overloaded operator e.g.

if (Right.is(tok::star)  && Right.is(TT_OverloadedOperator)

[sammccall]

clang/lib/Format/TokenAnnotator.cpp

955

I mean something like a conversion operator to pointer: bool Foo::operator void *();
The star appears before the operator arg list, but it's still a pointer.

So the testcases I'd try here would be:

Foo::operator*(); // should be OverloadedOperator
Foo::operator void *(); // should be PointerOrReference
Foo::operator()(void *); // should be PointerOrReference
Foo::operator*(void *); // should be OverloadedOperator, PointerOrReference
operator*(int(*)(), class Foo); // OverloadedOperator, PointerOrReference (this one is silly and doesn't need to work)

2105

I'm suspicious of code that handles star but not amp, though maybe I shouldn't be.

[MyDeveloperDay]

Address review comments

[MyDeveloperDay]

clang/lib/Format/TokenAnnotator.cpp

2105

added those and mutated your test cases to cover that

[merge_guards_bot]

Build result: pass - 59827 tests passed, 0 failed and 762 were skipped.
Log files: console-log.txt, CMakeCache.txt

[rianquinn]

Looking at the unit tests, it appears that you have successfully fixed the bug

[MyDeveloperDay]

gentle ping!

[sammccall]

Thanks, looks great!
Sorry for the delay, I was out last week.

[sylvestre.ledru]

@MyDeveloperDay @hans what about adding this to the release notes?
I was trying clang-format 10 on Firefox code base and I noticed this change which isn't documented in
https://prereleases.llvm.org/10.0.0/rc1/tools/clang/docs/ReleaseNotes.html#clang-format
(I can do it if you want)

[hans]

In D69573#1900089, @sylvestre.ledru wrote:

@MyDeveloperDay @hans what about adding this to the release notes?
I was trying clang-format 10 on Firefox code base and I noticed this change which isn't documented in
https://prereleases.llvm.org/10.0.0/rc1/tools/clang/docs/ReleaseNotes.html#clang-format
(I can do it if you want)

Release notes are very welcome :-)

[sylvestre.ledru]

@hans done here: https://github.com/llvm/llvm-project/commit/50eedc134a219ef6d2345e4efc5471a2e3824223 :)

[sylvestre.ledru]

FYI, in Mozilla build of clang-format, we reverted this change.
It was causing more issues than fixes.
https://bugzilla.mozilla.org/show_bug.cgi?id=1629853

[MyDeveloperDay]

Let me look into this a little, I'd prefer we fixed the corner cases than reverted the lot (https://bugs.llvm.org/show_bug.cgi?id=45357)

see D78879: [clang-format] [PR45357] Fix issue found with operator spacing for possible fix