Page MenuHomePhabricator

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

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

Details

Summary

Add two new attributes for struct initialization: 'requires_designator' and 'requires_init'. The first attribute attaches to a struct, class, or union 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.

requires_designator
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 requires_designator 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; // 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 'requires_designator' attribute here
struct [[clang::requires_designator]] Foo {
1 error generated.

requires_init
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::requires_init]] 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 'requires_init' attribute here
   [[clang::requires_init]] 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.Jul 9 2019, 5:07 PM
clang/test/SemaCXX/attr-designated-init-required.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.Jul 9 2019, 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.Jul 10 2019, 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
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.

3 ↗(On Diff #208829)

Same concerns here as above.

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

Addressed several of the comments regarding naming and style.

emmettneyman added inline comments.Jul 10 2019, 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.Jul 10 2019, 4:54 PM

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

aaron.ballman added inline comments.Jul 11 2019, 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.Jul 11 2019, 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.Jul 11 2019, 10:39 AM

Small changes: adding auto

emmettneyman marked 3 inline comments as done.Jul 11 2019, 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.Jul 11 2019, 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.Jul 11 2019, 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.Jul 16 2019, 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.Jul 18 2019, 12:15 PM

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

emmettneyman edited the summary of this revision. (Show Details)Jul 25 2019, 1:00 PM

Thanks for addressing the feedback @emmettneyman , LGTM, deferring to other reviewers for final call.

aaron.ballman added inline comments.Jul 26 2019, 6:25 AM
clang/include/clang/Basic/Attr.td
1948

Why does this have a Clang spelling in addition to the GCC spelling? I think it only needs the GCC spelling.

clang/include/clang/Basic/AttrDocs.td
1454–1455

Please show the struct declaration itself as part of the code block.

1464–1466

Same here.

1484–1485

Why is it valuable to differ from GCC's behavior in this regard?

1487

Given that it can be added to class types, I wonder what the behavior should be for code like:

struct Foo {
  int a = 1;
  int b, c;
  int d = 4;
};

Foo foo = {...};

Does the initializer for foo have to specify .a and .d?

1498

Attribute name seems stale here.

clang/include/clang/Basic/DiagnosticGroups.td
1069

Missing a full stop at the end of the comment.

clang/include/clang/Basic/DiagnosticSemaKinds.td
3537

This attribute spelling seems wrong, it should be designated_init, no?

clang/lib/Sema/SemaDecl.cpp
11214

You can drop the call to getTypePtr() and just use the overloaded operator->() instead.

11216

Attribute spelling is stale in this comment.

15314

Attribute name is stale in this comment.

This is the wrong place to do this work -- it should be done from SemaDeclAttr.cpp when processing the attribute. We should avoid adding the attribute in the first place rather than adding the attribute and then later removing it.

15318–15321

HasNonPublicMember = llvm::any_of(RD->fields(), [](const FieldDecl *FD) { return FD->getAccess() != AS_public; });

clang/lib/Sema/SemaDeclAttr.cpp
5313

No need for this, you can just use cast<> directly, as the subject was already checked by the common attribute handling code.

I would probably rewrite this as:

if (cast<FieldDecl>(D)->getParent()->isUnion()) {
  S.Diag(...);
  return;
}

D->addAttr(...);
5314

Do not use auto here as the type is not spelled out in the initialization.

emmettneyman marked 16 inline comments as done.Jul 26 2019, 12:41 PM
emmettneyman added inline comments.
clang/include/clang/Basic/Attr.td
1948

I kept the Clang spelling so that there's naming consistency between the two attributes (requires_designator and requires_init). I can remove it if it's redundant/unnecessary, let me know.

clang/include/clang/Basic/AttrDocs.td
1484–1485

The motivation behind GCC's attribute seems to be to avoid using positional arguments:

The intent of this attribute is to allow the programmer to indicate that a structure’s layout may change, and that therefore relying on positional initialization will result in future breakage.

The motivation behind this attribute is a little bit different. Instead of avoiding the use of positional arguments, the goal of this attribute is to bring ObjC-style named parameters to C/C++ and to be able to enforce their use in certain situations. In line with this goal, we wanted to forbid the following pattern:

Foo foo;
foo.x = 42;
foo.y = 36;

That's why this attribute enforces that all declarations be brace-initialized.

1487

The requires_designator attribute doesn't require that any of the fields have to be specified, only that designated initializer syntax has to be used. So, for the struct definition above, any of the following are valid:

Foo foo {};
Foo foo {.a = 1, .b = 2, .c = 3, .d = 4};
Foo foo {.b = 2, .c = 3};

Any of the fields can be left out and default-initialized. I've added this example to the lit tests for the attribute.

clang/include/clang/Basic/DiagnosticSemaKinds.td
3537

Does it make more sense to use designated_init or requires_designator as the primary spelling for this attribute? I chose to use the requires_designator spelling so that the two attributes have consistent and similar spellings. Let me know if you think it should be the other way around (or if you want the Clang spelling removed entirely).

clang/lib/Sema/SemaDecl.cpp
11216

See comments above about keeping this spelling.

15314

See question above regarding attribute spelling.

I thought about this a lot because I agree, it feels wrong to remove the attribute once it is already added to a field, struct, etc. However I could not think of any other way or any other place to do this check. Since all fields of the struct need to have been processed already in order to check whether there are any non-public members, it seems like the only place to do this check is at the end of handling the entire TagDecl. When the struct-level and field-level attributes are attached, we don't yet have information about the access modifiers of all the members.

emmettneyman marked 6 inline comments as done.

Addressed comments and feedback

aaron.ballman added inline comments.Aug 1 2019, 12:44 PM
clang/include/clang/Basic/Attr.td
1948

This is tricky. I don't see any value in [[clang::requires_designator]] in C++ mode because [[gnu::designated_init]] suffices, so I'd kind of prefer to see the Clang spelling go away. This attribute can also be used in C with __attribute__((designated_init)) which is great, but I'd also like to see it be made available in C with double square bracket syntax (supported in C2x or via a feature flag). However, there is no [[gnu::designated_init]] supported by GCC, so it would be an abuse of the gnu namespace to add it there. Having a Clang spelling gets around that issue. That said, I suspect GCC will support C2x attributes at some point, so I would expect this attribute to be supported there eventually, so the issue remains.

I think I've convinced myself that we should just drop the Clang spelling and wait to handle the C2x square bracket attribute until later, since C users can still use the GNU spelling. You should also rename the attribute DesignatedInit and change the other places with the older spelling.

clang/include/clang/Basic/AttrDocs.td
1484–1485

I may still be missing something, but your rationale feels a bit more like a style-based choice than preventing common mistakes that are hard to track down; I find the GCC rationale to be more compelling. What do you think about matching the GCC behavior in the compiler warning, but adding the additional restrictions you'd like to see as a clang-tidy check?

1487

So if I had a structure declaration like:

struct S {
  int a = 1;
  int b = 2;
};

S s;

This would fail because s is not initialized with curly braces? I don't think that's a particularly good behavior for C++.

clang/include/clang/Basic/DiagnosticSemaKinds.td
3537

This is a bit moot since I think we'll want to drop the secondary spelling, but a better approach would be to use %0 and pass in the Attr object directly so that we get the name used by the user instead of a static attribute name.