[clang-format] Ability to configure breaking after short return types

[rmarker]

Currently, when AlwaysBreakAfterReturnType is None, line breaks are prevented after 'short' return types.

From ContinuationIndenter.cpp

// Don't break after very short return types (e.g. "void") as that is often
// unexpected.
if (Current.is(TT_FunctionDeclarationName) && State.Column < 6) {
  if (Style.AlwaysBreakAfterReturnType == FormatStyle::RTBS_None)
    return false;
}

Note, that 'short' here isn't the length of the type itself, but the column where the type finishes.
So, the indentation can change the behaviour.

While the comment says that breaking would be unexpected, this behaviour takes precedence over any of the penalty options for configuring where to place line breaks. It is fairly specific behaviour and can also be unexpected as it avoids breaking after the return type seemly ignoring the penalty configuration.

It would be helpful if there was a way that this could be configured.

[rmarker]

It isn't exactly clear the best way that this behaviour could be configured.
It appears to have been a part of clang-format for some time.
There is the possibility of doing something more complex, for example shifting the behaviour to be a penalty instead.
However, since the current behaviour is so specific, in order to ensure backwards compatibility, a simple enable/disable is probably more suitable.

While investigating this, I experimented with an option to configure the column limit to mark the short return type.
Since this can effectively work as an on/off switch, it didn't seem prudent to limit it to a boolean.

I made a pull request (#78011) with the result of this work for consideration.

[mydeveloperday]

Should we use a field called ShortTypes where users could define the types they want

[rmarker]

Do you mean an option where you would list all the types you want to be affected by the short type behaviour?
I hadn't considered an approach like that.
That would address my scenario, allowing for the behaviour to be avoided by not enabling it for any types.

The main consideration would be whether it would cause a formatting change when upgrading to the new version.
All the affected types would have to be specified, otherwise the formatting would change.
Not sure if that is something you would want to avoid.

Also, if changing previous formatting, having it be based on the type itself rather than the column would make things clearer.
Since, it depending on the indentation is one way that it can be surprising.
For example:

// Short type, break after return type disallowed.
Type Looooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooong::
    FunctionDeclaration();

struct S {
  // Not short type due to indentation for the struct. Break after return type allowed.
  Type
  Loooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooong::
      FunctionDeclaration();
}

Though, I wonder whether it could be frustrating if someone wanted it to apply to all types under a certain length. I.e. having to manually add lots of types to the list.

[owenca]

Currently, when AlwaysBreakAfterReturnType is None, line breaks are prevented after 'short' return types.

From ContinuationIndenter.cpp

// Don't break after very short return types (e.g. "void") as that is often
// unexpected.
if (Current.is(TT_FunctionDeclarationName) && State.Column < 6) {
  if (Style.AlwaysBreakAfterReturnType == FormatStyle::RTBS_None)
    return false;
}

Note, that 'short' here isn't the length of the type itself, but the column where the type finishes. So, the indentation can change the behaviour.

That's likely a bug. IMO we should fix the bug and optionally add a new boolean value to AlwaysBreakAfterReturnType for backward compatibility.

[rmarker]

Yeah, the fact that the short return type is based on the column and not the type itself is strange and could be a bug.
When investigating I took a conservative approach and looked to maintain the existing behaviour.

Though I wouldn't say that just changing it to use the type length would completely solve the issue.
The indentation changing the behaviour does make the short return type behaviour much more unexpected/unintuitive.
However, I would say that the entire concept of preventing breaks after short return types is also potentially confusing in of itself.
There isn't any configuration options for it, and it can seem to conflict with PenaltyReturnTypeOnItsOwnLine.
Especially since by preventing the break, it can cause a line to go over the ColumnLimit for no 'apparent' reason.

Hence how I ended up at a possible solution which enables the functionality to be effectively disabled.
I went with a top-level option, but it does make sense for it to be connected to AlwaysBreakAfterReturnType.
Being a top-level option I thought it could be a configurable size rather than a boolean, but that was more why not, rather than there being a strong motivation for having that part configurable.
So maybe three possible states, column (current), type-length, off?

[owenca]

Yeah, the fact that the short return type is based on the column and not the type itself is strange and could be a bug.

IMO the bug is not that it doesn't use the length of the type. Instead, the issue is that the (absolute) column position is not adjusted for any indentation. So using the offset relative to the starting column position of the line should be enough to fix the problem.

[rmarker]

I agree with that.
In my mind, I'm just using return type length to mean the same thing as the relative column. In contrast to the absolute column.

With regards to the different states, should it be another enum for AlwaysBreakAfterReturnType, or maybe turning it into a struct, or leaving it at the top level?

[owenca]

How about adding a new boolean suboption enum value (e.g. RTBS_Short) to AlwaysBreakAfterReturnType?

[rmarker]

I'm just wondering whether that would suggest that it would always break short return types, rather than the opposite of allowing to break after short return types.
If we could change RTBS_None to be the option for no restrictions, the new one could be RTBS_NeverShort? Otherwise, what about RTBS_AllowShort?

[owenca]

You are right! I think RTBS_AllowShortType should work. We probably want to rename AlwaysBreakAfterReturnType to BreakAfterReturnType and change RTBS_None to RTBS_Automatic in another patch.

[rmarker]

Right. I will look into reworking the PR to use RTBS_AllowShortType.
Thanks.

[rmarker]

PR has now been updated with this new approach.

[rmarker]

@owenca, now that the change is in, do you want me to look at renaming AlwaysBreakAfterReturnType to BreakAfterReturnType?
I assume that AlwaysBreakAfterReturnType would be deprecated similarly to AlwaysBreakAfterDefinitionReturnType.

[owenca]

@owenca, now that the change is in, do you want me to look at renaming AlwaysBreakAfterReturnType to BreakAfterReturnType? I assume that AlwaysBreakAfterReturnType would be deprecated similarly to AlwaysBreakAfterDefinitionReturnType.

Yep. Thanks!