Page MenuHomePhabricator

[clang-format] Improve clang-formats handling of concepts
ClosedPublic

Authored by MyDeveloperDay on May 12 2020, 3:06 AM.

Details

Summary

This is a starting point to improve the handling of concepts in clang-format. There is currently no real formatting of concepts and this can lead to some odd formatting, e.g.

requires(R range) {
  typename Iterator_type<R>;
  { begin(range) }
  ->Iterator_type<R>;
  { end(range) }
  ->Iterator_type<R>;
  requires Input_iterator<Iterator_type<R>>();
};

template <typename T> concept Large = sizeof(T) > 10;

template <typename T, typename U> concept FooableWith = requires(T t, U u) {
  typename T::foo_type;
  { t.foo(u) }
  ->typename T::foo_type;
  t++;
};

The revision starts by resolving the additional newline added before the implicit conversion constraint and ensures that the concept keyword is always on the line below template<>

template <typename T>
concept bool EqualityComparable = requires(T a, T b) {
  { a == b } -> bool;
};

Additional Options include the following to allow a 1 level indentation of the requires clauses (as seems to be popular in a couple of sources, online reference and conference slides about concepts)

  • IndentRequires (indent the "requires keyword after template")

e.g.

template <class I, class S>
   requires Sentinel<S, I>()
constexpr void advance(I &i, S bound) noexcept(noexcept(++i != bound)) {
  while (i != bound) {
    ++i;
  }
}

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
miscco added inline comments.May 19 2020, 6:05 AM
clang/lib/Format/UnwrappedLineParser.cpp
2337

I guess you could use parseBracedList(/*ContinueOnSemicolons=*/false, /*IsEnum=*/false,/*ClosingBraceKind=*/tok::greater); here?

miscco added inline comments.May 19 2020, 10:45 PM
clang/lib/Format/UnwrappedLineParser.cpp
2337

To be more specific, I am concerned what happens if you have multiple template arguments where a linebreak should occur. Is this still happening here?

template <typename someLongTypename1, typename someLongTypename2>
concept someVeryLongConceptName = someVeryLongConstraintName1<someLongTypename1 && someLongTypename2>;
clang/unittests/Format/FormatTest.cpp
17194

I think concept bool should just be concept occurs below

MyDeveloperDay planned changes to this revision.May 20 2020, 2:29 AM
MyDeveloperDay marked 3 inline comments as done.
MyDeveloperDay added inline comments.
clang/lib/Format/TokenAnnotator.cpp
1624–1628

I'm not sure how possible that is as its dealt with in terms of normal parseBlock() handling so there might not be a possiblity to label the -> (which is actually what I'm just doing here) the other rules regarding TT_TrailingReturnArrow will ensure its doesn't get broken and there is spaces around it.

MyDeveloperDay marked 2 inline comments as done.

Add new AlwaysBreakBeforeConceptDeclarations

Better handling of requires expressions vs constraint expression

add more concept tests from the twitter-sphere

MyDeveloperDay marked 7 inline comments as done.May 20 2020, 5:54 AM
MyDeveloperDay added inline comments.
clang/lib/Format/UnwrappedLineParser.cpp
2306

I'm making some of these changes, the problem is with the form

concept id = Foo<T> && Bar<T>

in this case the

Foo<T> && Bar<T>

will be processed by the parseStructualElement() and this gets confused with the && being a TT_PointerOrRefernce and not a TT_BinaryOperator and that throws everything out.

I think parseStructualElement() is thinking Foo<T> && is part of

foo(Foo<T> &&abc);

and not

Foo<T> && abc<T>

We need to know we are in a concept expression

2337

This is formatted as

template <typename someLongTypename1, typename someLongTypename2>
concept someVeryLongConceptName =
    someVeryLongConstraintName1<someLongTypename1 && someLongTypename2>;

Question: Should I add my wip work as a child revision or what would you suggest

MyDeveloperDay marked 2 inline comments as done.May 20 2020, 7:35 AM

Question: Should I add my wip work as a child revision or what would you suggest

Certainly your revision on github was useful for me to take a look at, but to be honest this change is getting large and I don't like landing such a large change in one go but rather make small incremental improvements (I believe this is the LLVM way)

Feel free to create a revision (and reference it here). Once we land some form of initial implementation I feel we can start making improvement as we detect corner cases or adding new options (and that might be more easily spread between developers)

To be honest I would rather start getting bugs from external users than clang-format being the butt of peoples complaints https://twitter.com/the_moisrex/status/1262982971973423105

It seems the spacing of the binary operator is not yet stable. This test is breaking for me:

