Objective-C++11: Support static_assert() in @interface/@implementation ivar lists and method declarations

[thakis]

Summary

This adds support for static_assert() (and _Static_assert()) in @interface/@implementation ivar lists and in @interface method declarations.

It was already supported in @implementation blocks outside of the ivar lists.

The assert AST nodes are added at file scoped, matching where other (non-Objective-C) declarations at @interface / @implementation level go (cf allTUVariables).

Also add a __has_feature(objc_c_static_assert) that's true in C11 (and __has_extension(objc_c_static_assert) that's always true) and __has_feature(objc_cxx_static_assert) that's true in C++11 modeafter this patch, so it's possible to check if this is supported.

[thakis]

(Since jfb added a few people who might not have seen it: This was discussed in "Objective-C++11: concerns about adding static_assert support to @interface / @implementation?" on cfe-dev)

[aaron.ballman]

clang/lib/Parse/ParseDecl.cpp

3892–3893 ↗

(On Diff #190123)

Seems to be unrelated to this patch? Feel free to commit separately.

clang/test/Parser/objc-static-assert.mm

1 ↗

(On Diff #190123)

Can you try explicitly specifying C++98 as the underlying language standard mode? I feel like _Static_assert() will continue to work there (because we made it a language extension in all modes) but static_assert() may fail (because that's gated on C++11 support). If that turns out to be the case, then I think objc_static_assert should be more complex than expanding to true, or we should talk about supporting static_assert() in all C++ language modes like we do for _Static_assert().

[thakis]

clang/lib/Parse/ParseDecl.cpp

3892–3893 ↗

(On Diff #190123)

It's somewhat related in that ParseStructDeclaration() is called a few lines below where I'm adding the comment talking about ParseStructUnionBody()

clang/test/Parser/objc-static-assert.mm

1 ↗

(On Diff #190123)

Correct, with -std=c++98 static_assert isn't available but _Static_assert still is. If you want, I can add a test for this, but this is covered by regular c++ tests already.

I think the has_feature() should stay as-is though: Else you have no way to know if _Static_assert works in obj-c mode, and you can check if static_assert works by checkout has_feature && __cplusplus >= 201103L if you still care about c++98.

(And adding one feature each for static_assert and _Static_assert seems like overkill.)

[aaron.ballman]

clang/test/Parser/objc-static-assert.mm

1 ↗

(On Diff #190123)

Correct, with -std=c++98 static_assert isn't available but _Static_assert still is. If you want, I can add a test for this, but this is covered by regular c++ tests already.

Please do (if we don't change the availability of static_assert()), because the test currently relies on the unwritten -std option in order to pass.

I think the has_feature() should stay as-is though: Else you have no way to know if _Static_assert works in obj-c mode, and you can check if static_assert works by checkout has_feature && __cplusplus >= 201103L if you still care about c++98.

I don't think this is the correct approach. Testing for static_assert() support should not leave the user guessing at what the correct spelling is.

(And adding one feature each for static_assert and _Static_assert seems like overkill.)

Definitely agreed there.

I think the correct way forward is to support static_assert() in all language modes like we already do for _Static_assert(), then objc_static_assert becomes useful as-is. I cannot think of a reason why we would want to allow _Static_assert() in C++ but not allow static_assert().

[thakis]

test c++98

clang/test/Parser/objc-static-assert.mm

1 ↗

(On Diff #190123)

I updated the test.

Accepting static_assert() independent of language mode seems pretty unrelated to this patch here, so I don't want to do this.

If you don't like the current has_feature approach, I'm all ears for other approaches. The current approach allows you to detect if clang can handle static_assert in objc objects, and codebases that still care about c++98 will have a static_assert wrapping macro keyed off __cplusplus already, so that part will transparently just work as well. And codebases that are c++11 and newer are in a good position too. I think the current setup is pretty good. (But I'm happy to hear better suggestions.)

[erik.pilkington]

clang/test/Parser/objc-static-assert.mm

1 ↗

(On Diff #190123)

Accepting static_assert() independent of language mode seems pretty unrelated to this patch here, so I don't want to do this.

Yeah, we shouldn't be treating static_assert as a keyword in C++98 or C, I think. It would break code.

If you don't like the current has_feature approach, I'm all ears for other approaches. The current approach allows you to detect if clang can handle static_assert in objc objects, and codebases that still care about c++98 will have a static_assert wrapping macro keyed off __cplusplus already, so that part will transparently just work as well. And codebases that are c++11 and newer are in a good position too. I think the current setup is pretty good. (But I'm happy to hear better suggestions.)

This is pretty weird. This feature flag doesn't actually correspond to any feature, just the possibility of the existence of a feature (there isn't any way you could use this __has_feature portably without also including another __has_feature check). I think the most internally consistent way of doing this is to have two flags, as you mentioned above, objc_c_static_assert and objc_cxx_static_assert.

Just to keep the bikeshed going, maybe it should be spelt objc_interface_c_static_assert or something, to show that it doesn't control static_assert in ObjC, but static_assert in interfaces in ObjC.

[aaron.ballman]

clang/test/Parser/objc-static-assert.mm

1 ↗

(On Diff #190123)

Yeah, we shouldn't be treating static_assert as a keyword in C++98 or C, I think. It would break code.

That depends on how we implemented the feature (we could parse the token as an identifier and check the spelling in situations where static_assert() can grammatically appear, for instance). I do have a hunch that this should be possible to support, though it's nontrivial and I don't expect @thakis to do it as part of this feature.

I think the most internally consistent way of doing this is to have two flags, as you mentioned above, objc_c_static_assert and objc_cxx_static_assert.

Yeah, as much as I didn't like the idea at first blush, I'm starting to think it's the best way forward. I don't want to ever explain why __has_feature(objc_static_assert) return true with -std=c++98 and yet static_assert(true, ""); fails to compile while _Static_assert(true, "") succeeds. That's inexplicable behavior, IMO. Having two feature test macros alleviates that concern.

Just to keep the bikeshed going, maybe it should be spelt objc_interface_c_static_assert or something, to show that it doesn't control static_assert in ObjC, but static_assert in interfaces in ObjC.

That seems reasonable to me.

[thakis]

clang/test/Parser/objc-static-assert.mm

1 ↗

(On Diff #190123)

One last try to make a case for this patch as-is:

This is pretty weird. This feature flag doesn't actually correspond to any feature, just the possibility of the existence of a feature (there isn't any way you could use this has_feature portably without also including another has_feature check).

You can if you assume C++11, which most people probably do. And people who don't either use Objective-C (without ++) where _Static_assert always works, or use something like

#if __cplusplus >= 201103L
#define STATIC_ASSERT(x) static_assert(x)
#else
#define STATIC_ASSERT(x) static_assert(x)
#endif

For all three cases,

@interface Foo {
#if __has_feature(objc_static_assert)
  STATIC_ASSERT(...)
#endif
}

will do the right thing (with STATIC_ASSERT either being static_assert for folks who target C++11 and never, _Static_assert() (or static_assert + include assert.h) for Objective-C without ++, or literally STATIC_ASSERT with the macro above for people who target c++98 and c++11.

I don't think there's anything strange about this, and it allows having just a single feature check.

I don't feel super strongly about this, so if this still doesn't sway y'all I'll go with two features.

[thakis]

two features

[thakis]

Seems like this wasn't convincing, so now with two separate feature flags. I went with the shorter name, since adding "interface_" isn't just longer but also misleading, since this also works in @implementation ivar blocks now.

[erik.pilkington]

LGTM, after one more comment in Features.def :)

clang/include/clang/Basic/Features.def

121 ↗

(On Diff #190418)

We should only claim _Static_assert is a feature in languages where its actually a formal language feature (C11), you can also use EXTENSION to indicate what languages we accept it in (see the comment in the file header).

[thakis]

c89

[thakis]

clang/include/clang/Basic/Features.def

121 ↗

(On Diff #190418)

Sorry, I don't follow. clang accepts _Static_assert in C89. You're saying you want __has_feature(objc_c_static_assert) to be false there? I'm guessing because -pedantic warns on it?

But if we do this, there's no way to differentiate clang-in-std=c89 mode with this patch from clang-without-this-patch, is there? Is that what you want? i.e. replace true with LangOpts.C11?

(For now, I updated the test to check the __has_feature() is true in c89 too.)

(That seems like another argument to restore the single feature to me since this working in objc is orthogonal on if the language version supports static asserts.)

[erik.pilkington]

clang/include/clang/Basic/Features.def

121 ↗

(On Diff #190418)

Sorry, I don't follow. clang accepts _Static_assert in C89. You're saying you want __has_feature(objc_c_static_assert) to be false there? I'm guessing because -pedantic warns on it?

Yeah, __has_feature is meant to check for standard language features. __has_extension checks whether clang will accept it, regardless of what respective standard says. So _Static_assert here is a "standard" language feature in ObjC + C11 and an extension in ObjC + C99. I'm suggesting you write this like:

FEATURE(objc_c_static_assert, LangOpts.C11)
FEATURE(objc_cxx_static_assert, LangOpts.CPlusPlus11)
EXTENSION(objc_c_static_assert, true)

Just like we do now for c_static_assert in general:

FEATURE(c_static_assert, LangOpts.C11)
EXTENSION(c_static_assert, true)

That way, __has_extension(objc_c_static_assert) is always true (with a new compiler), and __has_feature only deals with standard language features. Does that make sense?

[thakis]

extension

[thakis]

Ah, got it, thanks for explaining!

[aaron.ballman]

clang/test/Parser/objc-static-assert.m

29 ↗

(On Diff #190534)

Given that static assertions are member declarations of a struct, should this "replay" the static asserts from the interface (including failing the assertions)?

clang/test/Parser/objc-static-assert.mm

4 ↗

(On Diff #190534)

Why do we expect objc_c_static_assert to be a supported feature in ObjC++? I would have expected this as an extension, but not a feature.

[thakis]

clang/test/Parser/objc-static-assert.m

29 ↗

(On Diff #190534)

This only works in the old fragile abi and is unsupported in "modern" (8 year old) objc, and it was used very rarely even back then. I don't think we need to do anything about this other than not crashing.

clang/test/Parser/objc-static-assert.mm

4 ↗

(On Diff #190534)

Thanks for catching this, looks I forgot to re-run the .mm test after changing Features.def :-( Fixed now in r356154.

[aaron.ballman]

clang/test/Parser/objc-static-assert.m

29 ↗

(On Diff #190534)

Fine by me!

clang/test/Parser/objc-static-assert.mm

4 ↗

(On Diff #190534)

Great, thank you!

Other than those points, this LGTM as well. :-)

include/clang/Basic/Features.def

 FEATURE(objc_boxed_expressions, LangOpts.ObjC)
 FEATURE(objc_boxed_nsvalue_expressions, LangOpts.ObjC)
 FEATURE(arc_cf_code_audited, true)
 FEATURE(objc_bridge_id, true)
 FEATURE(objc_bridge_id_on_typedefs, true)
 FEATURE(objc_generics, LangOpts.ObjC)
 FEATURE(objc_generics_variance, LangOpts.ObjC)
 FEATURE(objc_class_property, LangOpts.ObjC)
+FEATURE(objc_c_static_assert, LangOpts.C11)
+FEATURE(objc_cxx_static_assert, LangOpts.CPlusPlus11)
+EXTENSION(objc_c_static_assert, true)
 // C11 features
 FEATURE(c_alignas, LangOpts.C11)
 FEATURE(c_alignof, LangOpts.C11)
 FEATURE(c_atomic, LangOpts.C11)
 FEATURE(c_generic_selections, LangOpts.C11)
 FEATURE(c_static_assert, LangOpts.C11)
 FEATURE(c_thread_local, LangOpts.C11 &&PP.getTargetInfo().isTLSSupported())
 // C++11 features

lib/Parse/ParseDecl.cpp

#include "clang/Basic/OpenCLImageTypes.def"
 
     AttrsLastTime = false;
   }
 }
 
 /// ParseStructDeclaration - Parse a struct declaration without the terminating
 /// semicolon.
 ///
+/// Note that a struct declaration refers to a declaration in a struct,
+/// not to the declaration of a struct.
+///
 ///       struct-declaration:
 /// [C2x]   attributes-specifier-seq[opt]
 ///           specifier-qualifier-list struct-declarator-list
 /// [GNU]   __extension__ struct-declaration
 /// [GNU]   specifier-qualifier-list
 ///       struct-declarator-list:
 ///         struct-declarator
 ///         struct-declarator-list ',' struct-declarator

lib/Parse/ParseObjc.cpp

if (Tok.is(tok::l_paren)) {
       Diag(Tok, diag::err_expected_minus_or_plus);
       ParseObjCMethodDecl(Tok.getLocation(),
                           tok::minus,
                           MethodImplKind, false);
       continue;
     }
     // Ignore excess semicolons.
     if (Tok.is(tok::semi)) {
+      // FIXME: This should use ConsumeExtraSemi() for extraneous semicolons,
+      // to make -Wextra-semi diagnose them.
       ConsumeToken();
       continue;
     }
 
     // If we got to the end of the file, exit the loop.
     if (isEofOrEom())
       break;
 
     // Code completion within an Objective-C interface.
     if (Tok.is(tok::code_completion)) {
       Actions.CodeCompleteOrdinaryName(getCurScope(),
                             CurParsedObjCImpl? Sema::PCC_ObjCImplementation
                                              : Sema::PCC_ObjCInterface);
       return cutOffParsing();
     }
 
     // If we don't have an @ directive, parse it as a function definition.
     if (Tok.isNot(tok::at)) {
       // The code below does not consume '}'s because it is afraid of eating the
       // end of a namespace.  Because of the way this code is structured, an
       // erroneous r_brace would cause an infinite loop if not handled here.
       if (Tok.is(tok::r_brace))
         break;
+
       ParsedAttributesWithRange attrs(AttrFactory);
+
+      // Since we call ParseDeclarationOrFunctionDefinition() instead of
+      // ParseExternalDeclaration() below (so that this doesn't parse nested
+      // @interfaces), this needs to duplicate some code from the latter.
+      if (Tok.isOneOf(tok::kw_static_assert, tok::kw__Static_assert)) {
+        SourceLocation DeclEnd;
+        allTUVariables.push_back(
+            ParseDeclaration(DeclaratorContext::FileContext, DeclEnd, attrs));
+        continue;
+      }
+
       allTUVariables.push_back(ParseDeclarationOrFunctionDefinition(attrs));
       continue;
     }
 
     // Otherwise, we have an @ directive, eat the @.
     SourceLocation AtLoc = ConsumeToken(); // the "@"
     if (Tok.is(tok::code_completion)) {
       Actions.CodeCompleteObjCAtDirective(getCurScope());

 ///     '{' objc-instance-variable-decl-list[opt] '}'
 ///
 ///   objc-instance-variable-decl-list:
 ///     objc-visibility-spec
 ///     objc-instance-variable-decl ';'
 ///     ';'
 ///     objc-instance-variable-decl-list objc-visibility-spec
 ///     objc-instance-variable-decl-list objc-instance-variable-decl ';'
+///     objc-instance-variable-decl-list static_assert-declaration
 ///     objc-instance-variable-decl-list ';'
 ///
 ///   objc-visibility-spec:
 ///     @private
 ///     @protected
 ///     @public
 ///     @package [OBJC2]
 ///
while (Tok.isNot(tok::r_brace) && !isEofOrEom()) {
     }
 
     if (Tok.is(tok::code_completion)) {
       Actions.CodeCompleteOrdinaryName(getCurScope(),
                                        Sema::PCC_ObjCInstanceVariableList);
       return cutOffParsing();
     }
 
+    // This needs to duplicate a small amount of code from
+    // ParseStructUnionBody() for things that should work in both
+    // C struct and in Objective-C class instance variables.
+    if (Tok.isOneOf(tok::kw_static_assert, tok::kw__Static_assert)) {
+      SourceLocation DeclEnd;
+      ParseStaticAssertDeclaration(DeclEnd);
+      continue;
+    }
+
     auto ObjCIvarCallback = [&](ParsingFieldDeclarator &FD) {
       Actions.ActOnObjCContainerStartDefinition(interfaceDecl);
       // Install the declarator into the interface decl.
       FD.D.setObjCIvar(true);
       Decl *Field = Actions.ActOnIvar(
           getCurScope(), FD.D.getDeclSpec().getSourceRange().getBegin(), FD.D,
           FD.BitfieldSize, visibility);
       Actions.ActOnObjCContainerFinishDefinition();

lib/Sema/SemaExpr.cpp

 #define VALUE(type, base)
 #define DECL(type, base) \
     case Decl::type:
 #include "clang/AST/DeclNodes.inc"
       llvm_unreachable("invalid value decl kind");
 
     // These shouldn't make it here.
     case Decl::ObjCAtDefsField:
-    case Decl::ObjCIvar:
       llvm_unreachable("forming non-member reference to ivar?");
 
     // Enum constants are always r-values and never references.
     // Unresolved using declarations are dependent.
     case Decl::EnumConstant:
     case Decl::UnresolvedUsingValue:
     case Decl::OMPDeclareReduction:
     case Decl::OMPDeclareMapper:
       valueKind = VK_RValue;
       break;
 
     // Fields and indirect fields that got here must be for
     // pointer-to-member expressions; we just call them l-values for
     // internal consistency, because this subexpression doesn't really
     // exist in the high-level semantics.
     case Decl::Field:
     case Decl::IndirectField:
+    case Decl::ObjCIvar:
       assert(getLangOpts().CPlusPlus &&
              "building reference to field in C?");
 
       // These can't have reference type in well-formed programs, but
       // for internal consistency we do this anyway.
       type = type.getNonReferenceType();
       valueKind = VK_LValue;
       break;

test/Parser/objc-static-assert.m

+// RUN: %clang_cc1 -fobjc-runtime=macosx-fragile -fsyntax-only -verify -Wno-objc-root-class %s
+// RUN: %clang_cc1 -std=c89 -fobjc-runtime=macosx-fragile -fsyntax-only -verify -Wno-objc-root-class %s
+
+
+#if __STDC_VERSION__ >= 201112L
+
+#if !__has_feature(objc_c_static_assert)
+#error failed
+#endif
+
+#if !__has_extension(objc_c_static_assert)
+#error failed
+#endif
+
+@interface A {
+  int a;
+  _Static_assert(1, "");
+  _Static_assert(0, ""); // expected-error {{static_assert failed}}
+
+  _Static_assert(a, ""); // expected-error {{use of undeclared identifier 'a'}}
+  _Static_assert(sizeof(a), ""); // expected-error {{use of undeclared identifier 'a'}}
+}
+
+_Static_assert(1, "");
+
+@end
+
+struct S {
+  @defs(A);
+};
+
+#else
+
+// _Static_assert is available before C11 as an extension, but -pedantic
+// warns on it.
+#if __has_feature(objc_c_static_assert)
+#error failed
+#endif
+
+#if !__has_extension(objc_c_static_assert)
+#error failed
+#endif
+
+@interface A {
+  int a;
+  _Static_assert(1, "");
+  _Static_assert(0, ""); // expected-error {{static_assert failed}}
+}
+
+_Static_assert(1, "");
+
+@end
+
+#endif

test/Parser/objc-static-assert.mm

+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-objc-root-class %s
+// RUN: %clang_cc1 -std=c++98 -fsyntax-only -verify -Wno-objc-root-class %s
+
+#if !__has_feature(objc_c_static_assert)
+#error failed
+#endif
+
+#if __cplusplus >= 201103L
+
+#if !__has_feature(objc_cxx_static_assert)
+#error failed
+#endif
+
+// C++11
+
+@interface A {
+  int a;
+  static_assert(1, "");
+  _Static_assert(1, "");
+
+  static_assert(0, ""); // expected-error {{static_assert failed}}
+  _Static_assert(0, ""); // expected-error {{static_assert failed}}
+
+  static_assert(a, ""); // expected-error {{static_assert expression is not an integral constant expression}}
+  static_assert(sizeof(a) == 4, "");
+  static_assert(sizeof(a) == 3, ""); // expected-error {{static_assert failed}}
+}
+
+static_assert(1, "");
+_Static_assert(1, "");
+
+- (void)f;
+@end
+
+@implementation A {
+  int b;
+  static_assert(1, "");
+  _Static_assert(1, "");
+  static_assert(sizeof(b) == 4, "");
+  static_assert(sizeof(b) == 3, ""); // expected-error {{static_assert failed}}
+}
+
+static_assert(1, "");
+
+- (void)f {
+  static_assert(1, "");
+}
+@end
+
+@interface B
+@end
+
+@interface B () {
+  int b;
+  static_assert(sizeof(b) == 4, "");
+  static_assert(sizeof(b) == 3, ""); // expected-error {{static_assert failed}}
+}
+@end
+
+#else
+
+#if __has_feature(objc_cxx_static_assert)
+#error failed
+#endif
+
+// C++98
+@interface A {
+  int a;
+  static_assert(1, ""); // expected-error {{type name requires a specifier or qualifier}} expected-error{{expected parameter declarator}} expected-error {{expected ')'}} expected-note {{to match this '('}}
+  _Static_assert(1, "");
+  _Static_assert(0, ""); // expected-error {{static_assert failed}}
+}
+@end
+#endif