Allow __attribute__((swift_attr)) in attribute push pragmas

[beccadax]

Summary

This change does what it says on the tin: it allows SwiftAttr to be used with #pragma clang attribute push to add Swift attributes to large regions of header files. We plan to use this to annotate headers with concurrency information.

rdar://83499885

[arphaman]

Looks good to me. Can you post the patch with the full context next time so that it's easier to review?

[aaron.ballman]

swift_attr has no subjects, so this will attempt to spray the attribute onto literally *everything*. That makes this incredibly risky to use with the pragma approach (not to mention what it could do to memory consumption of large ASTs). I'm not certain I'm comfortable allowing this without an explicit set of subjects for the attribute. I took a look and the only other attribute currently allowed there is annotate, and I'm not convinced that was a good idea to allow for use in this pragma. Can we add subjects to this attribute to try to limit the damage somewhat?

[beccadax]

In D112773#3096185, @aaron.ballman wrote:

swift_attr has no subjects, so this will attempt to spray the attribute onto literally *everything*. That makes this incredibly risky to use with the pragma approach (not to mention what it could do to memory consumption of large ASTs). I'm not certain I'm comfortable allowing this without an explicit set of subjects for the attribute.

What exactly is the risk here? In my testing, omitting apply_to or setting it to any or any() caused clang to apply the attribute to nothing, not everything. If there is a way I didn't find, perhaps we could address that by emitting a warning if you use the "match anything" subject with an attribute that has an empty subject list.

Can we add subjects to this attribute to try to limit the damage somewhat?

I don't think we can, because swift_attr is by design very widely applicable—it allows you to apply arbitrary Swift language features to the imported representation of any Clang declaration. Any declaration Swift imports, including types and typedefs, functions, non-local variables, and parameters, ought to accept it.

Looking through the subject matchers available, almost everything would at least theoretically be a reasonable target for swift_attr (variable(is_local) is the only exception I noticed). We could restrict it to only declarations we're currently using it for, but many of these (e.g. C++ declarations) are likely to be supported in the future, so this would create clang churn and integration issues as Swift's capabilities expand.

So I don't think the risk you're worried about is really there, and I don't think adding a subject list would mitigate it very much. But if I've missed something, please point it out.

[aaron.ballman]

In D112773#3097730, @beccadax wrote:

In D112773#3096185, @aaron.ballman wrote:

swift_attr has no subjects, so this will attempt to spray the attribute onto literally *everything*. That makes this incredibly risky to use with the pragma approach (not to mention what it could do to memory consumption of large ASTs). I'm not certain I'm comfortable allowing this without an explicit set of subjects for the attribute.

What exactly is the risk here? In my testing, omitting apply_to or setting it to any or any() caused clang to apply the attribute to nothing, not everything. If there is a way I didn't find, perhaps we could address that by emitting a warning if you use the "match anything" subject with an attribute that has an empty subject list.

Oohhhh, that is not what my expectations were. I thought we would be applying it to anything in this situation. That we're not is possibly a bug, but I happen to like the behavior in this case.

Can we add subjects to this attribute to try to limit the damage somewhat?

I don't think we can, because swift_attr is by design very widely applicable—it allows you to apply arbitrary Swift language features to the imported representation of any Clang declaration. Any declaration Swift imports, including types and typedefs, functions, non-local variables, and parameters, ought to accept it.

Looking through the subject matchers available, almost everything would at least theoretically be a reasonable target for swift_attr (variable(is_local) is the only exception I noticed). We could restrict it to only declarations we're currently using it for, but many of these (e.g. C++ declarations) are likely to be supported in the future, so this would create clang churn and integration issues as Swift's capabilities expand.

That sounds like a good reason to not give it a subject list.

So I don't think the risk you're worried about is really there, and I don't think adding a subject list would mitigate it very much. But if I've missed something, please point it out.

I'd like some confirmation of that. My reading of https://clang.llvm.org/docs/LanguageExtensions.html#specifying-an-attribute-for-multiple-declarations-pragma-clang-attribute suggests that any() /could/ apply to everything. (The any rule applies attributes to all declarations that are matched by at least one of the rules in the any.) My recollection of the discussions when we adopted #pragma clang attribute is that we tried our best to limit the damage from the pragma over-applying attributes, but I don't recall if we explicitly talked about any() with no subjects. What would make me comfortable with this patch is 1) doing some archeology to find the original reviews for pragma clang attribute to see if we made any sort of explicit decision here but forgot to document it. If we didn't make an explicit decision in this area, we should make one now. We can either decide "any() with no subjects applies to everything", "any() with no subjects is dangerous and
will apply to nothing", or "any() with no subjects is dangerous and we will diagnose if the user tries this" (probably other ideas we could consider). Ultimately, I would like to see the pragma clang attribute documentation updated before/when we land this bit so that our documentation clearly sets user expectations.