verifyFormat("template <typename T>\nconcept someConcept = Constraint1<T> && Constraint2<T>;");

It seems the spacing of the binary operator is not yet stable. This test is breaking for me:

verifyFormat("template <typename T>\nconcept someConcept = Constraint1<T> && Constraint2<T>;");

Hmm... are you upto date

$ ./FormatTests.exe --gtest_filter=*Concept*
Note: Google Test filter = *Concept*
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from FormatTest
[ RUN      ] FormatTest.ConceptsAndRequires
[       OK ] FormatTest.ConceptsAndRequires (642 ms)
[----------] 1 test from FormatTest (645 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (650 ms total)
[  PASSED  ] 1 test.
miscco added inline comments.May 23 2020, 3:54 AM
clang/lib/Format/UnwrappedLineParser.cpp
2313

Line->Level is an unsigned int so this gives me a warning. I guess it should be unsigned int OriginalLevel same below

switch function to unsigned

JohelEGP requested changes to this revision.Jun 25 2020, 6:47 PM
JohelEGP added a subscriber: JohelEGP.

BreakBeforeBraces: Allman isn't respected.

template <class D, class U, class Rep2>
friend constexpr auto
operator*(const size2d& l, const units::quantity<D, U, Rep2>& r) noexcept(
    noexcept(l.w* r)) requires(requires { l.w* r; }) //
{
    return size2d<decltype(l.w() * r)>{l.w * r, l.h * r};
}

When a trailing requires-clause is right before the function body, its opening brace should go on its own line, as dictated by my config file.

This revision now requires changes to proceed.Jun 25 2020, 6:47 PM
curdeius added inline comments.
clang/include/clang/Format/Format.h
533

It would be nice to have an example here in the doc.

clang/unittests/Format/FormatTest.cpp
14098

Format: leading whitespace. Should it be in the same "group" as AlignConsecutiveMacros?

17466

Isn't it a strange indentation?

constexpr plane(const plane& other) noexcept(
    (detail::default_allocator_is_nothrow &&
     std::is_nothrow_copy_constructible_v<T>)) requires(std::copyable<T>)
  : plane{to1d(other), other.sz.w}
{
}

Without the parentheses in trailing requires-clauses, all code following the constructor initializer list is further indented.

MyDeveloperDay marked 3 inline comments as done.Jul 1 2020, 3:55 AM
MyDeveloperDay added inline comments.
clang/unittests/Format/FormatTest.cpp
14098

I might be misunderstanding your comment, but the list should alphabetic but now I realize it should be after the Allows

17466

there is no newline on the line above, but I clang-format the tests so it wraps it, I know visually its confusing

MyDeveloperDay marked an inline comment as done.Jul 1 2020, 3:56 AM
constexpr plane(const plane& other) noexcept(
    (detail::default_allocator_is_nothrow &&
     std::is_nothrow_copy_constructible_v<T>)) requires(std::copyable<T>)
  : plane{to1d(other), other.sz.w}
{
}

Without the parentheses in trailing requires-clauses, all code following the constructor initializer list is further indented.

Just so I'm clear for this and your Allman coment would you show me what you are seeing and what you expect to see? just so I understand.

Just so I'm clear for this and your Allman coment would you show me what you are seeing and what you expect to see? just so I understand.

Sure, thank you.

This is what I have, as shown above. Actually fixed to show surrounding context, which is relevant to formatting. Note the // comment:

namespace jge
{
template <std::regular Rep>
struct [[nodiscard]] size2d
{
    template <class D, class U, class Rep2>
    friend constexpr auto
    operator*(const size2d& l, const units::quantity<D, U, Rep2>& r) noexcept(
        noexcept(l.w* r)) requires(requires { l.w* r; }) //
    {
        return size2d<decltype(l.w() * r)>{l.w * r, l.h * r};
    }
};

} // namespace jge

I added // to make the formatting look like what I want. If I remove it, this is what I get:

namespace jge
{
template <std::regular Rep>
struct [[nodiscard]] size2d
{
    template <class D, class U, class Rep2>
    friend constexpr auto
    operator*(const size2d& l, const units::quantity<D, U, Rep2>& r) noexcept(
        noexcept(l.w* r)) requires(requires { l.w* r; }) {
        return size2d<decltype(l.w() * r)>{l.w * r, l.h * r};
    }
};

} // namespace jge

So this is what I want, without the // comment:

namespace jge
{
template <std::regular Rep>
struct [[nodiscard]] size2d
{
    template <class D, class U, class Rep2>
    friend constexpr auto
    operator*(const size2d& l, const units::quantity<D, U, Rep2>& r) noexcept(
        noexcept(l.w* r)) requires(requires { l.w* r; }) 
    {
        return size2d<decltype(l.w() * r)>{l.w * r, l.h * r};
    }
};

} // namespace jge

For the constructor initializer list, this is what I have. Note the parentheses in the first requires-clause:

namespace jge
{
template <class T>
class [[nodiscard]] plane
{
    constexpr plane(const plane& other) noexcept(
        (detail::default_allocator_is_nothrow &&
         std::is_nothrow_copy_constructible_v<T>)) requires(std::copyable<T>)
      : plane{to1d(other), other.sz.w}
    {
    }

    constexpr plane& operator=(const plane& other) noexcept(
        (std::is_nothrow_copy_constructible_v<plane> &&
         std::is_nothrow_copy_assignable_v<T>)) requires std::copyable<T> //
    {
        assign(to1d(other), other.sz.w);
        return *this;
    }
};

} // namespace jge

This is what I get without the parentheses. Note that all code after the constructor initializer list is further indented:

namespace jge
{
template <class T>
class [[nodiscard]] plane
{
    constexpr plane(const plane& other) noexcept(
        (detail::default_allocator_is_nothrow &&
         std::is_nothrow_copy_constructible_v<T>)) requires std::copyable<T>
      : plane{to1d(other), other.sz.w}
      {
      }

      constexpr plane& operator=(const plane& other) noexcept(
          (std::is_nothrow_copy_constructible_v<plane> &&
           std::is_nothrow_copy_assignable_v<T>)) requires std::copyable<T> //
      {
          assign(to1d(other), other.sz.w);
          return *this;
      }
};

} // namespace jge

So this is what I want. Without extra parentheses, nor extra indentation:

namespace jge
{
template <class T>
class [[nodiscard]] plane
{
    constexpr plane(const plane& other) noexcept(
        (detail::default_allocator_is_nothrow &&
         std::is_nothrow_copy_constructible_v<T>)) requires std::copyable<T>
      : plane{to1d(other), other.sz.w}
    {
    }

    constexpr plane& operator=(const plane& other) noexcept(
        (std::is_nothrow_copy_constructible_v<plane> &&
         std::is_nothrow_copy_assignable_v<T>)) requires std::copyable<T> //
    {
        assign(to1d(other), other.sz.w);
        return *this;
    }
};

} // namespace jge

All with the config file linked above.

Thank you I see now... super subtle but I'll take a look.

Using parameter packs within non-parenthesized requires-clauses results in weird formatting:

template <std::semiregular F, std::semiregular... Args>
    requires std::invocable < F, std::invoke_result_t<Args>
... > struct constant;

vs

template <std::semiregular F, std::semiregular... Args>
    requires(std::invocable<F, std::invoke_result_t<Args>...>)
struct constant;

Still a work in progress but covers I think the case @JohelEGP found.

curdeius marked an inline comment as done.Jul 2 2020, 1:51 PM
curdeius added inline comments.
clang/unittests/Format/FormatTest.cpp
14098

I just wanted to point out that when you don't put a blank line after CHECK_PARSE_BOOL, then the next one gets indented. And yes, the order isn't ok. BTW, us "Always" in the name necessary? Cf. other "Break" options.

17466

O, ok, thanks for the explanation. It's indeed confusing in the review.

MyDeveloperDay marked an inline comment as done.Jul 3 2020, 2:05 AM
MyDeveloperDay added inline comments.
clang/unittests/Format/FormatTest.cpp
14098

@curdeius I didn't get to make changes to match your previous review comments yet, but I 100% agree with you why did I put Always here!

I WILL remove that as I can see further down the road if this got changed to an enumeration it would be

AlwaysBreakBeforeConceptDeclarations = Always
AlwaysBreakBeforeConceptDeclarations = Sometimes
AlwaysBreakBeforeConceptDeclarations = Never

Which would be very confusing...

I'll address those concerns next

MyDeveloperDay marked 4 inline comments as done.

Addressed review comments
Added one or two more tests

NOTE: renamed the option from AlwaysBreakBeforeConceptDeclarations -> BreakBeforeConceptDeclarations
JohelEGP requested changes to this revision.Jul 4 2020, 8:17 PM

Thank you. Everything I reported works fine now.

I have two more cases for now. First are your examples in the OP.
You have

template <typename T>
concept bool EqualityComparable = requires(T a, T b) {
  { a == b } -> bool;
};

But this is what I get:

template <typename T>
concept bool EqualityComparable = requires(T a, T b)
{
    {
        a == b
        } -> bool;
};

Perhaps it's because I compiled against commit 71d88cebfb42c8c5ac2d54b42afdcca956e55660 (Fri Jul 3 13:46:41 2020 -0400).
Next:

template <std::semiregular F, std::semiregular... Args>
    requires std::invocable<F, std::invoke_result_t<Args>...>
struct [[nodiscard]] constant : std::bool_constant < requires
{
    typename _require_constant_<(std::invoke(F{}, Args{}()...), 0)>;
} > {};

There's spaces around the template brackets. The lack of indentation is also odd, considering that in this context things are indented.

This revision now requires changes to proceed.Jul 4 2020, 8:17 PM

Another weird formatting when direct-initializing a variable with a non-parenthesized requires-expression:

constexpr void test_width()
{
    bool invalid_rep{!requires {typename jge::width<T>;
}
}
;
}

vs

constexpr void test_width()
{
    bool invalid_rep{!(requires { typename jge::width<T>; })};
    bool invalid_rep = !(requires { typename jge::width<T>; });
    bool invalid_rep = !requires
    {
        typename jge::width<T>;
    };
}

Thank you @JohelEGP please keep them coming, I'll try and update the patch as soon as I have some fixes, I kind of feel like we are addressing what would have just been downstream bugs anyway. (some are quite bizarre!) so lets squish as many of them as we can before we land this.

My assumption is you are giving me valid c++ code, if so that is great I'll try and address them, but your examples are probably beyond my own concepts knowledge. (This is actually why I started to add this to clang-format, I wanted a way to learn about concepts)

MyDeveloperDay added a comment.EditedJul 7 2020, 12:48 AM

https://reviews.llvm.org/D79773#2131680 has something to do with your .clang-format file, the base LLVM style seems fine.

template <typename T>
concept bool EqualityComparable = requires(T a, T b) {
    { a == b } -> bool;
};

vs

template <typename T>
concept bool EqualityComparable = requires(T a, T b)
{
    {
        a == b
        } -> bool;
};

From what I can tell its because of the "BreakBeforeBraces: Allman or GNU" styles.

I'm wondering what an Allman version should look like?

template <typename T>
concept bool EqualityComparable = requires(T a, T b)
{
    {
        a == b
     } -> bool;
};
JohelEGP added a comment.EditedJul 8 2020, 12:32 AM

Ah, that makes sense. The very definition of Allman is to always break before braces. I suppose I'll need to use BreakBeforeBraces: Custom and BraceWrapping:. I did some testing and noticed that the weird format comes with any of these:

BreakBeforeBraces: Custom
BraceWrapping:
  AfterControlStatement: MultiLine
BreakBeforeBraces: Custom
BraceWrapping:
  AfterFunction: true

Since a compound requirement is neither a control statement nor a function, I suppose it might eventually need its own BraceWrapping nested configuration flag. For now, I'd prefer if they never break.

MyDeveloperDay added a subscriber: klimek.EditedJul 9 2020, 1:54 AM

This issue is caused by the presence of the ; in the requires clause (0)>;) inside the template declaration

