[clang] store an identifer instead of declref for ns_error_domain attribute

[arphaman]

Summary

The attribute with declref is incompatible with Apple's SDKs, as the declref
at the point of use in the attribute might not be available due to the availability
annotations on the domain declaration. The users also can't fix this by applying
the availability attributes to the user of ns_error_domain as at that point the
availability diagnostic is already emitted. The use of identifier is ideal as
clang then doesn't need to do availability checking when its referenced in the attribute.

[arphaman]

The original implementation of the attribute on GitHub.com/apple/llvm-project also used the identifier instead of the declref, so we would like to go back to that.

[MForster]

That's ok for me. I'd suggest to wait for Aaron's feedback, because IIRC it was his suggestion in the first place to go to declref.

[aaron.ballman]

In D98450#2621877, @MForster wrote:

That's ok for me. I'd suggest to wait for Aaron's feedback, because IIRC it was his suggestion in the first place to go to declref.

I have the same concerns with this patch as I did with the initial one, see https://reviews.llvm.org/D84005#inline-775391 -- basically, we're doing a lookup here in SemaDeclAttr.cpp and saying "I found the identifier, everything's good", then storing the identifier into the attribute so we can look it up again later, at which point looking up the identifier may find something totally unrelated to what was found in SemaDeclAttr.cpp.

The attribute with declref is incompatible with Apple's SDKs, as the declref at the point of use in the attribute might not be available due to the availability annotations on the domain declaration.

Can you help me to understand why this isn't the expected behavior? If you name something that's not available, it seems surprising to me that it would then be available but only as one particular attribute argument.

[arphaman]

Sorry, getting back to this only now.

In D98450#2621907, @aaron.ballman wrote:

In D98450#2621877, @MForster wrote:

That's ok for me. I'd suggest to wait for Aaron's feedback, because IIRC it was his suggestion in the first place to go to declref.

I have the same concerns with this patch as I did with the initial one, see https://reviews.llvm.org/D84005#inline-775391 -- basically, we're doing a lookup here in SemaDeclAttr.cpp and saying "I found the identifier, everything's good", then storing the identifier into the attribute so we can look it up again later, at which point looking up the identifier may find something totally unrelated to what was found in SemaDeclAttr.cpp.

The attribute with declref is incompatible with Apple's SDKs, as the declref at the point of use in the attribute might not be available due to the availability annotations on the domain declaration.

Can you help me to understand why this isn't the expected behavior? If you name something that's not available, it seems surprising to me that it would then be available but only as one particular attribute argument.

Your argument makes sense. The problem right now is that clang doesn't allow the name to be used even when the user marks up availability correctly trying to guard the use of the declaration. This stems from the combination of this macro in Foundation:

#define NS_ERROR_ENUM(_type, _name, _domain)  \
  enum _name : _type _name; enum __attribute__((ns_error_domain(_domain))) _name : _type

and the fact that when the user uses it like this:

API_AVAILABLE(ios(14.0)) API_UNAVAILABLE(macos, tvos)
typedef NS_ERROR_ENUM(NIErrorDomain, NIErrorCode) {
    NIErrorA = 1,
};

which is equivalent to:

__attribute__((availability(ios,introduced=14.0))) __attribute__((availability(macos,unavailable))) __attribute__((availability(tvos,unavailable)))
NSErrorDomain const NIErrorDomain;

__attribute__((availability(ios,introduced=14.0))) __attribute__((availability(macos,unavailable))) __attribute__((availability(tvos,unavailable)))
typedef enum NIErrorCode : NSInteger NIErrorCode; enum __attribute__((ns_error_domain(NIErrorDomain))) NIErrorCode : NSInteger {
     NIErrorA = 1
};

In this case clang doesn't know about availability on the NIErrorDomain declaration as it's placed on the typedef, so it reports the diagnostic that NIErrorDomain is unavailable for macOS, even though from users perspective the use is guarded correctly by the API_UNAVAILABLE(macos, tvOS) before the NS_ERROR_ENUM macro.

Do you think in this case it would make sense to try to teach clang to reason about this by special casing the fact that the enum declaration should check if the typedef is annotated with the correct availability?