[beccadax]

My reading of https://clang.llvm.org/docs/LanguageExtensions.html#specifying-an-attribute-for-multiple-declarations-pragma-clang-attribute suggests that any() /could/ apply to everything. (The any rule applies attributes to all declarations that are matched by at least one of the rules in the any.)

My reading of that rule suggest that an any() with no subrules would not match anything because, if you fed in a declaration, none of the (nonexistent) subrules of the any() would match it.

My recollection of the discussions when we adopted #pragma clang attribute is that we tried our best to limit the damage from the pragma over-applying attributes, but I don't recall if we explicitly talked about any() with no subjects. What would make me comfortable with this patch is 1) doing some archeology to find the original reviews for pragma clang attribute to see if we made any sort of explicit decision here but forgot to document it. If we didn't make an explicit decision in this area, we should make one now. We can either decide "any() with no subjects applies to everything", "any() with no subjects is dangerous and will apply to nothing", or "any() with no subjects is dangerous and we will diagnose if the user tries this" (probably other ideas we could consider). Ultimately, I would like to see the pragma clang attribute documentation updated before/when we land this bit so that our documentation clearly sets user expectations.

Looking at https://reviews.llvm.org/D30009, I don't see any relevant discussion of how any() (without subrules) should behave. What I do see is that they started with "match everything always" behavior, but concerns about users not knowing which declarations it would be applied to (and how that behavior might change as the compiler evolved) led them to the idea of requiring users to list the declarations they expected the compiler to match and having a way to allow a subset; then they realized that only the subset version was actually useful. What I take from this is that there was clearly concern about users not being able to tell what the pragma would affect and they changed the design so that users would need to specify it clearly, which suggests that a way to leave it unspecified was seen as an anti-goal.

Looking at the implementation of Parser::ParsePragmaAttributeSubjectMatchRuleSet(), it looks like any or any() would cause a parse error—we return true (as in the error branches) if we do not parse an open parenthesis or do not parse an identifier after the open parenthesis—which again makes me think that the design intentionally left out any mechanism to match all subjects.

We don't have a statement that there is and should be no way to apply attribute push to everything, but I think we can conclude that having such a capability wasn't a goal of the final design.

What would you like me to do from here?

[aaron.ballman]

In D112773#3135975, @beccadax wrote:

My reading of https://clang.llvm.org/docs/LanguageExtensions.html#specifying-an-attribute-for-multiple-declarations-pragma-clang-attribute suggests that any() /could/ apply to everything. (The any rule applies attributes to all declarations that are matched by at least one of the rules in the any.)

My reading of that rule suggest that an any() with no subrules would not match anything because, if you fed in a declaration, none of the (nonexistent) subrules of the any() would match it.

My recollection of the discussions when we adopted #pragma clang attribute is that we tried our best to limit the damage from the pragma over-applying attributes, but I don't recall if we explicitly talked about any() with no subjects. What would make me comfortable with this patch is 1) doing some archeology to find the original reviews for pragma clang attribute to see if we made any sort of explicit decision here but forgot to document it. If we didn't make an explicit decision in this area, we should make one now. We can either decide "any() with no subjects applies to everything", "any() with no subjects is dangerous and will apply to nothing", or "any() with no subjects is dangerous and we will diagnose if the user tries this" (probably other ideas we could consider). Ultimately, I would like to see the pragma clang attribute documentation updated before/when we land this bit so that our documentation clearly sets user expectations.

Looking at https://reviews.llvm.org/D30009, I don't see any relevant discussion of how any() (without subrules) should behave. What I do see is that they started with "match everything always" behavior, but concerns about users not knowing which declarations it would be applied to (and how that behavior might change as the compiler evolved) led them to the idea of requiring users to list the declarations they expected the compiler to match and having a way to allow a subset; then they realized that only the subset version was actually useful. What I take from this is that there was clearly concern about users not being able to tell what the pragma would affect and they changed the design so that users would need to specify it clearly, which suggests that a way to leave it unspecified was seen as an anti-goal.

