[clang-format] Add option for aligning requires clause body

[rymiel]

Summary

• Add an option whether requires clause body should be aligned with requires keyword
• Add tests for RequiresExpressionIndentation

Fixes https://github.com/llvm/llvm-project/issues/56283

[curdeius]

Haven't you forgotten to add formatting tests? :)

[HazardyKnusperkeks]

Thanks for the patch. Please add the full context for the next revision: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

[eoanermine]

In D129443#3641571, @curdeius wrote:

Haven't you forgotten to add formatting tests? :)

Yes, I have. Thank you.

[eoanermine]

Add tests for AlignRequiresClauseBody option
Add full context

[eoanermine]

In D129443#3644795, @eoanermine wrote:

Add tests for AlignRequiresClauseBody option

Are these tests fine?

[Vigilans]

In my knowledge of clang-format, Requires Clause and Requires Expression are treated differently.
In cppreference, A Requires Clause is specific to the requires keyword used for constraints on template arguments or on a function declaration. In this issue's context of continuation indent, the requires keyword is used for Requires Expression, just as the code relative to this diff suggests:

if (Current.is(TT_RequiresExpression) /* <-- it's requires expression here */ && Style.AlignRequiresClauseBody)
  CurrentState.NestedBlockIndent = State.Column;

Some more example options in clang-format:

• IndentRequiresClause

true:
template <typename It>
  requires Iterator<It>
void sort(It begin, It end) {
  //....
}

false:
template <typename It>
requires Iterator<It>
void sort(It begin, It end) {
  //....
}

• SpaceBeforeParensOptions -> AfterRequiresInClause

true:                                  false:
template<typename T>            vs.    template<typename T>
requires (A<T> && B<T>)                requires(A<T> && B<T>)
...                                    ...

• SpaceBeforeParensOptions -> AfterRequiresInExpression

true:                                  false:
template<typename T>            vs.    template<typename T>
concept C = requires (T t) {           concept C = requires(T t) {
              ...                                    ...
            }                                      }

So the option in this diff should use RequiresExpressionBody.

Besides, there is an option that does similar thing as this diff do:

• LambdaBodyIndentation

The indentation style of lambda bodies. Signature (the default) causes the lambda body to be indented one additional level relative to the indentation level of the signature. OuterScope forces the lambda body to be indented one additional level relative to the parent scope containing the lambda signature. For callback-heavy code, it may improve readability to have the signature indented two levels and to use OuterScope. The KJ style guide requires OuterScope. KJ style guide

Possible values:

• LBI_Signature (in configuration: Signature) Align lambda body relative to the lambda signature. This is the default.

someMethod(
    [](SomeReallyLongLambdaSignatureArgument foo) {
      return;
    });

• LBI_OuterScope (in configuration: OuterScope) Align lambda body relative to the indentation level of the outer scope the lambda signature resides in.

someMethod(
    [](SomeReallyLongLambdaSignatureArgument foo) {
  return;
});

Related code of implementation: https://github.com/llvm/llvm-project/blob/f338f416baf09aaed59b1b3bc693cab5d5cdf7ab/clang/lib/Format/UnwrappedLineFormatter.cpp#L967-L978

So we may select a similar option naming (e.g. RequiresExpressionIndentation) and values (e.g. RequiresExpressionIndentationKind with Keyword and OuterScope) as this lambda body option for consistency, and leaving some room for future update? (I have encountered some more cases about concept definitions that may expect requires expression with more intelligent indentation style, though left behind and conforming with current clang-format style.)

[HazardyKnusperkeks]

In D129443#3645795, @Vigilans wrote:

In my knowledge of clang-format, Requires Clause and Requires Expression are treated differently.
<snip>

+1 for the enum, even if we only have 2 options yet.

And yes, the option is wrongly named. You want to indent/align requires expressions, not clauses.

The tests seem mostly to suffice. You could add a test where the outer scope is indented, e.g. a class with a member function, and a test where the expression is used within an if.

Please also run clang/docs/tools/dump_format_style.py to update the ClangFormatStyleOptions.rst.

[eoanermine]

• Rename AlignRequiresClauseBody to RequiresExpressionIndentation
• Introduce RequiresExpressionIndentationKind as a type of RequiresExpressionIndentation
• Add more tests

[HazardyKnusperkeks]

Looks very promising.

clang/include/clang/Format/Format.h

396

Please resort.

3885

Please resort.

clang/unittests/Format/FormatTest.cpp

20036

Please add the parsing test for the enum.

24805–24810

This is a bad example, since we can't see any difference.

What happens if it's not the first expression in the if.

[rymiel]

Commandeering to finish the final nits as per https://github.com/llvm/llvm-project/issues/56283#issuecomment-1261653291

[rymiel]

Resolve nits:

• Re-sorted uses of the name RequiresExpressionIndentationKind
• Updated version to be 16
• Add test for demonstrating indentation as subexpression
• Reformatted

[rymiel]