template <std::semiregular F, std::semiregular... Args>
    requires std::invocable<F, std::invoke_result_t<Args>...>
struct [[nodiscard]] constant : std::bool_constant < requires
{
    typename _require_constant_<(std::invoke(F{}, Args{}()...), 0)>;
} > {};

These below may not be legal C++ examples but show the same behaviour

struct constant : Foo < typename {
    Foo;
} > {};

struct constant : Foo<typename {Foo}> { };

This is actually failing in the handling of parsing the <> in (parseAngle() actually parseBrace() inside of that), it is not really concepts specific except it may be seen now because of the presence of the ;

The problem comes about I think because the line is broken by the ; during the parseBrace at some point the Next token is null, I assume that's because the lines are broken by the ; as such ultimately the < and > rather than being seen as TT_TemplateOpener and TT_TemplateCloser are seen as TT_BinaryOperators (by default when 'parseAngle' fails)

I'm not sure how I could solve this, I might need help from a higher power @klimek

klimek added inline comments.Jul 9 2020, 4:32 AM
clang/lib/Format/UnwrappedLineParser.cpp
638–643

I'd have expected this to be just in front of line 629 (if... tok::semi) - does that break something? Don't we want to munch the semi after this?

2337

This seems like a very detailed way to parse them; generally, we try to only parse rough structure here.