[arphaman]

In D98450#2699970, @arphaman wrote:

Sorry, getting back to this only now.

In D98450#2621907, @aaron.ballman wrote:

In D98450#2621877, @MForster wrote:

That's ok for me. I'd suggest to wait for Aaron's feedback, because IIRC it was his suggestion in the first place to go to declref.

I have the same concerns with this patch as I did with the initial one, see https://reviews.llvm.org/D84005#inline-775391 -- basically, we're doing a lookup here in SemaDeclAttr.cpp and saying "I found the identifier, everything's good", then storing the identifier into the attribute so we can look it up again later, at which point looking up the identifier may find something totally unrelated to what was found in SemaDeclAttr.cpp.

The attribute with declref is incompatible with Apple's SDKs, as the declref at the point of use in the attribute might not be available due to the availability annotations on the domain declaration.

Can you help me to understand why this isn't the expected behavior? If you name something that's not available, it seems surprising to me that it would then be available but only as one particular attribute argument.

Your argument makes sense. The problem right now is that clang doesn't allow the name to be used even when the user marks up availability correctly trying to guard the use of the declaration. This stems from the combination of this macro in Foundation:

#define NS_ERROR_ENUM(_type, _name, _domain)  \
  enum _name : _type _name; enum __attribute__((ns_error_domain(_domain))) _name : _type

and the fact that when the user uses it like this:

API_AVAILABLE(ios(14.0)) API_UNAVAILABLE(macos, tvos)
typedef NS_ERROR_ENUM(NIErrorDomain, NIErrorCode) {
    NIErrorA = 1,
};

which is equivalent to:

__attribute__((availability(ios,introduced=14.0))) __attribute__((availability(macos,unavailable))) __attribute__((availability(tvos,unavailable)))
NSErrorDomain const NIErrorDomain;

__attribute__((availability(ios,introduced=14.0))) __attribute__((availability(macos,unavailable))) __attribute__((availability(tvos,unavailable)))
typedef enum NIErrorCode : NSInteger NIErrorCode; enum __attribute__((ns_error_domain(NIErrorDomain))) NIErrorCode : NSInteger {
     NIErrorA = 1
};

In this case clang doesn't know about availability on the NIErrorDomain declaration as it's placed on the typedef, so it reports the diagnostic that NIErrorDomain is unavailable for macOS, even though from users perspective the use is guarded correctly by the API_UNAVAILABLE(macos, tvOS) before the NS_ERROR_ENUM macro.

Do you think in this case it would make sense to try to teach clang to reason about this by special casing the fact that the enum declaration should check if the typedef is annotated with the correct availability?

I'm thinking maybe we could have an attribute that transcribes the availability from the typedef to the enum, e.g.

#define NS_ERROR_ENUM(_type, _name, _domain)  \
  enum _name : _type _name; enum __attribute__((transcribe_availability(_name))) __attribute__((ns_error_domain(_domain))) _name : _type

in that case we could teach clang that the enum should take the availability attributes from the typedef and apply to the enum, which should solve the use problem in the ns_error_domain attribute. Or potentially clang could try to infer it without the new attribute just by detecting this code pattern. WDYT?

[aaron.ballman]

In D98450#2699970, @arphaman wrote:

Your argument makes sense. The problem right now is that clang doesn't allow the name to be used even when the user marks up availability correctly trying to guard the use of the declaration. This stems from the combination of this macro in Foundation:

#define NS_ERROR_ENUM(_type, _name, _domain)  \
  enum _name : _type _name; enum __attribute__((ns_error_domain(_domain))) _name : _type

and the fact that when the user uses it like this:

API_AVAILABLE(ios(14.0)) API_UNAVAILABLE(macos, tvos)
typedef NS_ERROR_ENUM(NIErrorDomain, NIErrorCode) {
    NIErrorA = 1,
};

Is there an argument missing to NS_ERROR_ENUM? I don't see it passing the domain.

which is equivalent to:

__attribute__((availability(ios,introduced=14.0))) __attribute__((availability(macos,unavailable))) __attribute__((availability(tvos,unavailable)))
NSErrorDomain const NIErrorDomain;