Missed a spot when re-sorting

[owenpan]

Is this option really required? Can we just fix the regression as reported in https://github.com/llvm/llvm-project/issues/56283? It seems that we haven't followed the policy lately when adding new options.

[rymiel]

It wasn't an accidental regression, though, it was a conscious formatting decision (maybe ask @HazardyKnusperkeks for more). Similar to how lambdas can be formatted aligned to the introducer, i think this option does make sense.

[HazardyKnusperkeks]

In D129443#3841248, @owenpan wrote:

Is this option really required? Can we just fix the regression as reported in https://github.com/llvm/llvm-project/issues/56283? It seems that we haven't followed the policy lately when adding new options.

If it's a regression or not is a matter of perspective. Before my patch there were no real handling of concepts and they just looked like that. I had changed that (on purpose) if I remember correctly to bring in closer to lambdas.
We can fix that regression, and I will adopt this change for my company to keep the requires expressions where they are. :)

Also asking for a style guide concerning concepts and requires is a bit hard I think, I think in none of the styles we implement it is mentioned, it at least wasn't when I added the handling.

[owenpan]

In D129443#3842427, @HazardyKnusperkeks wrote:

We can fix that regression, and I will adopt this change for my company to keep the requires expressions where they are. :)

Then we have three options:

1. Only fix the "regression".
2. Add the option and make OuterScope the default.
3. Add the option and make Keyword the default.

@HazardyKnusperkeks I will defer to your choice. :)

clang/unittests/Format/FormatTest.cpp

20036

@rymiel Is this done?

[HazardyKnusperkeks]

In D129443#3844535, @owenpan wrote:

In D129443#3842427, @HazardyKnusperkeks wrote:

We can fix that regression, and I will adopt this change for my company to keep the requires expressions where they are. :)

Then we have three options:

1. Only fix the "regression".
2. Add the option and make OuterScope the default.
3. Add the option and make Keyword the default.

@HazardyKnusperkeks I will defer to your choice. :)

Add the option, and then either way we will have a regression to somebody. OuterScope will regress to version 15. Keyword up to 14. So it should be the most reasonable to make OuterScope the default, since the early adapters will most be more open to change their configuration.

@rymiel please add a change log note about that changed default.

[Vigilans]

In D129443#3844805, @HazardyKnusperkeks wrote:

In D129443#3844535, @owenpan wrote:

In D129443#3842427, @HazardyKnusperkeks wrote:

We can fix that regression, and I will adopt this change for my company to keep the requires expressions where they are. :)

Then we have three options:

1. Only fix the "regression".
2. Add the option and make OuterScope the default.
3. Add the option and make Keyword the default.

@HazardyKnusperkeks I will defer to your choice. :)

Add the option, and then either way we will have a regression to somebody. OuterScope will regress to version 15. Keyword up to 14. So it should be the most reasonable to make OuterScope the default, since the early adapters will most be more open to change their configuration.

@rymiel please add a change log note about that changed default.

I am fine with either default option with their persuasive reasons:

1. Default to Keyword: Interpreted as Formally documented formatting behavior, and the options arrangement looks more consistent with LambdaBodyIndentation (Signature (default) and OuterScope).
2. Default to OuterScope: Interpreted as Fix the regression, and the style looks more consistent with example codes in common concept tutorials (the expression is indented the same level as concept keyword, not requires keyword).

For consistency with clang-format current behaviors and options, we may prefer Keyword; For consistency with common current tutorial code, we may prefer OuterScope.

[rymiel]

Flip the default and note that in the Release Notes

Sorry, I did not realize I was mentioned.
I think the default option will always be the first option? So I swapped them around.
Note that I didn't change the LLVM config, which right now is still set to Keyword.
I'm also not sure if the release notes should note this as "important" or "breaking"

[rymiel]

clang/unittests/Format/FormatTest.cpp

20036

The code this comment was added on has been completely removed; so i guess i'll mark it as done

[owenpan]

In D129443#3847319, @rymiel wrote:

Note that I didn't change the LLVM config, which right now is still set to Keyword.

We should change it back to OuterScope.

clang/lib/Format/Format.cpp

1304

Delete it.

[HazardyKnusperkeks]

clang/docs/ReleaseNotes.rst

540 ↗