I found another case. See how everything after the requires-clause is indented for constructors with just the constructor's name (it works otherwise, maybe because it looks like a function).

class [[nodiscard]] data_t
{
public:
    template <std::ranges::input_range R>
        requires std::same_as<std::ranges::range_value_t<R>, flippable_tile_id>
        /*explicit*/ data_t(R&& ids, const format_t f = encoding_t::csv)
          : format_t{f}, tile_ids_{ranges::to_vector(std::forward<R>(ids))}
        {
        }

        [[nodiscard]] bool operator==(const data_t&) const = default;
};

This is actually failing in the handling of parsing the <> in (parseAngle() actually parseBrace() inside of that), it is not really concepts specific except it may be seen now because of the presence of the ;

I opened LLVM 47161 to defer this to upstream.

Rheel added a subscriber: Rheel.Aug 17 2020, 6:47 AM

Could you do a rebase? The patch no longer applies:

error: patch failed: clang/docs/ClangFormatStyleOptions.rst:2711
error: clang/docs/ClangFormatStyleOptions.rst: patch does not apply
error: patch failed: clang/docs/ReleaseNotes.rst:398
error: clang/docs/ReleaseNotes.rst: patch does not apply
error: patch failed: clang/lib/Format/FormatToken.h:470
error: clang/lib/Format/FormatToken.h: patch does not apply
error: patch failed: clang/lib/Format/TokenAnnotator.cpp:1337
error: clang/lib/Format/TokenAnnotator.cpp: patch does not apply