Looking at the implementation of Parser::ParsePragmaAttributeSubjectMatchRuleSet(), it looks like any or any() would cause a parse error—we return true (as in the error branches) if we do not parse an open parenthesis or do not parse an identifier after the open parenthesis—which again makes me think that the design intentionally left out any mechanism to match all subjects.

We don't have a statement that there is and should be no way to apply attribute push to everything, but I think we can conclude that having such a capability wasn't a goal of the final design.

What would you like me to do from here?

Thank you for diving into those details! I'm now sold on the idea that any with no subjects is dangerous and we will diagnose if the user tries this. So I think the only thing that needs to happen is a documentation update and some additional test coverage for this scenario, which I'm happy to take care of myself since it's orthogonal to these changes. Based on that, this LGTM as-is.

[aaron.ballman]

In D112773#3137338, @aaron.ballman wrote:

Thank you for diving into those details! I'm now sold on the idea that any with no subjects is dangerous and we will diagnose if the user tries this. So I think the only thing that needs to happen is a documentation update and some additional test coverage for this scenario, which I'm happy to take care of myself since it's orthogonal to these changes. Based on that, this LGTM as-is.

FWIW, I updated the docs and added explicit tests in 3874277f415dca0bb222956983f117a6211c0e39.

[arphaman]

I will commit this on behalf of @beccadax as she doesn't have commit access yet.

clang/include/clang/Basic/Attr.td

def SwiftAsyncName : InheritableAttr {
   let Subjects = SubjectList<[ObjCMethod, Function], ErrorDiag>;
   let Documentation = [SwiftAsyncNameDocs];
 }
 
 def SwiftAttr : InheritableAttr {
   let Spellings = [GNU<"swift_attr">];
   let Args = [StringArgument<"Attribute">];
   let Documentation = [SwiftAttrDocs];
+  let PragmaAttributeSupport = 1;
 }
 
 def SwiftBridge : InheritableAttr {
   let Spellings = [GNU<"swift_bridge">];
   let Args = [StringArgument<"SwiftType">];
   let Subjects = SubjectList<[Tag, TypedefName, ObjCInterface, ObjCProtocol],
                              ErrorDiag>;
   let Documentation = [SwiftBridgeDocs];

clang/test/AST/attr-swift_attr.m

 // RUN: %clang_cc1 -fsyntax-only -ast-dump %s | FileCheck %s
 
 __attribute__((swift_attr("@actor")))
 @interface View
 @end
 
-// CHECK: InterfaceDecl {{.*}} View
+// CHECK-LABEL: InterfaceDecl {{.*}} View
 // CHECK-NEXT: SwiftAttrAttr {{.*}} "@actor"
+
+#pragma clang attribute push(__attribute__((swift_attr("@sendable"))), apply_to=objc_interface)
+@interface Contact
+@end
+#pragma clang attribute pop
+
+// CHECK-LABEL: InterfaceDecl {{.*}} Contact
+// CHECK-NEXT: SwiftAttrAttr {{.*}} "@sendable"

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

 // CHECK-NEXT: Section (SubjectMatchRule_function, SubjectMatchRule_variable_is_global, SubjectMatchRule_objc_method, SubjectMatchRule_objc_property)
 // CHECK-NEXT: SetTypestate (SubjectMatchRule_function_is_member)
 // CHECK-NEXT: SpeculativeLoadHardening (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: StandaloneDebug (SubjectMatchRule_record)
 // CHECK-NEXT: SwiftAsync (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: SwiftAsyncContext (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: SwiftAsyncError (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: SwiftAsyncName (SubjectMatchRule_objc_method, SubjectMatchRule_function)
+// CHECK-NEXT: SwiftAttr ()
 // CHECK-NEXT: SwiftBridgedTypedef (SubjectMatchRule_type_alias)
 // CHECK-NEXT: SwiftContext (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: SwiftError (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: SwiftErrorResult (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: SwiftIndirectResult (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: SwiftNewType (SubjectMatchRule_type_alias)
 // CHECK-NEXT: SwiftObjCMembers (SubjectMatchRule_objc_interface)
 // CHECK-NEXT: TLSModel (SubjectMatchRule_variable_is_thread_local)