(On Diff #466551)

Unrelated.

clang/lib/Format/Format.cpp

1304

Why delete?
Set it to OuterScope.

[owenpan]

clang/docs/ReleaseNotes.rst

540 ↗

(On Diff #466551)

Unrelated.

I’m ok with fixing it here so that we don’t need another patch.

clang/lib/Format/Format.cpp

1304

Why delete?
Set it to OuterScope.

Cuz OuterScope is the default. :)

[HazardyKnusperkeks]

clang/lib/Format/Format.cpp

1304

The default is uninitialized. Not setting anything here is undefined behavior.

[owenpan]

clang/lib/Format/Format.cpp

1304

@HazardyKnusperkeks you are right!

[rymiel]

Changing the default LLVM style would change the output of all the requires-related test cases in FormatTest.Concepts. Should I change these test cases to use the new indentation or pass the REI_Keyword style to them?

[owenpan]

In D129443#3857608, @rymiel wrote:

Changing the default LLVM style would change the output of all the requires-related test cases in FormatTest.Concepts. Should I change these test cases to use the new indentation or pass the REI_Keyword style to them?

I prefer the latter so as to minimize the diff.

[HazardyKnusperkeks]

In D129443#3857816, @owenpan wrote:

In D129443#3857608, @rymiel wrote:

Changing the default LLVM style would change the output of all the requires-related test cases in FormatTest.Concepts. Should I change these test cases to use the new indentation or pass the REI_Keyword style to them?

I prefer the latter so as to minimize the diff.

+1

Maybe with a comment, why it isn't tested with the LLVM default.

[rymiel]

Swap the default for LLVM style, and adjust tests which depended on the previous style to still use it.

[rymiel]

How should I proceed with a stale rejecting review?

[owenpan]

In D129443#3867364, @rymiel wrote:

How should I proceed with a stale rejecting review?

You can commit as https://reviews.llvm.org/D129443#3641571 has been addressed.

[HazardyKnusperkeks]

In D129443#3867400, @owenpan wrote:

In D129443#3867364, @rymiel wrote:

How should I proceed with a stale rejecting review?

You can commit as https://reviews.llvm.org/D129443#3641571 has been addressed.

And given that he wasn't active in the last days (weeks?), he will probably not respond anytime soon.

clang/include/clang/Format/Format.h

   /// \version 3.5
   OperandAlignmentStyle AlignOperands;
 
+  /// If ``true``, aligns requires clause bodies with `requires` keyword.
+  /// \code
+  ///   true:                                   false:
+  ///   template <typename T>                   template <typename T>
+  ///   concept C = requires(T t) {      vs.    concept C = requires(T t) {
+  ///                 ...                         ...
+  ///               }                           }
+  /// \endcode
+  /// \version 15
+  bool AlignRequiresClauseBody;
+
   /// If ``true``, aligns trailing comments.
   /// \code
   ///   true:                                   false:

            AlignConsecutiveMacros == R.AlignConsecutiveMacros &&
            AlignEscapedNewlines == R.AlignEscapedNewlines &&
            AlignOperands == R.AlignOperands &&
+           AlignRequiresClauseBody == R.AlignRequiresClauseBody &&
            AlignTrailingComments == R.AlignTrailingComments &&
            AllowAllArgumentsOnNextLine == R.AllowAllArgumentsOnNextLine &&
            AllowAllParametersOfDeclarationOnNextLine ==

clang/lib/Format/ContinuationIndenter.cpp

     CurrentState.NestedBlockIndent = State.Column + Current.ColumnWidth + 1;
   if (Current.isOneOf(TT_LambdaLSquare, TT_LambdaArrow))
     CurrentState.LastSpace = State.Column;
-  if (Current.is(TT_RequiresExpression))
+  if (Current.is(TT_RequiresExpression) && Style.AlignRequiresClauseBody)
     CurrentState.NestedBlockIndent = State.Column;
 
   // Insert scopes created by fake parenthesis.

clang/lib/Format/Format.cpp

     IO.mapOptional("AlignConsecutiveMacros", Style.AlignConsecutiveMacros);
     IO.mapOptional("AlignEscapedNewlines", Style.AlignEscapedNewlines);
     IO.mapOptional("AlignOperands", Style.AlignOperands);
+    IO.mapOptional("AlignRequiresClauseBody", Style.AlignRequiresClauseBody);
     IO.mapOptional("AlignTrailingComments", Style.AlignTrailingComments);
     IO.mapOptional("AllowAllArgumentsOnNextLine",
                    Style.AllowAllArgumentsOnNextLine);

   LLVMStyle.AlignAfterOpenBracket = FormatStyle::BAS_Align;
   LLVMStyle.AlignArrayOfStructures = FormatStyle::AIAS_None;
   LLVMStyle.AlignOperands = FormatStyle::OAS_Align;
+  LLVMStyle.AlignRequiresClauseBody = true;
   LLVMStyle.AlignTrailingComments = true;
   LLVMStyle.AlignConsecutiveAssignments = {};
   LLVMStyle.AlignConsecutiveAssignments.Enabled = false;

clang/unittests/Format/FormatTest.cpp

 TEST_F(FormatTest, ParsesConfigurationBools) {
   FormatStyle Style = {};
   Style.Language = FormatStyle::LK_Cpp;
+  CHECK_PARSE_BOOL(AlignRequiresClauseBody);
   CHECK_PARSE_BOOL(AlignTrailingComments);
   CHECK_PARSE_BOOL(AllowAllArgumentsOnNextLine);
   CHECK_PARSE_BOOL(AllowAllParametersOfDeclarationOnNextLine);