Page MenuHomePhabricator

Add 'require_designated_init' and 'required' attribute to clang
Needs ReviewPublic

Authored by emmettneyman on Mon, Jul 8, 4:27 PM.

Details

Summary

Add two new attributes for struct initialization: 'require_designated_init' and 'designated_init_required'. The first attribute attaches to a struct or class and specifies that whenever a new variable of that type is declared, designated initializer syntax must be used. The second attribute attaches to a specific field and requires that that field must be explicitly initialized with a value whenever a new instance of the struct is declared.

require_designated_init
For a struct Foo with one integer field x, the following declarations are valid and invalid under the require_designated_init attribute.

Foo foo {.x = 42}; // valid
Foo foo {}; // valid
Foo foo {42}; // invalid
Foo foo; // invalid

For a struct Foo with multiple integer fields, the following declarations are valid and invalid under the require_designated_init attribute.

Foo foo {.x = 42, .y = 21, .z = 1}; // valid
Foo foo {.z = 1, .x = 42, .y = 21}; // valid
Foo foo {.x = 42, .z = 1}; // valid
Foo foo {}; // valid
Foo foo {42, 21, 1}; // invalid
Foo foo {.x = 42, 21, 1}; // invalid
Foo foo; // invalid

When the source code violates the attribute, an error is printed like this:

$ clang TestSingleFieldBad.cpp
TestSingleFieldBad.cpp:6:14: error: struct variable declaration does not use designated initializer syntax
    Foo foo {10};
TestSingleFieldBad.cpp:1:23: note: required by 'require_designated_init' attribute here
struct [[clang::require_designated_init]] Foo {
1 error generated.

designated_init_required
For each field that is marked with the required attribute, that field must be explicitly initialized using designated initializer syntax.
For example, if we have the following struct:

struct Foo {
  [[clang::designated_init_required]] int x;
};

you must initialize the struct like this:

Foo foo {.x = 1};

The following declarations are all invalid:

Foo f1;
Foo f2 {};
Foo f3 {1};

When an invalid program is compiled, you'll get an error like this:

$ clang TestSimpleBad.cpp
TestSimpleBad.cpp:6:11: error: initializer for variable foo must explicitly initialize field x
  Foo foo {};
TestSimpleBad.cpp:2:18: note: enforced by 'designated_init_required' attribute here
   [[clang::designated_init_required]] int x;
1 error generated.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
compnerd added inline comments.Mon, Jul 8, 5:56 PM
clang/lib/Sema/SemaDecl.cpp
11214

I would just use auto, you spell out the type in the expression.

11218

TD && is tautologically true. Please drop it.

11219

I would use auto as you spell out the type in the expression.

11223

I would just combine this with the previous like and unindent the rest of the path by using the following:

if (!isa<DesignatedInitExpr>(init))
  continue;
11238

A new line before this line would be nice.

11247

Any reason that this cannot be const auto *FD?

emmettneyman marked 6 inline comments as done.

Small changes in response to feedback

emmettneyman planned changes to this revision.Tue, Jul 9, 11:49 AM
emmettneyman marked 3 inline comments as done.

Changed attribute spelling from GNU to CXX11 and renamed the 'required' attribute to 'designated_init_required'

emmettneyman edited the summary of this revision. (Show Details)Tue, Jul 9, 2:40 PM
emmettneyman edited the summary of this revision. (Show Details)Tue, Jul 9, 2:43 PM
emmettneyman edited the summary of this revision. (Show Details)Tue, Jul 9, 2:49 PM

Updated tests and diagnostic messages with new attribute spelling

emmettneyman marked an inline comment as done.

Remove cppreference link, add reference to C++ spec.

rsmith added a comment.Tue, Jul 9, 4:47 PM

C++20 designated initializers don't permit mixing designated fields with non-designated ones, so some of the examples here are invalid. However, I think we should be looking for an attribute design that works well in both C and C++, and with the various Clang extensions that permit the full generality of C designated initializers in other language modes. I also think this patch is combining multiple features that would be useful to expose separately. To that end, I think something like this might make sense:

