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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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#260 , @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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98450/new/

https://reviews.llvm.org/D98450

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98450/new/

https://reviews.llvm.org/D98450

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

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;
  
  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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98450/new/

https://reviews.llvm.org/D98450

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98450/new/

https://reviews.llvm.org/D98450

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Michael Forster via Phabricator via cfe-commits]

MForster added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98450/new/

https://reviews.llvm.org/D98450

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98450/new/

https://reviews.llvm.org/D98450

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman created this revision.
arphaman added reviewers: steven_wu, MForster.
Herald added subscribers: ributzka, jkorous.
Herald added a reviewer: aaron.ballman.
arphaman requested review of this revision.

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.


https://reviews.llvm.org/D98450

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Sema/ns_error_enum.m


Index: clang/test/Sema/ns_error_enum.m
===
--- clang/test/Sema/ns_error_enum.m
+++ clang/test/Sema/ns_error_enum.m
@@ -53,7 +53,6 @@
 
 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}}
@@ -68,7 +67,7 @@
 // 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,
 };
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -5510,28 +5510,28 @@
 }
 
 static void handleNSErrorDomain(Sema , Decl *D, const ParsedAttr ) {
-  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(AL.getArgAsExpr(0));
-  if (!DRE) {
 S.Diag(Loc, diag::err_nserrordomain_invalid_decl) << 0;
 return;
   }
 
-  auto *VD = dyn_cast(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()) {
+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 , Decl *D, const ParsedAttr ) {
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -9683,8 +9683,6 @@
   " 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;
Index: clang/include/clang/Basic/Attr.td
===
--- clang/include/clang/Basic/Attr.td
+++ clang/include/clang/Basic/Attr.td
@@ -1967,7 +1967,7 @@
 def NSErrorDomain : InheritableAttr {
   let Spellings = [GNU<"ns_error_domain">];
   let Subjects = SubjectList<[Enum], ErrorDiag>;
-  let Args = [DeclArgument];
+  let Args = [IdentifierArgument<"ErrorDomain">];
   let Documentation = [NSErrorDomainDocs];
 }
 


Index: clang/test/Sema/ns_error_enum.m
===
--- clang/test/Sema/ns_error_enum.m
+++ clang/test/Sema/ns_error_enum.m
@@ -53,7 +53,6 @@
 
 extern char *const