Rebase the patch

njakob added a subscriber: njakob.Oct 25 2020, 8:19 AM

@MyDeveloperDay, is there anything I can do to help you on that?

BTW, libc++ would like to use clang-format (more than it does currently) and having concepts support would be awesome as there are more and more concept-related patches coming in.
IMHO, even support for most common cases would be a great improvement.

I don't think it was far off, I just I agreed with @klimek in trying to address the many different cases that came up I started to feel that parseConstraintExpression was becoming fragile

Having said that I like to think because they are in concepts specific parse code I won't break too much (famous last words!), any if you've tried to format concept code with the current clang-format well good luck with that.

Perhaps something is better than nothing?

if you think it has merits, if it covers most cases ok, then I'd be happy to see it land and work on any refinements from there.

It would be maybe possible to:

  • address @klimek 's comment and do any other necessary cleanup
  • create a bug ticket for what @JohelEGP found with ctors
  • mark clang-format's support of concept as experimental?

This patch is getting big IMO and I really think that something is better than nothing in this case.
And at least we could parallelise the efforts on the remaining (edge) cases when this gets landed.
WDYT?

MyDeveloperDay marked an inline comment as done.

rebase before making any further changes

MyDeveloperDay marked an inline comment as done.Dec 3 2020, 1:12 PM
MyDeveloperDay added inline comments.
clang/lib/Format/UnwrappedLineParser.cpp
2337

So whilst I tend to agree, and I've tried to write this loop to be less and it just didn't seem to catch the cases that had already been given in the unit tests,

This ended up being a lot likes its own parseStructuralElement, I think the difference here is that I've written it as a series of if's rather than a switch statement.

I feel I'd be happier to push the patch through then address individual specific cases

MyDeveloperDay marked an inline comment as done.

Address the issue with still munching the semi
mark as experimental in the release notes

Having reviewed the current status I'm going to leave this patch with just minor changes from where its been for some time, if others feel it has merits in landing as experimental support I'd be happy with that. I would rather build from a base than try and keep rebuilding the same tower.

The concepts support in clang-format is currently poor, actually, it's downright destructive, I've tried to cover the basic concept usage and I'm happy to support this feature against individual bug report where I can.

miscco added a comment.Dec 3 2020, 1:32 PM

As someone who has extensivly worked with conscepts I cannot stress how much this would improve my live

As someone who has extensively worked with concepts I cannot stress how much this would improve my live

and I'd like to think that this will help even in its current form, but it needs someone to accept it first before it can land,

This has been my problem with clang-format , I do try and give some of my time to keeping up with the new and outstanding reviews coming in, but my own progress on clang-format or the fixing of bugs has slowed up because of a lack of people to review, we just need a couple more people who are willing to come once or twice a week and just keep things rolling along. but someone has to "watch the watcher" too.

mitchell-stellar accepted this revision.Dec 4 2020, 6:18 AM

I am in favor of landing this and iterating.

curdeius accepted this revision.Dec 4 2020, 6:27 AM

LGTM.

miscco accepted this revision.Dec 4 2020, 6:56 AM

@JohelEGP can you let me know if you plan to remove the "requested changes" you asked for back in July?

I cannot commit with the revision until that is removed

This revision was not accepted when it landed; it landed in state Needs Review.Dec 4 2020, 9:46 AM
This revision was automatically updated to reflect the committed changes.

I was a bit late, but thanks for everything!

I was a bit late, but thanks for everything!

No problem, sorry not for waiting, but let the games begin.

P.S. I think I may have left a comment for you over in one of your bugs. not sure if you saw that