  • An attribute that can be applied to either a field or to a struct that requires a designator to be used on any initializer for that field (and for a struct, is equivalent to specifying the attribute on all fields)
  • An attribute that can be applied to either a field or to a struct that requires an explicit initializer to be used for that field, for both aggregate initialization and constructor mem-initializer lists (and for a struct, is equivalent to specifying the attribute on all fields with no default member initializers)

Maybe requires_designator and requires_init or similar?

And I think these attributes should be made available in both C++11 attribute syntax and GNU attribute syntax. Inventing a C++-only extension to improve support for designated initializers doesn't seem like the right choice to me.

slight change to specification reference format

I don't see any cases where [[clang::required]] is tested, am I missing something?

clang/test/SemaCXX/attr-designated-init-required.cpp
3 ↗(On Diff #208829)

Why the macro?

clang/test/SemaCXX/attr-require-designated-init.cpp
3 ↗(On Diff #208829)

Why the macro?

I don't see any cases where [[clang::required]] is tested, am I missing something?

I renamed the attribute at your suggestion. It's now called [[clang::designated_init_required].

emmettneyman marked 2 inline comments as done.Tue, Jul 9, 5:35 PM
emmettneyman added inline comments.
clang/test/SemaCXX/attr-designated-init-required.cpp
3 ↗(On Diff #208829)

I modeled this file after test/SemaCXX/attr-require-constant-initialization.cpp where a macro is defined at the top. I assume just to keep things neat.

clang/test/SemaCXX/attr-require-designated-init.cpp
3 ↗(On Diff #208829)

See above.

aaron.ballman added inline comments.Wed, Jul 10, 7:19 AM
clang/include/clang/Basic/Attr.td
1947

This should be RequiresDesignator.

1948

This should use the Clang attribute spelling rather than CXX11.

1948

I'd prefer this be named requires_designator.

1949

I'm a bit confused by this subject -- it's a declaration attribute that appertains to types? Why is this not RecordDecl instead?

1953

This should be RequiresInit

1954

This should also use the Clang spelling. Also, I'd prefer this be named requires_init.

As it stands, these two attribute names are *way* too similar.

clang/include/clang/Basic/AttrDocs.td
1448

The behavior should be documented as to what happens when you apply these attributes to unions.

1451

anytime -> any time

1452

struct's -> structure's

1484

anytime -> any time
struct/class -> struct

clang/include/clang/Basic/DiagnosticSemaKinds.td
3533–3541

I'm a little uncomfortable with these being errors rather than warnings. I think of these attributes as being preferences rather than requirements; the code isn't *wrong* if it fails to use the designated initializer (it's conforming to C or C++, does not have UB, etc) and other implementations are free to ignore those attributes and the end result will be identical to what's produced by Clang.

What do you think about turning these into warnings? Users can always use -Werror to strengthen their own requirements.

clang/lib/Sema/SemaDecl.cpp
11214

Any reason this shouldn't be const auto *? (Propagated elsewhere.)

11220–11221

You can use a range-based for loop over inits(), I believe. Also, the variable should be named Init to meet the coding style.

11228

Rather than use hasAttr<> and getAttr<> like this, you should only use getAttr<> above. e.g., if (const auto *RAttr = TD->getAttr<RequireDesignatedInitAttr>()).

11255

You can use const auto * here.

11256–11257

inits() and Init, as above.

11258

const auto *

11261

Name per coding styles.

11270

const auto *

11276

const auto *A (naming conventions and not conflicting with a type name).

11628

const auto *

11631–11634

Don't do hasAttr<> followed by getAttr<>, also watch the naming conventions.

11645

const auto *

11646

const auto *

11647–11650

hasAttr<> followed by getAttr<>, as above.

clang/lib/Sema/SemaDeclAttr.cpp
5312–5327

Currently, both of these functions aren't needed (once you fix the code in Attr.td) because the checking is handled automatically for you. You can use handleSimpleAttribute() instead, from within the switch below.

clang/test/Misc/pragma-attribute-supported-attributes-list.test
125

I think you're missing the changes for the other attribute.

clang/test/SemaCXX/attr-designated-init-required.cpp
3 ↗(On Diff #208829)

FWIW, I prefer seeing the attribute used explicitly. The macro is more of a distraction.

63 ↗(On Diff #208842)

What should happen in these sort of situations?

class C {
  ATTR int x; // This field is private
};

struct S {
  ATTR int x;
};

struct D : private S { // S::x is private when referenced through D
};

If we're ignoring the attribute, we should diagnose it as being ignored. But I'm also not certain how friendship should interact with this.

clang/test/SemaCXX/attr-require-designated-init.cpp
3 ↗(On Diff #208829)

Same concerns here as above.

104 ↗(On Diff #208842)

I think this should ignore fields that cannot be initialized, as in:

class C {
  int a;
protected:
  int b;
public:
  int c;
};

C c{.c = 12}; // Don't complain about a or b.

I'd also like to see more involved situations, like classes with inherited fields, privately inherited classes, etc. Also, some tests with unions would be appreciated as well.

mattd added a subscriber: mattd.Wed, Jul 10, 11:44 AM
emmettneyman planned changes to this revision.Wed, Jul 10, 12:30 PM
emmettneyman marked 32 inline comments as done.

Addressed several of the comments regarding naming and style.

emmettneyman added inline comments.Wed, Jul 10, 4:50 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
3533–3541

That makes sense, I'll make the change.

emmettneyman planned changes to this revision.Wed, Jul 10, 4:54 PM

Working on adding more information to the documentation and adding more in-depth unit tests.

aaron.ballman added inline comments.Thu, Jul 11, 8:57 AM
clang/include/clang/Basic/Attr.td
1948

Hmm, after making this suggestion, I noticed that GCC seems to support a similar attribute named designated_init (https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html#Common-Type-Attributes). Was your goal to support the same thing GCC supported?

clang/include/clang/Basic/DiagnosticSemaKinds.td
3534

These new warnings should be controlled under a diagnostic group. I would recommend designated-init because that's what's used by GCC for their attribute, but that's only because I am assuming you want your attribute to match their behavior.

clang/lib/Sema/SemaDecl.cpp
11220

const auto * here as well?

11221

You don't use DIE -- this should switch to an isa<> instead.

11254

const auto * here and below?

compnerd added inline comments.Thu, Jul 11, 9:49 AM
clang/include/clang/Basic/Attr.td
1948

designated_init is a suggestion, the original patch seemed to be a stronger version. I think that we should be supporting the GNU spelling for designated_init and can support a Clang spelling of requires_designated_init if the goal is to have the stronger guarantee that this *must* happen.

emmettneyman marked 3 inline comments as done.Thu, Jul 11, 10:39 AM

Small changes: adding auto

emmettneyman marked 3 inline comments as done.Thu, Jul 11, 11:17 AM
emmettneyman added inline comments.
clang/include/clang/Basic/Attr.td
1948

I hadn't known about the GCC attribute until now. Yes, the requires_designator (originally require_designated_init) attribute wants to enforce the same thing I believe. I couldn't find more documentation for the GCC designated_init attribute so it's a little tough to tell whether the behavior is the exact same. The attribute in this patch allows a field to be default constructed (unless the other attribute is applied to that specific field) but enforces that a brace initializer must be used. So Foo foo {}; would be valid (every field is default constructed) but Foo foo; would not be valid. I'm not sure if that's the same behavior the GCC attribute is trying to enforce. But on a high level, both are trying to prohibit using positional args when declaring a struct.

@compnerd I don't mind this attribute generating warnings rather than errors. It's ok for this attribute to be a "suggestion" as well. Like @aaron.ballman mentioned, "users can always use -Werror to strengthen their own requirements."

aaron.ballman added inline comments.Thu, Jul 11, 2:02 PM
clang/include/clang/Basic/Attr.td
1948

I think the correct approach then is: add this attribute under the name designated_init with the GCC spelling kind and match GCC's behavior for it (or, if we don't want to match GCC's behavior, note where we differ and why). We should put its diagnostics under a -Wdesignated-init warning group.

The next question is: are you interested in finding out whether GCC would be willing to implement the requires_init attribute? If GCC is interested in picking it up, then we can name it with the GCC spelling as well. If the GCC folks are not interested in picking up the attribute, then we should probably leave it with the Clang spelling. I'd recommend we put the diagnostics into the -Wdesignated-init warning group as well.

I think both of these attributes should also be available in C2x mode as well, but that can probably be done in a follow-up patch as it will involve tablegen changes and opens up other questions (i.e., does GCC handle C2x attributes yet, and if they do, are they making designated_init available in that mode?).

emmettneyman marked 2 inline comments as done.

Added documentation about using the attributes with unions.

emmettneyman added inline comments.Thu, Jul 11, 4:15 PM
clang/include/clang/Basic/AttrDocs.td
1448

For now, adding a requires_init attribute to a field of a union will result in a warning that the attribute is ignored. In the future, I might submit another patch in the future adding behavior for this attribute for fields of a union.

emmettneyman planned changes to this revision.Tue, Jul 16, 1:21 PM

Changes planned:

  • Move diagnostics into a diagnostic group.
  • Add behavior and test cases for private members of structs/classes.
  • Investigate behavior of GCC designated_init attribute
emmettneyman marked an inline comment as done.Thu, Jul 18, 12:15 PM

Created diagnostic group, added behavior and documentation for private fields, added documentation regarding GCC attribute