[PATCH] D26922: [ObjC++] Don't enter a C++ declarator context when the current context is an Objective-C declaration
This revision was automatically updated to reflect the committed changes. Closed by commit rL288893: [ObjC++] Don't enter a C++ declarator scope when the current context is (authored by arphaman). Changed prior to commit: https://reviews.llvm.org/D26922?vs=79722&id=80556#toc Repository: rL LLVM https://reviews.llvm.org/D26922 Files: cfe/trunk/lib/Parse/ParseDecl.cpp cfe/trunk/lib/Sema/SemaCXXScopeSpec.cpp cfe/trunk/test/SemaObjCXX/crash.mm Index: cfe/trunk/test/SemaObjCXX/crash.mm === --- cfe/trunk/test/SemaObjCXX/crash.mm +++ cfe/trunk/test/SemaObjCXX/crash.mm @@ -25,3 +25,38 @@ // expected-warning@-2 {{variadic templates are a C++11 extension}} #endif @end + +// rdar://20560175 + +struct OuterType { + typedef int InnerType; +}; + +namespace ns { + typedef int InnerType; +}; + +@protocol InvalidProperties + +@property (nonatomic) (OuterType::InnerType) invalidTypeParens; +// expected-error@-1 {{type name requires a specifier or qualifier}} +// expected-error@-2 {{property requires fields to be named}} +// expected-error@-3 {{expected ';' at end of declaration list}} +// expected-error@-4 {{C++ requires a type specifier for all declarations}} +// expected-error@-5 {{cannot declare variable inside @interface or @protocol}} + +@property (nonatomic) (ns::InnerType) invalidTypeParens2; +// expected-error@-1 {{type name requires a specifier or qualifier}} +// expected-error@-2 {{property requires fields to be named}} +// expected-error@-3 {{expected ';' at end of declaration list}} +// expected-error@-4 {{C++ requires a type specifier for all declarations}} +// expected-error@-5 {{cannot declare variable inside @interface or @protocol}} + +@property (nonatomic) int OuterType::InnerType; // expected-error {{property requires fields to be named}} + +@property (nonatomic) int OuterType::InnerType foo; // expected-error {{property requires fields to be named}} +// expected-error@-1 {{expected ';' at end of declaration list}} +// expected-error@-2 {{C++ requires a type specifier for all declarations}} +// expected-error@-3 {{cannot declare variable inside @interface or @protocol}} + +@end Index: cfe/trunk/lib/Sema/SemaCXXScopeSpec.cpp === --- cfe/trunk/lib/Sema/SemaCXXScopeSpec.cpp +++ cfe/trunk/lib/Sema/SemaCXXScopeSpec.cpp @@ -1001,6 +1001,11 @@ bool Sema::ShouldEnterDeclaratorScope(Scope *S, const CXXScopeSpec &SS) { assert(SS.isSet() && "Parser passed invalid CXXScopeSpec."); + // Don't enter a declarator context when the current context is an Objective-C + // declaration. + if (isa(CurContext) || isa(CurContext)) +return false; + NestedNameSpecifier *Qualifier = SS.getScopeRep(); // There are only two places a well-formed program may qualify a Index: cfe/trunk/lib/Parse/ParseDecl.cpp === --- cfe/trunk/lib/Parse/ParseDecl.cpp +++ cfe/trunk/lib/Parse/ParseDecl.cpp @@ -5264,6 +5264,14 @@ // Change the declaration context for name lookup, until this function // is exited (and the declarator has been parsed). DeclScopeObj.EnterDeclaratorScope(); + else if (getObjCDeclContext()) { +// Ensure that we don't interpret the next token as an identifier when +// dealing with declarations in an Objective-C container. +D.SetIdentifier(nullptr, Tok.getLocation()); +D.setInvalidType(true); +ConsumeToken(); +goto PastIdentifier; + } } // C++0x [dcl.fct]p14: Index: cfe/trunk/test/SemaObjCXX/crash.mm === --- cfe/trunk/test/SemaObjCXX/crash.mm +++ cfe/trunk/test/SemaObjCXX/crash.mm @@ -25,3 +25,38 @@ // expected-warning@-2 {{variadic templates are a C++11 extension}} #endif @end + +// rdar://20560175 + +struct OuterType { + typedef int InnerType; +}; + +namespace ns { + typedef int InnerType; +}; + +@protocol InvalidProperties + +@property (nonatomic) (OuterType::InnerType) invalidTypeParens; +// expected-error@-1 {{type name requires a specifier or qualifier}} +// expected-error@-2 {{property requires fields to be named}} +// expected-error@-3 {{expected ';' at end of declaration list}} +// expected-error@-4 {{C++ requires a type specifier for all declarations}} +// expected-error@-5 {{cannot declare variable inside @interface or @protocol}} + +@property (nonatomic) (ns::InnerType) invalidTypeParens2; +// expected-error@-1 {{type name requires a specifier or qualifier}} +// expected-error@-2 {{property requires fields to be named}} +// expected-error@-3 {{expected ';' at end of declaration list}} +// expected-error@-4 {{C++ requires a type specifier for all declarations}} +// expected-error@-5 {{cannot declare variable inside @interface or @protocol}} + +@property (nonatomic) int OuterType::InnerType; // expected-error {{property re
[PATCH] D26922: [ObjC++] Don't enter a C++ declarator context when the current context is an Objective-C declaration
ahatanak accepted this revision. ahatanak added a comment. This revision is now accepted and ready to land. I think this is fine. I guess we can print a more helpful error message than "error: property requires fields to be named", but we can probably do it later. Repository: rL LLVM https://reviews.llvm.org/D26922 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26922: [ObjC++] Don't enter a C++ declarator context when the current context is an Objective-C declaration
arphaman added a comment. > Also, the following code (which is not valid) crashes with ToT trunk, > > @property (nonatomic) int OuterType::InnerType > > > but compiles without any errors with your patch applied. Do you know why > clang doesn't error out? This worked because clang continued parsing `InnerType` as the name of the property. The updated patch fixes this by ensuring that we don't interpret this token as a property name, so now this example will have a compilation error. Repository: rL LLVM https://reviews.llvm.org/D26922 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26922: [ObjC++] Don't enter a C++ declarator context when the current context is an Objective-C declaration
arphaman updated this revision to Diff 79722. arphaman added a comment. The updated patch performs the check in `ShouldEnterDeclaratorScope`. Repository: rL LLVM https://reviews.llvm.org/D26922 Files: lib/Parse/ParseDecl.cpp lib/Sema/SemaCXXScopeSpec.cpp test/SemaObjCXX/crash.mm Index: test/SemaObjCXX/crash.mm === --- test/SemaObjCXX/crash.mm +++ test/SemaObjCXX/crash.mm @@ -25,3 +25,38 @@ // expected-warning@-2 {{variadic templates are a C++11 extension}} #endif @end + +// rdar://20560175 + +struct OuterType { + typedef int InnerType; +}; + +namespace ns { + typedef int InnerType; +}; + +@protocol InvalidProperties + +@property (nonatomic) (OuterType::InnerType) invalidTypeParens; +// expected-error@-1 {{type name requires a specifier or qualifier}} +// expected-error@-2 {{property requires fields to be named}} +// expected-error@-3 {{expected ';' at end of declaration list}} +// expected-error@-4 {{C++ requires a type specifier for all declarations}} +// expected-error@-5 {{cannot declare variable inside @interface or @protocol}} + +@property (nonatomic) (ns::InnerType) invalidTypeParens2; +// expected-error@-1 {{type name requires a specifier or qualifier}} +// expected-error@-2 {{property requires fields to be named}} +// expected-error@-3 {{expected ';' at end of declaration list}} +// expected-error@-4 {{C++ requires a type specifier for all declarations}} +// expected-error@-5 {{cannot declare variable inside @interface or @protocol}} + +@property (nonatomic) int OuterType::InnerType; // expected-error {{property requires fields to be named}} + +@property (nonatomic) int OuterType::InnerType foo; // expected-error {{property requires fields to be named}} +// expected-error@-1 {{expected ';' at end of declaration list}} +// expected-error@-2 {{C++ requires a type specifier for all declarations}} +// expected-error@-3 {{cannot declare variable inside @interface or @protocol}} + +@end Index: lib/Sema/SemaCXXScopeSpec.cpp === --- lib/Sema/SemaCXXScopeSpec.cpp +++ lib/Sema/SemaCXXScopeSpec.cpp @@ -1001,6 +1001,11 @@ bool Sema::ShouldEnterDeclaratorScope(Scope *S, const CXXScopeSpec &SS) { assert(SS.isSet() && "Parser passed invalid CXXScopeSpec."); + // Don't enter a declarator context when the current context is an Objective-C + // declaration. + if (isa(CurContext) || isa(CurContext)) +return false; + NestedNameSpecifier *Qualifier = SS.getScopeRep(); // There are only two places a well-formed program may qualify a Index: lib/Parse/ParseDecl.cpp === --- lib/Parse/ParseDecl.cpp +++ lib/Parse/ParseDecl.cpp @@ -5264,6 +5264,14 @@ // Change the declaration context for name lookup, until this function // is exited (and the declarator has been parsed). DeclScopeObj.EnterDeclaratorScope(); + else if (getObjCDeclContext()) { +// Ensure that we don't interpret the next token as an identifier when +// dealing with declarations in an Objective-C container. +D.SetIdentifier(nullptr, Tok.getLocation()); +D.setInvalidType(true); +ConsumeToken(); +goto PastIdentifier; + } } // C++0x [dcl.fct]p14: Index: test/SemaObjCXX/crash.mm === --- test/SemaObjCXX/crash.mm +++ test/SemaObjCXX/crash.mm @@ -25,3 +25,38 @@ // expected-warning@-2 {{variadic templates are a C++11 extension}} #endif @end + +// rdar://20560175 + +struct OuterType { + typedef int InnerType; +}; + +namespace ns { + typedef int InnerType; +}; + +@protocol InvalidProperties + +@property (nonatomic) (OuterType::InnerType) invalidTypeParens; +// expected-error@-1 {{type name requires a specifier or qualifier}} +// expected-error@-2 {{property requires fields to be named}} +// expected-error@-3 {{expected ';' at end of declaration list}} +// expected-error@-4 {{C++ requires a type specifier for all declarations}} +// expected-error@-5 {{cannot declare variable inside @interface or @protocol}} + +@property (nonatomic) (ns::InnerType) invalidTypeParens2; +// expected-error@-1 {{type name requires a specifier or qualifier}} +// expected-error@-2 {{property requires fields to be named}} +// expected-error@-3 {{expected ';' at end of declaration list}} +// expected-error@-4 {{C++ requires a type specifier for all declarations}} +// expected-error@-5 {{cannot declare variable inside @interface or @protocol}} + +@property (nonatomic) int OuterType::InnerType; // expected-error {{property requires fields to be named}} + +@property (nonatomic) int OuterType::InnerType foo; // expected-error {{property requires fields to be named}} +// expected-error@-1 {{expected ';' at end of declaration list}} +// expected-error@-2 {{C++ requires a type specifier for all declarations}} +
[PATCH] D26922: [ObjC++] Don't enter a C++ declarator context when the current context is an Objective-C declaration
arphaman added a comment. In https://reviews.llvm.org/D26922#608815, @ahatanak wrote: > Yes, I meant ParseDecl:5266. Reading the comment in > ShouldEnterDeclaratorScope, it seemed to me that it shouldn't return true in > this context (when parsing an objective-c). Yes, I agree that it's better to move the check to ShouldEnterDeclaratorScope, I will update the patch. > Also, the following code (which is not valid) crashes with ToT trunk, > > @property (nonatomic) int OuterType::InnerType > > > but compiles without any errors with your patch applied. Do you know why > clang doesn't error out? Repository: rL LLVM https://reviews.llvm.org/D26922 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26922: [ObjC++] Don't enter a C++ declarator context when the current context is an Objective-C declaration
arphaman added a comment. In https://reviews.llvm.org/D26922#608815, @ahatanak wrote: > Yes, I meant ParseDecl:5266. Reading the comment in > ShouldEnterDeclaratorScope, it seemed to me that it shouldn't return true in > this context (when parsing an objective-c). > > Also, the following code (which is not valid) crashes with ToT trunk, > > @property (nonatomic) int OuterType::InnerType > > > but compiles without any errors with your patch applied. Do you know why > clang doesn't error out? Hmm, that's interesting, I have to investigate this. It looks like clang is creating a property named `InnerType` which is obviously invalid. Repository: rL LLVM https://reviews.llvm.org/D26922 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26922: [ObjC++] Don't enter a C++ declarator context when the current context is an Objective-C declaration
ahatanak added a comment. Yes, I meant ParseDecl:5266. Reading the comment in ShouldEnterDeclaratorScope, it seemed to me that it shouldn't return true in this context (when parsing an objective-c). Also, the following code (which is not valid) crashes with ToT trunk, @property (nonatomic) int OuterType::InnerType but compiles without any errors with your patch applied. Do you know why clang doesn't error out? Repository: rL LLVM https://reviews.llvm.org/D26922 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26922: [ObjC++] Don't enter a C++ declarator context when the current context is an Objective-C declaration
arphaman added a comment. In https://reviews.llvm.org/D26922#607186, @ahatanak wrote: > I wonder whether it is possible to avoid calling > DeclScopeObj.EnterDeclaratorScope at ParseDecl.cpp:5317 instead of fixing > Sema::ActOnCXXEnterDeclaratorScope? > > I believe it's calling EnterDeclaratorScope to enable name lookup when a > namespace-member variable is defined outside the namespace. Since that's not > the case here, it seems to me that it shouldn't called in the first place. > > http://en.cppreference.com/w/cpp/language/unqualified_lookup Do you mean at ParseDecl:5266 (I get this line in the crash backtrace with ToT)? AFAIK it's also used for static class member lookups for variables defined outside of a class. The `OuterType` is a C++ record so that's why it should be valid to call `EnterDeclaratorScope` there. I tested to verify this and I reach that call on line 5266 with the following example 3 times: namespace Foo { extern int x; } int Foo::x = 2; // EnterDeclaratorScope is called here class Bar { public: void foo(); static int m; }; int Bar::m = 2; // here class FooBar { friend void Bar::foo(); // and here }; So it doesn't seem reasonable to required this only for namespaces. I suppose it might be better to avoid calling `EnterDeclaratorScope` anyway by moving the current context ObjC isa checks to `ShouldEnterDeclaratorScope`. Repository: rL LLVM https://reviews.llvm.org/D26922 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26922: [ObjC++] Don't enter a C++ declarator context when the current context is an Objective-C declaration
ahatanak added a comment. I wonder whether it is possible to avoid calling DeclScopeObj.EnterDeclaratorScope at ParseDecl.cpp:5317 instead of fixing Sema::ActOnCXXEnterDeclaratorScope? I believe it's calling EnterDeclaratorScope to enable name lookup when a namespace-member variable is defined outside the namespace. Since that's not the case here, it seems to me that it shouldn't called in the first place. http://en.cppreference.com/w/cpp/language/unqualified_lookup Repository: rL LLVM https://reviews.llvm.org/D26922 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26922: [ObjC++] Don't enter a C++ declarator context when the current context is an Objective-C declaration
arphaman created this revision. arphaman added reviewers: manmanren, ahatanak. arphaman added a subscriber: cfe-commits. arphaman set the repository for this revision to rL LLVM. This patch ensures that Sema won't enter a C++ declarator context when the current context is an Objective-C declaration. This prevents an assertion failure in `EnterDeclaratorContext` that's used to ensure that current context will be restored correctly after exiting the declarator context. I think that this approach is reasonable as AFAIK we don't need to use the C++ declarator contexts directly in Objective-C declarations, since they're mostly used to define out of line variables declared in classes or namespaces which shouldn't be done inside an Objective-C declaration. Repository: rL LLVM https://reviews.llvm.org/D26922 Files: lib/Sema/SemaCXXScopeSpec.cpp test/SemaObjCXX/crash.mm Index: test/SemaObjCXX/crash.mm === --- test/SemaObjCXX/crash.mm +++ test/SemaObjCXX/crash.mm @@ -25,3 +25,17 @@ // expected-warning@-2 {{variadic templates are a C++11 extension}} #endif @end + +// rdar://20560175 + +struct OuterType { + typedef int InnerType; +}; + +@protocol InvalidProperty +@property (nonatomic) (OuterType::InnerType) invalidTypeParens; +// expected-error@-1 {{type name requires a specifier or qualifier}} +// expected-error@-2 {{expected ';' at end of declaration list}} +// expected-error@-3 {{C++ requires a type specifier for all declarations}} +// expected-error@-4 {{cannot declare variable inside @interface or @protocol}} +@end Index: lib/Sema/SemaCXXScopeSpec.cpp === --- lib/Sema/SemaCXXScopeSpec.cpp +++ lib/Sema/SemaCXXScopeSpec.cpp @@ -1054,7 +1054,12 @@ // it is a complete declaration context. if (!DC->isDependentContext() && RequireCompleteDeclContext(SS, DC)) return true; - + + // Don't enter a declarator context when the current context is an Objective-C + // declaration as we might no be able to restore it when exiting the scope. + if (isa(CurContext) || isa(CurContext)) +return true; + EnterDeclaratorContext(S, DC); // Rebuild the nested name specifier for the new scope. Index: test/SemaObjCXX/crash.mm === --- test/SemaObjCXX/crash.mm +++ test/SemaObjCXX/crash.mm @@ -25,3 +25,17 @@ // expected-warning@-2 {{variadic templates are a C++11 extension}} #endif @end + +// rdar://20560175 + +struct OuterType { + typedef int InnerType; +}; + +@protocol InvalidProperty +@property (nonatomic) (OuterType::InnerType) invalidTypeParens; +// expected-error@-1 {{type name requires a specifier or qualifier}} +// expected-error@-2 {{expected ';' at end of declaration list}} +// expected-error@-3 {{C++ requires a type specifier for all declarations}} +// expected-error@-4 {{cannot declare variable inside @interface or @protocol}} +@end Index: lib/Sema/SemaCXXScopeSpec.cpp === --- lib/Sema/SemaCXXScopeSpec.cpp +++ lib/Sema/SemaCXXScopeSpec.cpp @@ -1054,7 +1054,12 @@ // it is a complete declaration context. if (!DC->isDependentContext() && RequireCompleteDeclContext(SS, DC)) return true; - + + // Don't enter a declarator context when the current context is an Objective-C + // declaration as we might no be able to restore it when exiting the scope. + if (isa(CurContext) || isa(CurContext)) +return true; + EnterDeclaratorContext(S, DC); // Rebuild the nested name specifier for the new scope. ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits