Skip to content

[Clang] Implement C++26 Attributes for Structured Bindings (P0609R3) #89906

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Apr 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions clang/docs/LanguageExtensions.rst
Original file line number Diff line number Diff line change
Expand Up @@ -1493,6 +1493,7 @@ Conditional ``explicit`` __cpp_conditional_explicit C+
``if consteval`` __cpp_if_consteval C++23 C++20
``static operator()`` __cpp_static_call_operator C++23 C++03
Attributes on Lambda-Expressions C++23 C++11
Attributes on Structured Bindings __cpp_structured_bindings C++26 C++03
``= delete ("should have a reason");`` __cpp_deleted_function C++26 C++03
-------------------------------------------- -------------------------------- ------------- -------------
Designated initializers (N494) C99 C89
Expand Down
2 changes: 2 additions & 0 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,8 @@ C++2c Feature Support

- Implemented `P2573R2: = delete("should have a reason"); <https://wg21.link/P2573R2>`_

- Implemented `P0609R3: Attributes for Structured Bindings <https://wg21.link/P0609R3>`_


Resolutions to C++ Defect Reports
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand Down
2 changes: 1 addition & 1 deletion clang/include/clang/Basic/Attr.td
Original file line number Diff line number Diff line change
Expand Up @@ -3211,7 +3211,7 @@ def ObjCRequiresPropertyDefs : InheritableAttr {
def Unused : InheritableAttr {
let Spellings = [CXX11<"", "maybe_unused", 201603>, GCC<"unused">,
C23<"", "maybe_unused", 202106>];
let Subjects = SubjectList<[Var, ObjCIvar, Type, Enum, EnumConstant, Label,
let Subjects = SubjectList<[Var, Binding, ObjCIvar, Type, Enum, EnumConstant, Label,
Field, ObjCMethod, FunctionLike]>;
let Documentation = [WarnMaybeUnusedDocs];
}
Expand Down
9 changes: 9 additions & 0 deletions clang/include/clang/Basic/DiagnosticParseKinds.td
Original file line number Diff line number Diff line change
Expand Up @@ -478,6 +478,15 @@ def ext_decomp_decl_empty : ExtWarn<
"ISO C++17 does not allow a decomposition group to be empty">,
InGroup<DiagGroup<"empty-decomposition">>;

// C++26 structured bindings
def ext_decl_attrs_on_binding : ExtWarn<
"an attribute specifier sequence attached to a structured binding declaration "
"is a C++2c extension">, InGroup<CXX26>;
def warn_cxx23_compat_decl_attrs_on_binding : Warning<
"an attribute specifier sequence attached to a structured binding declaration "
"is incompatible with C++ standards before C++2c">,
InGroup<CXXPre26Compat>, DefaultIgnore;

/// Objective-C parser diagnostics
def err_expected_minus_or_plus : Error<
"method type specifier must start with '-' or '+'">;
Expand Down
10 changes: 6 additions & 4 deletions clang/include/clang/Sema/DeclSpec.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
#include "llvm/ADT/SmallVector.h"
#include "llvm/Support/Compiler.h"
#include "llvm/Support/ErrorHandling.h"
#include <optional>

namespace clang {
class ASTContext;
Expand Down Expand Up @@ -1790,6 +1791,7 @@ class DecompositionDeclarator {
struct Binding {
IdentifierInfo *Name;
SourceLocation NameLoc;
std::optional<ParsedAttributes> Attrs;
};

private:
Expand Down Expand Up @@ -2339,10 +2341,10 @@ class Declarator {
}

/// Set the decomposition bindings for this declarator.
void
setDecompositionBindings(SourceLocation LSquareLoc,
ArrayRef<DecompositionDeclarator::Binding> Bindings,
SourceLocation RSquareLoc);
void setDecompositionBindings(
SourceLocation LSquareLoc,
MutableArrayRef<DecompositionDeclarator::Binding> Bindings,
SourceLocation RSquareLoc);

/// AddTypeInfo - Add a chunk to this declarator. Also extend the range to
/// EndLoc, which should be the last token of the chunk.
Expand Down
1 change: 1 addition & 0 deletions clang/include/clang/Sema/ParsedAttr.h
Original file line number Diff line number Diff line change
Expand Up @@ -948,6 +948,7 @@ class ParsedAttributes : public ParsedAttributesView {
ParsedAttributes(AttributeFactory &factory) : pool(factory) {}
ParsedAttributes(const ParsedAttributes &) = delete;
ParsedAttributes &operator=(const ParsedAttributes &) = delete;
ParsedAttributes(ParsedAttributes &&G) = default;

AttributePool &getPool() const { return pool; }

Expand Down
4 changes: 3 additions & 1 deletion clang/lib/AST/DeclBase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1115,7 +1115,9 @@ int64_t Decl::getID() const {

const FunctionType *Decl::getFunctionType(bool BlocksToo) const {
QualType Ty;
if (const auto *D = dyn_cast<ValueDecl>(this))
if (const auto *D = dyn_cast<BindingDecl>(this))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems causes the warning. [-Wunused-but-set-variable]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix that shortly

return nullptr;
else if (const auto *D = dyn_cast<ValueDecl>(this))
Ty = D->getType();
else if (const auto *D = dyn_cast<TypedefNameDecl>(this))
Ty = D->getUnderlyingType();
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/Frontend/InitPreprocessor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -704,7 +704,7 @@ static void InitializeCPlusPlusFeatureTestMacros(const LangOptions &LangOpts,
Builder.defineMacro("__cpp_nested_namespace_definitions", "201411L");
Builder.defineMacro("__cpp_variadic_using", "201611L");
Builder.defineMacro("__cpp_aggregate_bases", "201603L");
Builder.defineMacro("__cpp_structured_bindings", "201606L");
Builder.defineMacro("__cpp_structured_bindings", "202403L");
Builder.defineMacro("__cpp_nontype_template_args",
"201411L"); // (not latest)
Builder.defineMacro("__cpp_fold_expressions", "201603L");
Expand Down
41 changes: 31 additions & 10 deletions clang/lib/Parse/ParseDecl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7038,18 +7038,23 @@ void Parser::ParseDirectDeclarator(Declarator &D) {
void Parser::ParseDecompositionDeclarator(Declarator &D) {
assert(Tok.is(tok::l_square));

TentativeParsingAction PA(*this);
BalancedDelimiterTracker T(*this, tok::l_square);
T.consumeOpen();

if (isCXX11AttributeSpecifier())
DiagnoseAndSkipCXX11Attributes();

// If this doesn't look like a structured binding, maybe it's a misplaced
// array declarator.
// FIXME: Consume the l_square first so we don't need extra lookahead for
// this.
if (!(NextToken().is(tok::identifier) &&
GetLookAheadToken(2).isOneOf(tok::comma, tok::r_square)) &&
!(NextToken().is(tok::r_square) &&
GetLookAheadToken(2).isOneOf(tok::equal, tok::l_brace)))
if (!(Tok.is(tok::identifier) &&
NextToken().isOneOf(tok::comma, tok::r_square, tok::kw_alignas,
tok::l_square)) &&
!(Tok.is(tok::r_square) &&
NextToken().isOneOf(tok::equal, tok::l_brace))) {
PA.Revert();
return ParseMisplacedBracketDeclarator(D);

BalancedDelimiterTracker T(*this, tok::l_square);
T.consumeOpen();
}

SmallVector<DecompositionDeclarator::Binding, 32> Bindings;
while (Tok.isNot(tok::r_square)) {
Expand All @@ -7074,13 +7079,27 @@ void Parser::ParseDecompositionDeclarator(Declarator &D) {
}
}

if (isCXX11AttributeSpecifier())
DiagnoseAndSkipCXX11Attributes();

if (Tok.isNot(tok::identifier)) {
Diag(Tok, diag::err_expected) << tok::identifier;
break;
}

Bindings.push_back({Tok.getIdentifierInfo(), Tok.getLocation()});
IdentifierInfo *II = Tok.getIdentifierInfo();
SourceLocation Loc = Tok.getLocation();
ConsumeToken();

ParsedAttributes Attrs(AttrFactory);
if (isCXX11AttributeSpecifier()) {
Diag(Tok, getLangOpts().CPlusPlus26
? diag::warn_cxx23_compat_decl_attrs_on_binding
: diag::ext_decl_attrs_on_binding);
MaybeParseCXX11Attributes(Attrs);
}

Bindings.push_back({II, Loc, std::move(Attrs)});
}

if (Tok.isNot(tok::r_square))
Expand All @@ -7095,6 +7114,8 @@ void Parser::ParseDecompositionDeclarator(Declarator &D) {
T.consumeClose();
}

PA.Commit();

return D.setDecompositionBindings(T.getOpenLocation(), Bindings,
T.getCloseLocation());
}
Expand Down
4 changes: 2 additions & 2 deletions clang/lib/Sema/DeclSpec.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ DeclaratorChunk DeclaratorChunk::getFunction(bool hasProto,

void Declarator::setDecompositionBindings(
SourceLocation LSquareLoc,
ArrayRef<DecompositionDeclarator::Binding> Bindings,
MutableArrayRef<DecompositionDeclarator::Binding> Bindings,
SourceLocation RSquareLoc) {
assert(!hasName() && "declarator given multiple names!");

Expand All @@ -317,7 +317,7 @@ void Declarator::setDecompositionBindings(
new DecompositionDeclarator::Binding[Bindings.size()];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note I'm not super fan of the hidden vector implementation here.
It forces Binding to be default constructible.

BindingGroup.DeleteBindings = true;
}
std::uninitialized_copy(Bindings.begin(), Bindings.end(),
std::uninitialized_move(Bindings.begin(), Bindings.end(),
BindingGroup.Bindings);
}
}
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/Sema/SemaDecl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1974,7 +1974,7 @@ static bool ShouldDiagnoseUnusedDecl(const LangOptions &LangOpts,
// it is, by the bindings' expressions).
bool IsAllPlaceholders = true;
for (const auto *BD : DD->bindings()) {
if (BD->isReferenced())
if (BD->isReferenced() || BD->hasAttr<UnusedAttr>())
return false;
IsAllPlaceholders = IsAllPlaceholders && BD->isPlaceholderVar(LangOpts);
}
Expand Down
2 changes: 2 additions & 0 deletions clang/lib/Sema/SemaDeclCXX.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -910,6 +910,8 @@ Sema::ActOnDecompositionDeclarator(Scope *S, Declarator &D,

auto *BD = BindingDecl::Create(Context, DC, B.NameLoc, VarName);

ProcessDeclAttributeList(S, BD, *B.Attrs);

// Find the shadowed declaration before filtering for scope.
NamedDecl *ShadowedDecl = D.getCXXScopeSpec().isEmpty()
? getShadowedDeclaration(BD, Previous)
Expand Down
2 changes: 1 addition & 1 deletion clang/test/Lexer/cxx-features.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@
#error "wrong value for __cpp_aggregate_bases"
#endif

#if check(structured_bindings, 0, 0, 0, 201606, 201606, 201606, 201606)
#if check(structured_bindings, 0, 0, 0, 202403L, 202403L, 202403L, 202403L)
#error "wrong value for __cpp_structured_bindings"
#endif

Expand Down
63 changes: 54 additions & 9 deletions clang/test/Parser/cxx1z-decomposition.cpp
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// RUN: %clang_cc1 -std=c++17 %s -verify=expected,cxx17 -fcxx-exceptions
// RUN: %clang_cc1 -std=c++2b %s -verify=expected,cxx2b -fcxx-exceptions
// RUN: not %clang_cc1 -std=c++17 %s -emit-llvm-only -fcxx-exceptions
// RUN: %clang_cc1 -std=c++17 %s -triple x86_64-unknown-linux-gnu -verify=expected,cxx17,pre2c -fcxx-exceptions
// RUN: %clang_cc1 -std=c++2b %s -triple x86_64-unknown-linux-gnu -verify=expected,cxx2b,pre2c,post2b -fcxx-exceptions
// RUN: %clang_cc1 -std=c++2c %s -triple x86_64-unknown-linux-gnu -verify=expected,cxx2c,post2b -fcxx-exceptions
// RUN: not %clang_cc1 -std=c++17 %s -triple x86_64-unknown-linux-gnu -emit-llvm-only -fcxx-exceptions

struct S { int a, b, c; };

Expand Down Expand Up @@ -58,7 +59,7 @@ namespace OtherDecl {
namespace GoodSpecifiers {
void f() {
int n[1];
const volatile auto &[a] = n; // cxx2b-warning {{volatile qualifier in structured binding declaration is deprecated}}
const volatile auto &[a] = n; // post2b-warning {{volatile qualifier in structured binding declaration is deprecated}}
}
}

Expand Down Expand Up @@ -97,8 +98,8 @@ namespace BadSpecifiers {
S [a] = s; // expected-error {{cannot be declared with type 'S'}}
decltype(auto) [b] = s; // expected-error {{cannot be declared with type 'decltype(auto)'}}
auto ([c2]) = s; // cxx17-error {{decomposition declaration cannot be declared with parenthese}} \
// cxx2b-error {{use of undeclared identifier 'c2'}} \
// cxx2b-error {{expected body of lambda expression}} \
// post2b-error {{use of undeclared identifier 'c2'}} \
// post2b-error {{expected body of lambda expression}} \

// FIXME: This error is not very good.
auto [d]() = s; // expected-error {{expected ';'}} expected-error {{expected expression}}
Expand All @@ -119,9 +120,6 @@ namespace BadSpecifiers {
[[]] auto [ok_3] = s;
alignas(S) auto [ok_4] = s;

// ... but not after the identifier or declarator.
// FIXME: These errors are not very good.
auto [bad_attr_1 [[]]] = s; // expected-error {{attribute list cannot appear here}} expected-error 2{{}}
auto [bad_attr_2] [[]] = s; // expected-error {{expected ';'}} expected-error {{}}
}
}
Expand Down Expand Up @@ -156,3 +154,50 @@ namespace Init {
S [goodish4] { 4 }; // expected-error {{cannot be declared with type 'S'}}
}
}


namespace attributes {

struct S{
int a;
int b = 0;
};

void err() {
auto [[]] = S{0}; // expected-error {{expected unqualified-id}}
auto [ alignas(42) a, foo ] = S{0}; // expected-error {{an attribute list cannot appear here}}
auto [ c, [[]] d ] = S{0}; // expected-error {{an attribute list cannot appear here}}
auto [ e, alignas(42) f ] = S{0}; // expected-error {{an attribute list cannot appear here}}
}

void ok() {
auto [ a alignas(42) [[]], b alignas(42) [[]]] = S{0}; // expected-error 2{{'alignas' attribute only applies to variables, data members and tag types}} \
// pre2c-warning 2{{an attribute specifier sequence attached to a structured binding declaration is a C++2c extension}}
auto [ c [[]] alignas(42), d [[]] alignas(42) [[]]] = S{0}; // expected-error 2{{'alignas' attribute only applies to variables, data members and tag types}} \
// pre2c-warning 2{{an attribute specifier sequence attached to a structured binding declaration is a C++2c extension}}
}


auto [G1 [[deprecated]], G2 [[deprecated]]] = S{42}; // #deprecated-here
// pre2c-warning@-1 2{{an attribute specifier sequence attached to a structured binding declaration is a C++2c extension}}

int test() {
return G1 + G2; // expected-warning {{'G1' is deprecated}} expected-note@#deprecated-here {{here}} \
// expected-warning {{'G2' is deprecated}} expected-note@#deprecated-here {{here}}
}

void invalid_attributes() {
// pre2c-warning@+1 {{an attribute specifier sequence attached to a structured binding declaration is a C++2c extension}}
auto [a alignas(42) // expected-error {{'alignas' attribute only applies to variables, data members and tag types}}
[[assume(true), // expected-error {{'assume' attribute cannot be applied to a declaration}}
carries_dependency, // expected-error {{'carries_dependency' attribute only applies to parameters, Objective-C methods, and functions}}
fallthrough, // expected-error {{'fallthrough' attribute cannot be applied to a declaration}}
likely, // expected-error {{'likely' attribute cannot be applied to a declaration}}
unlikely, // expected-error {{'unlikely' attribute cannot be applied to a declaration}}
nodiscard, // expected-warning {{'nodiscard' attribute only applies to Objective-C methods, enums, structs, unions, classes, functions, function pointers, and typedefs}}
noreturn, // expected-error {{'noreturn' attribute only applies to functions}}
no_unique_address]], // expected-error {{'no_unique_address' attribute only applies to non-bit-field non-static data members}}
b] = S{0};
}

}
12 changes: 11 additions & 1 deletion clang/test/SemaCXX/unused.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -102,11 +102,21 @@ namespace PR33839 {
for (auto [x] : a) { // expected-warning {{unused variable '[x]'}}
}
}
void use() {
void use() {
f<int>(); // expected-note {{instantiation of}}
g<true>();
g<false>();
h<int>(); // expected-note {{instantiation of}}
}
}

namespace maybe_unused_binding {

void test() {
struct X { int a, b; } x;
auto [a [[maybe_unused]], b] = x; // expected-warning {{an attribute specifier sequence attached to a structured binding declaration is a C++2c extension}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any standard attributes other than this that make sense on SB's? If not, I'd like all of the standards ones tested to show what the behavior is (and 'not valid here' type errors are totally acceptable).

[[indeterminate]] seems useful, but the rest should likely result in a rejection.

Additionally, we should do an audit of ALL our "C++" spelling attributes to see which make sense here and to make sure they are treated reasonably. I'm not asking to do that HERE, but a bug in our bug tracker (perhaps with a 'good starter bug' tag, particularly if we list ALL our attributes that need auditing) would be acceptable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any standard attributes other than this that make sense on SB's? If not, I'd like all of the standards ones tested to show what the behavior is (and 'not valid here' type errors are totally acceptable).

No, I can add tests

[[indeterminate]] seems useful, but the rest should likely result in a rejection.

We do not have this one

Additionally, we should do an audit of ALL our "C++" spelling attributes to see which make sense here and to make sure they are treated reasonably. I'm not asking to do that HERE, but a bug in our bug tracker (perhaps with a 'good starter bug' tag, particularly if we list ALL our attributes that need auditing) would be acceptable.

Yes, I can create an issue once we land that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done
I created an issue to allow the deprecated attribute cplusplus/CWG#530
(We now support that as an extension , and it would be challenging not to)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a test to check the [[maybe_unused]] takes effect.
e.g. checks whether there has warning: unused variable '[a, b]' [-Wunused-variable].

I've a question about this test case. We only add [[maybe_unused]] to a , IIUC, should we also emit a warning that unused variable 'b'?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is that test.
By marking one of the member unused, we don't warn on any of the member.
This perhaps not perfect but the standard already specified that if one of the member is used, we should not warn on any of the biding

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree!

}

}

#endif
2 changes: 1 addition & 1 deletion clang/www/cxx_status.html
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ <h2 id="cxx26">C++2c implementation status</h2>
<tr>
<td>Trivial infinite loops are not Undefined Behavior</td>
<td><a href="https://wg21.link/P2809R3">P2809R3</a> (<a href="#dr">DR</a>)</td>
<td class="none" align="center">No</td>
<td class="unreleased" align="center">Clang 19</td>
Comment on lines 188 to +190
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the wrong paper was marked as implemented

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix that shortly

</tr>
<tr>
<td>Erroneous behaviour for uninitialized reads</td>
Expand Down
Loading