__attribute__((availability(ios,introduced=14.0))) __attribute__((availability(macos,unavailable))) __attribute__((availability(tvos,unavailable)))
typedef enum NIErrorCode : NSInteger NIErrorCode; enum __attribute__((ns_error_domain(NIErrorDomain))) NIErrorCode : NSInteger {
     NIErrorA = 1
};

In this case clang doesn't know about availability on the NIErrorDomain declaration as it's placed on the typedef, so it reports the diagnostic that NIErrorDomain is unavailable for macOS, even though from users perspective the use is guarded correctly by the API_UNAVAILABLE(macos, tvOS) before the NS_ERROR_ENUM macro.

Ah, thank you for the detailed explanation!

In D98450#2699990, @arphaman wrote:

I'm thinking maybe we could have an attribute that transcribes the availability from the typedef to the enum, e.g.

#define NS_ERROR_ENUM(_type, _name, _domain)  \
  enum _name : _type _name; enum __attribute__((transcribe_availability(_name))) __attribute__((ns_error_domain(_domain))) _name : _type

in that case we could teach clang that the enum should take the availability attributes from the typedef and apply to the enum, which should solve the use problem in the ns_error_domain attribute. Or potentially clang could try to infer it without the new attribute just by detecting this code pattern. WDYT?

I am mostly ignorant of the availability attributes, but do you think this functionality would be generally useful for effectively being able to say "make this declaration as available as this other declaration"? e.g., if you have a structure and a function that are used together, you can say:

struct __attribute__((availability(...))) S { ... };
__attribute__((transcribe_availability(S))) void func(struct S  s);

If the attribute is generally useful, then I think it would be better to add an explicit attribute. That also helps tools like static analyzers or AST matchers to make the connection between the availabilities, which would be harder if the information was inferred by the frontend.

clang/include/clang/Basic/Attr.td

let Args = [IdentifierArgument<"RelatedClass">,
           IdentifierArgument<"InstanceMethod">];
   let HasCustomParsing = 1;
   let Documentation = [Undocumented];
 }
 
 def NSErrorDomain : InheritableAttr {
   let Spellings = [GNU<"ns_error_domain">];
   let Subjects = SubjectList<[Enum], ErrorDiag>;
-  let Args = [DeclArgument<Var, "ErrorDomain">];
+  let Args = [IdentifierArgument<"ErrorDomain">];
   let Documentation = [NSErrorDomainDocs];
 }
 
 def NSReturnsRetained : DeclOrTypeAttr {
   let Spellings = [Clang<"ns_returns_retained">];
 //  let Subjects = SubjectList<[ObjCMethod, ObjCProperty, Function]>;
   let Documentation = [RetainBehaviorDocs];
 }

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_nsconsumed_attribute_mismatch : Error<
   "overriding method has mismatched ns_consumed attribute on its"
   " parameter">;
 def err_nsreturns_retained_attribute_mismatch : Error<
   "overriding method has mismatched ns_returns_%select{not_retained|retained}0"
   " attributes">;
 def err_nserrordomain_invalid_decl : Error<
   "domain argument %select{|%1 }0does not refer to global constant">;
-def err_nserrordomain_wrong_type : Error<
-  "domain argument %0 does not point to an NSString or CFString constant">;
 
 def warn_nsconsumed_attribute_mismatch : Warning<
   err_nsconsumed_attribute_mismatch.Text>, InGroup<NSConsumedMismatch>;
 def warn_nsreturns_retained_attribute_mismatch : Warning<
   err_nsreturns_retained_attribute_mismatch.Text>, InGroup<NSReturnsMismatch>;
 
 def note_getter_unavailable : Note<
   "or because setter is declared here, but no getter method %0 is found">;

clang/lib/Sema/SemaDeclAttr.cpp

