[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 <https://reviews.llvm.org/D98450#2699970>, @arphaman wrote:

> Sorry, getting back to this only now.
>
> In D98450#2621907 <https://reviews.llvm.org/D98450#2621907>, @aaron.ballman wrote:
>
>> In D98450#2621877 <https://reviews.llvm.org/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