[Sema] Tweak merging of availability attributes

[aaron.ballman]

Summary

Two calls to mergeAvailabilityAttr dropped the original
AttributeCommonInfo and created a new one from just the range.
This new attribute would have a 0 kind (rather than AT_Availability)
and a 0 syntax (GNU). I don't have any proof that this makes a
difference in practice.

Noticed while doing some changes to the attribute handling.

[erichkeane]

I think the change is probably fine, but can you add an AST-dump test for this one?

[rsandifo-arm]

Add AST dump test

[rsandifo-arm]

In D147661#4248574, @erichkeane wrote:

I think the change is probably fine,

Thanks!

but can you add an AST-dump test for this one?

Sure. How does this look?

[erichkeane]

This test doesn't show a change in behavior. See:

https://godbolt.org/z/vrWM1bTPE

|-FunctionDecl 0x5555dbfa5a20 <<source>:3:1, col:60> col:6 foo 'void (void)'
| |-AvailabilityAttr 0x5555dbfa5ac8 <col:27, col:58> ios 2.0 0 0 "" "" 0
| `-AvailabilityAttr 0x5555dbfa5b80 <col:27, col:58> Implicit maccatalyst 13.1 0 0 "" "" 2
`-FunctionDecl 0x5555dbfa5c80 <line:4:1, col:55> col:6 bar 'void (void)'
  |-AvailabilityAttr 0x5555dbfa5d28 <col:19, col:50> ios 2.0 0 0 "" "" 0
  `-AvailabilityAttr 0x5555dbfa5de0 <col:19, col:50> Implicit maccatalyst 13.1 0 0 "" "" 2

[rsandifo-arm]

In D147661#4249004, @erichkeane wrote:

This test doesn't show a change in behavior.

Yeah. Is there a way to force the syntax and spelling fields to be dumped, even for implicit attributes? I was struggling to find one, which is why I wasn't sure this made a difference in practice.

The patch is supposed to be a minor clean-up, since similar calls in surrounding code uses AL rather than AL.getRange(). The reason I originally hit this was that the series I'm working on removes the:

AttributeCommonInfo(SourceRange)
AttributeCommonInfo(SourceLocation)

constructors (but keeps the ability to use FooAttr::CreateImplicit without more than a source range). It might be easier to explain why that seemed a good idea alongside the patches themselves.

The two main uses of these constructors outside tablegen-generated code were the two in this patch and the one in https://reviews.llvm.org/D147657.

[erichkeane]

In D147661#4249244, @rsandifo-arm wrote:

In D147661#4249004, @erichkeane wrote:

This test doesn't show a change in behavior.

Yeah. Is there a way to force the syntax and spelling fields to be dumped, even for implicit attributes? I was struggling to find one, which is why I wasn't sure this made a difference in practice.

The patch is supposed to be a minor clean-up, since similar calls in surrounding code uses AL rather than AL.getRange(). The reason I originally hit this was that the series I'm working on removes the:

AttributeCommonInfo(SourceRange)
AttributeCommonInfo(SourceLocation)

constructors (but keeps the ability to use FooAttr::CreateImplicit without more than a source range). It might be easier to explain why that seemed a good idea alongside the patches themselves.

The two main uses of these constructors outside tablegen-generated code were the two in this patch and the one in https://reviews.llvm.org/D147657.

Ah, hrm... I don't have a really good idea of how to test this now then. Typically you'd have to do manual dump logic for an attribute like this i think? Perhaps @aaron.ballman knows better. I see the value to this, but I guess I want to see if we can capture the change somehow in a test.

[aaron.ballman]

I don't think there's a way to test this change, unfortunately. However, I think this exposed a bigger concern, which is that AttributeCommonInfo has some single-argument constructors that are not explicit so that we don't run into this issue again. However, fixing that entails changing clang's tablegen, so I don't think it should block these changes.

[aaron.ballman]

In D147661#4249696, @aaron.ballman wrote:

I don't think there's a way to test this change, unfortunately. However, I think this exposed a bigger concern, which is that AttributeCommonInfo has some single-argument constructors that are not explicit so that we don't run into this issue again. However, fixing that entails changing clang's tablegen, so I don't think it should block these changes.

I threw together a patch to make the constructors explicit and the only two compile failures I have are with what you're fixing in this patch. If you'd like, I can commandeer this patch and subsume it with the larger refactor. Alternatively, we can land this (I'd drop the test though) and I can rebase on top of your changes. Either is fine by me.

[rsandifo-arm]

I threw together a patch to make the constructors explicit and the only two compile failures I have are with what you're fixing in this patch. If you'd like, I can commandeer this patch and subsume it with the larger refactor. Alternatively, we can land this (I'd drop the test though) and I can rebase on top of your changes. Either is fine by me.

Yeah, please feel free to commandeer it. My original motivation for doing this was to remove the single-argument constructors rather than keep them. However, I agree that making them explicit in the meantime would ensure forward progress, if what I'm doing in the follow-on patches turns out not to be acceptable.

[aaron.ballman]

In D147661#4255043, @rsandifo-arm wrote:

I threw together a patch to make the constructors explicit and the only two compile failures I have are with what you're fixing in this patch. If you'd like, I can commandeer this patch and subsume it with the larger refactor. Alternatively, we can land this (I'd drop the test though) and I can rebase on top of your changes. Either is fine by me.

Yeah, please feel free to commandeer it. My original motivation for doing this was to remove the single-argument constructors rather than keep them. However, I agree that making them explicit in the meantime would ensure forward progress, if what I'm doing in the follow-on patches turns out not to be acceptable.

Okay, thanks! I think getting rid of these constructors will be a bit of a challenge because of how many implicit attributes are created internally, but at the same time, I would not be sad if they disappeared either.

I'm commandeering this revision and will cite it in the commit message when I land the patch so there's a paper trail explaining how we got to those changes.

clang/include/clang/Basic/Attr.td

def Annotate : InheritableParamAttr {
   // Ensure that the annotate attribute can be used with
   // '#pragma clang attribute' even though it has no subject list.
   let AdditionalMembers = [{
   static AnnotateAttr *Create(ASTContext &Ctx, llvm::StringRef Annotation, \
               const AttributeCommonInfo &CommonInfo) {
     return AnnotateAttr::Create(Ctx, Annotation, nullptr, 0, CommonInfo);
   }
   static AnnotateAttr *CreateImplicit(ASTContext &Ctx, llvm::StringRef Annotation, \
-              const AttributeCommonInfo &CommonInfo = {SourceRange{}}) {
+              const AttributeCommonInfo &CommonInfo = AttributeCommonInfo{SourceRange{}}) {
     return AnnotateAttr::CreateImplicit(Ctx, Annotation, nullptr, 0, CommonInfo);
   }
   }];
   let PragmaAttributeSupport = 1;
   let AcceptsExprPack = 1;
   let Documentation = [Undocumented];
 }
 

clang/include/clang/Basic/AttributeCommonInfo.h

private:
   /// Corresponds to the Syntax enum.
   unsigned SyntaxUsed : 4;
   unsigned SpellingIndex : 4;
 
 protected:
   static constexpr unsigned SpellingNotCalculated = 0xf;
 
 public:
-  AttributeCommonInfo(SourceRange AttrRange)
+  explicit AttributeCommonInfo(SourceRange AttrRange)
       : AttrRange(AttrRange), ScopeLoc(), AttrKind(0), SyntaxUsed(0),
         SpellingIndex(SpellingNotCalculated) {}
 
-  AttributeCommonInfo(SourceLocation AttrLoc)
+  explicit AttributeCommonInfo(SourceLocation AttrLoc)
       : AttrRange(AttrLoc), ScopeLoc(), AttrKind(0), SyntaxUsed(0),
         SpellingIndex(SpellingNotCalculated) {}
 
   AttributeCommonInfo(const IdentifierInfo *AttrName,
                       const IdentifierInfo *ScopeName, SourceRange AttrRange,
                       SourceLocation ScopeLoc, Syntax SyntaxUsed)
       : AttrName(AttrName), ScopeName(ScopeName), AttrRange(AttrRange),
         ScopeLoc(ScopeLoc),

clang/lib/Sema/SemaDeclAttr.cpp

if (NewII) {
         if (V.empty())
           return V;
         if (V.getMajor() < 13 ||
             (V.getMajor() == 13 && V.getMinor() && *V.getMinor() < 1))
           return VersionTuple(13, 1); // The min Mac Catalyst version is 13.1.
         return V;
       };
       AvailabilityAttr *NewAttr = S.mergeAvailabilityAttr(
-          ND, AL.getRange(), NewII, true /*Implicit*/,
+          ND, AL, NewII, true /*Implicit*/,
           MinMacCatalystVersion(Introduced.Version),
           MinMacCatalystVersion(Deprecated.Version),
           MinMacCatalystVersion(Obsoleted.Version), IsUnavailable, Str,
           IsStrict, Replacement, Sema::AMK_None,
           PriorityModifier + Sema::AP_InferredFromOtherPlatform);
       if (NewAttr)
         D->addAttr(NewAttr);
     } else if (II->getName() == "macos" && GetSDKInfo() &&
if (NewII) {
                                     NewObsoleted =
                                         RemapMacOSVersion(Obsoleted.Version);
         if (NewIntroduced || NewDeprecated || NewObsoleted) {
           auto VersionOrEmptyVersion =
               [](const std::optional<VersionTuple> &V) -> VersionTuple {
             return V ? *V : VersionTuple();
           };
           AvailabilityAttr *NewAttr = S.mergeAvailabilityAttr(
-              ND, AL.getRange(), NewII, true /*Implicit*/,
+              ND, AL, NewII, true /*Implicit*/,
               VersionOrEmptyVersion(NewIntroduced),
               VersionOrEmptyVersion(NewDeprecated),
               VersionOrEmptyVersion(NewObsoleted), /*IsUnavailable=*/false, Str,
               IsStrict, Replacement, Sema::AMK_None,
               PriorityModifier + Sema::AP_InferredFromOtherPlatform +
                   Sema::AP_InferredFromOtherPlatform);
           if (NewAttr)
             D->addAttr(NewAttr);

clang/utils/TableGen/ClangAttrEmitter.cpp

for (const auto *Attr : Attrs) {
     if (Header)
       OS << SpellingEnum;
 
     const auto &ParsedAttrSpellingItr = llvm::find_if(
         AttrMap, [R](const std::pair<std::string, const Record *> &P) {
           return &R == P.second;
         });
 
+    enum class CreateKind {
+      WithAttributeCommonInfo,
+      WithSourceRange,
+      WithNoArgs,
+    };
+
     // Emit CreateImplicit factory methods.
-    auto emitCreate = [&](bool Implicit, bool DelayedArgsOnly, bool emitFake) {
+    auto emitCreate = [&](bool Implicit, bool DelayedArgsOnly,
+                          bool emitFake, CreateKind Kind) {
       if (Header)
         OS << "  static ";
       OS << R.getName() << "Attr *";
       if (!Header)
         OS << R.getName() << "Attr::";
       OS << "Create";
       if (Implicit)
         OS << "Implicit";
       if (DelayedArgsOnly)
         OS << "WithDelayedArgs";
       OS << "(";
       OS << "ASTContext &Ctx";
       if (!DelayedArgsOnly) {
         for (auto const &ai : Args) {
           if (ai->isFake() && !emitFake)
             continue;
           OS << ", ";
           ai->writeCtorParameters(OS);
         }
       } else {
         OS << ", ";
         DelayedArgs->writeCtorParameters(OS);
       }
+      if (Kind == CreateKind::WithAttributeCommonInfo)
         OS << ", const AttributeCommonInfo &CommonInfo";
-      if (Header && Implicit)
-        OS << " = {SourceRange{}}";
+      else if (Kind == CreateKind::WithSourceRange)
+        OS << ", SourceRange R";
       OS << ")";
       if (Header) {
         OS << ";\n";
         return;
       }
 
       OS << " {\n";
       OS << "  auto *A = new (Ctx) " << R.getName();
+      if (Kind == CreateKind::WithAttributeCommonInfo)
         OS << "Attr(Ctx, CommonInfo";
+      else if (Kind == CreateKind::WithSourceRange)
+        OS << "Attr(Ctx, AttributeCommonInfo{R}";
+      else if (Kind == CreateKind::WithNoArgs)
+        OS << "Attr(Ctx, AttributeCommonInfo{SourceLocation{}}";
+
       if (!DelayedArgsOnly) {
         for (auto const &ai : Args) {
           if (ai->isFake() && !emitFake)
             continue;
           OS << ", ";
           ai->writeImplicitCtorArgs(OS);
         }
       }
auto emitCreateNoCI = [&](bool Implicit, bool DelayedArgsOnly,
       } else {
         OS << ", ";
         DelayedArgs->writeImplicitCtorArgs(OS);
       }
       OS << ", I);\n";
       OS << "}\n\n";
     };
 
+    auto emitBothImplicitAndNonCreates = [&](bool DelayedArgsOnly,
+                                             bool emitFake, CreateKind Kind) {
+      emitCreate(true, DelayedArgsOnly, emitFake, Kind);
+      emitCreate(false, DelayedArgsOnly, emitFake, Kind);
+    };
+
     auto emitCreates = [&](bool DelayedArgsOnly, bool emitFake) {
-      emitCreate(true, DelayedArgsOnly, emitFake);
-      emitCreate(false, DelayedArgsOnly, emitFake);
+      emitBothImplicitAndNonCreates(DelayedArgsOnly, emitFake,
+                                    CreateKind::WithNoArgs);
+      emitBothImplicitAndNonCreates(DelayedArgsOnly, emitFake,
+                                    CreateKind::WithAttributeCommonInfo);
+      emitBothImplicitAndNonCreates(DelayedArgsOnly, emitFake,
+                                    CreateKind::WithSourceRange);
       emitCreateNoCI(true, DelayedArgsOnly, emitFake);
       emitCreateNoCI(false, DelayedArgsOnly, emitFake);
     };
 
     if (Header)
       OS << "  // Factory methods\n";
 
     // Emit a CreateImplicit that takes all the arguments.