S.Diag(D->getBeginLoc(), diag::warn_objc_requires_super_protocol) << Attrs
                                                                       << 1;
     return;
   }
 
   D->addAttr(::new (S.Context) ObjCRequiresSuperAttr(S.Context, Attrs));
 }
 
 static void handleNSErrorDomain(Sema &S, Decl *D, const ParsedAttr &AL) {
-  auto *E = AL.getArgAsExpr(0);
-  auto Loc = E ? E->getBeginLoc() : AL.getLoc();
+  IdentifierLoc *IdentLoc = AL.isArgIdent(0) ? AL.getArgAsIdent(0) : nullptr;
+  if (!IdentLoc || !IdentLoc->Ident) {
+    // Try to locate the argument directly
+    SourceLocation Loc = AL.getLoc();
+    if (AL.isArgExpr(0) && AL.getArgAsExpr(0))
+      Loc = AL.getArgAsExpr(0)->getBeginLoc();
 
-  auto *DRE = dyn_cast<DeclRefExpr>(AL.getArgAsExpr(0));
-  if (!DRE) {
     S.Diag(Loc, diag::err_nserrordomain_invalid_decl) << 0;
     return;
   }
 
-  auto *VD = dyn_cast<VarDecl>(DRE->getDecl());
-  if (!VD) {
-    S.Diag(Loc, diag::err_nserrordomain_invalid_decl) << 1 << DRE->getDecl();
+  // Verify that the identifier is a valid decl in the C decl namespace
+  LookupResult LR(S, DeclarationName(IdentLoc->Ident), SourceLocation(),
+                  Sema::LookupNameKind::LookupOrdinaryName);
+  if (!S.LookupName(LR, S.TUScope) || !LR.getAsSingle<VarDecl>()) {
+    S.Diag(IdentLoc->Loc, diag::err_nserrordomain_invalid_decl)
+        << 1 << IdentLoc->Ident;
     return;
   }
 
-  if (!isNSStringType(VD->getType(), S.Context) &&
-      !isCFStringType(VD->getType(), S.Context)) {
-    S.Diag(Loc, diag::err_nserrordomain_wrong_type) << VD;
-    return;
-  }
-
-  D->addAttr(::new (S.Context) NSErrorDomainAttr(S.Context, AL, VD));
+  D->addAttr(::new (S.Context)
+                 NSErrorDomainAttr(S.Context, AL, IdentLoc->Ident));
 }
 
 static void handleObjCBridgeAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   IdentifierLoc *Parm = AL.isArgIdent(0) ? AL.getArgAsIdent(0) : nullptr;
 
   if (!Parm) {
     S.Diag(D->getBeginLoc(), diag::err_objc_attr_not_id) << AL << 0;
     return;

clang/test/Sema/ns_error_enum.m

 extern CFErrorDomain const MyCFTypedefErrorDomain;
 typedef NS_ERROR_ENUM(unsigned char, MyCFTypedefErrorEnum, MyCFTypedefErrorDomain) {
   MyCFTypedefErrFirst,
   MyCFTypedefErrSecond,
 };
 
 extern char *const WrongErrorDomainType;
 enum __attribute__((ns_error_domain(WrongErrorDomainType))) MyWrongErrorDomainType { MyWrongErrorDomain };
-// expected-error@-1{{domain argument 'WrongErrorDomainType' does not point to an NSString or CFString constant}}
 
 struct __attribute__((ns_error_domain(MyErrorDomain))) MyStructWithErrorDomain {};
 // expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
 
 int __attribute__((ns_error_domain(MyErrorDomain))) NotTagDecl;
   // expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
 
 enum __attribute__((ns_error_domain())) NoArg { NoArgError };
 // expected-error@-1{{'ns_error_domain' attribute takes one argument}}
 
 enum __attribute__((ns_error_domain(MyErrorDomain, MyErrorDomain))) TwoArgs { TwoArgsError };
 // expected-error@-1{{'ns_error_domain' attribute takes one argument}}
 
 typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, InvalidDomain) {
-  // expected-error@-1{{use of undeclared identifier 'InvalidDomain'}}
+  // expected-error@-1{{domain argument 'InvalidDomain' does not refer to global constant}}
   MyErrFirstInvalid,
   MyErrSecondInvalid,
 };
 
 typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, "domain-string");
   // expected-error@-1{{domain argument does not refer to global constant}}
 
 void foo() {}
 typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalidFunction, foo);
   // expected-error@-1{{domain argument 'foo' does not refer to global constant}}