[Attributes]: refactor to expose ParsedAttrInfo::acceptsLangOpts. NFC

[sammccall]

Summary

We will use this function to filter code completion of attributes.

[aaron.ballman]

clang/include/clang/Sema/ParsedAttr.h

96

Plugin attributes inherit from ParsedAttrInfo, not ParsedAttr, so one downside to this change is that plugin authors no longer have a way to diagnose language options with a custom diagnostic; all they can get is "attribute ignored".

Perhaps another approach is to add an output parameter so the overrider can signify whether the language options are valid or not (because it's plausible that the plugin wants to diagnose the language options but they're still valid enough that the attribute should be accepted)?

[sammccall]

clang/include/clang/Sema/ParsedAttr.h

96

Plugin attributes inherit from ParsedAttrInfo, not ParsedAttr, so one downside to this change is that plugin authors no longer have a way to diagnose language options with a custom diagnostic; all they can get is "attribute ignored".

This is less flexible, indeed. What's not clear to me is:

• do we know of any real plugins where the ability to use a custom diagnostic here is important? Hypothetical flexibility may not be worth keeping.
• if custom diagnostics are needed, can they be emitted when the attribute is handled instead?
• why we'd need this flexibility for LangOpts but not Target (existsInTarget)

Perhaps another approach is to add an output parameter so the overrider can signify whether the language options are valid or not (because it's plausible that the plugin wants to diagnose the language options but they're still valid enough that the attribute should be accepted)?

diagLangOpts already returns bool.

The blocking issue with the diagLangOpts signature for code-completion purposes isn't actually the diagnostics (completion suppresses them), it's rather that you have to construct a ParsedAttr in order to emit them, which isn't easy to do "out of thin air".

[aaron.ballman]

clang/include/clang/Sema/ParsedAttr.h

96

do we know of any real plugins where the ability to use a custom diagnostic here is important? Hypothetical flexibility may not be worth keeping.

We don't know how plugins are being used and we have to assume Hryum's Law bites us just as much as anyone else. I'm fine potentially breaking plugins so long as we're still leaving them with the ability to do what they were previously doing. However, @john.brawn (the attribute plugin author) might know more here on the usage front.

if custom diagnostics are needed, can they be emitted when the attribute is handled instead?

Good question! I think that might work -- they get access to Sema and so they can check the language options and produce diagnostics from there. It might mean slight behavioral changes (if we used to bail early on a lang opt mismatch, it means we might get different diagnostics by the delay until the attribute is handled), but I think those are reasonable.

why we'd need this flexibility for LangOpts but not Target (existsInTarget)

Targets are a bit different -- you're in one target and that's it (so you either support the attribute for that target or not), but language options can have interactions between them (attribute may require something like CUDA to be enabled and it may also require some other CUDA-specific option to be enabled, and it wants to produce a nice diagnostic about that rather than just saying the attribute is ignored).

The blocking issue with the diagLangOpts signature for code-completion purposes isn't actually the diagnostics (completion suppresses them), it's rather that you have to construct a ParsedAttr in order to emit them, which isn't easy to do "out of thin air".

Ahh, I had missed that.

Given that the user can still do custom diagnostics when handling the attribute itself, I think this change is reasonable as-is. However, I think you should add a mention to the release notes that 1) the interface changed for plugin authors, and 2) the new approach to custom diagnostics for language options is to diagnose them when handling the attribute.

[sammccall]

Add release notes

clang/include/clang/Sema/ParsedAttr.h

96

We don't know how plugins are being used and we have to assume Hryum's Law bites us just as much as anyone else. I'm fine potentially breaking plugins so long as we're still leaving them with the ability to do what they were previously doing. However, @john.brawn (the attribute plugin author) might know more here on the usage front.

Thanks! John, please let me know if you see this causing a problem - I'm sure we can find a solution but it's easier to find a good one if we know the requirements :-)

(This makes me curious: officially, APIs are unstable and Hyrum's law is Hyrum's problem. In practice, we certainly have the idea that if people actually use something out-of-tree, it shouldn't be dropped without a high level replacement. I haven't seen this written down, maybe it's just common sense).

It might mean slight behavioral changes (if we used to bail early on a lang opt mismatch, it means we might get different diagnostics by the delay until the attribute is handled), but I think those are reasonable.

Yeah, it's not as nice and declarative but it's not common as far as we know.

I think you should add a mention to the release notes that 1) the interface changed for plugin authors, and 2) the new approach to custom diagnostics for language options is to diagnose them when handling the attribute.

Sounds good, I'll add that to this patch.

[aaron.ballman]

LGTM! If @john.brawn has concerns, we can address them post commit.

clang/include/clang/Sema/ParsedAttr.h

96

(This makes me curious: officially, APIs are unstable and Hyrum's law is Hyrum's problem. In practice, we certainly have the idea that if people actually use something out-of-tree, it shouldn't be dropped without a high level replacement. I haven't seen this written down, maybe it's just common sense).

I don't know if we document this for Clang plugins (we don't seem to for attributes anyway), but my expectation is that the API is not stable (and certainly not the ABI) with the exception of dot releases (e.g., we shouldn't break APIs between X.0 and X.0.1 releases, and we should probably be skeptical of breaking ABIs as well, but everything is fair game between X.0 and Y.0 releases). However, once we put some ability out to the public and say "you can do these things", we should be thoughtful about removing the ability. Adding some actual documentation (of whatever guarantees the community cares to give, if any) would be useful IMO. (Though certainly nothing to do with this review.)

clang/docs/ReleaseNotes.rst

 New Pragmas in Clang
 --------------------
 
 - ...
 
 Attribute Changes in Clang
 --------------------------
 
-- ...
+- Attributes loaded as clang plugins which are sensitive to LangOpts must
+  now override ``acceptsLangOpts`` instead of ``diagLangOpts``.
+  Returning false will produce a generic "attribute ignored" diagnostic, as
+  with clang's built-in attributes.
+  If plugins want to provide richer diagnostics, they can do so when the
+  attribute is handled instead, e.g. in ``handleDeclAttribute``.
+  (This was changed in order to better support attributes in code completion).
 
 Windows Support
 ---------------
 
 C Language Changes in Clang
 ---------------------------
 
 - ...

clang/include/clang/Sema/ParsedAttr.h

virtual bool diagAppertainsToStmt(Sema &S, const ParsedAttr &Attr,
     return true;
   }
   /// Check if the given attribute is mutually exclusive with other attributes
   /// already applied to the given declaration.
   virtual bool diagMutualExclusion(Sema &S, const ParsedAttr &A,
                                    const Decl *D) const {
     return true;
   }
-  /// Check if this attribute is allowed by the language we are compiling, and
-  /// issue a diagnostic if not.
-  virtual bool diagLangOpts(Sema &S, const ParsedAttr &Attr) const {
-    return true;
-  }
+  /// Check if this attribute is allowed by the language we are compiling.
+  virtual bool acceptsLangOpts(const LangOptions &LO) const { return true; }
+
   /// Check if this attribute is allowed when compiling for the given target.
   virtual bool existsInTarget(const TargetInfo &Target) const {
     return true;
   }
   /// Convert the spelling index of Attr to a semantic spelling enum value.
   virtual unsigned
   spellingIndexToSemanticSpelling(const ParsedAttr &Attr) const {
     return UINT_MAX;

clang/lib/Sema/ParsedAttr.cpp

 void ParsedAttr::getMatchRules(
     const LangOptions &LangOpts,
     SmallVectorImpl<std::pair<attr::SubjectMatchRule, bool>> &MatchRules)
     const {
   return getInfo().getPragmaAttributeMatchRules(MatchRules, LangOpts);
 }
 
 bool ParsedAttr::diagnoseLangOpts(Sema &S) const {
-  return getInfo().diagLangOpts(S, *this);
+  if (getInfo().acceptsLangOpts(S.getLangOpts()))
+    return true;
+  S.Diag(getLoc(), diag::warn_attribute_ignored) << *this;
+  return false;
 }
 
 bool ParsedAttr::isTargetSpecificAttr() const {
   return getInfo().IsTargetSpecific;
 }
 
 bool ParsedAttr::isTypeAttr() const { return getInfo().IsType; }
 

clang/utils/TableGen/ClangAttrEmitter.cpp

 static void GenerateLangOptRequirements(const Record &R,
                                         raw_ostream &OS) {
   // If the attribute has an empty or unset list of language requirements,
   // use the default handler.
   std::vector<Record *> LangOpts = R.getValueAsListOfDefs("LangOpts");
   if (LangOpts.empty())
     return;
 
-  OS << "bool diagLangOpts(Sema &S, const ParsedAttr &Attr) ";
-  OS << "const override {\n";
-  OS << "  auto &LangOpts = S.LangOpts;\n";
-  OS << "  if (" << GenerateTestExpression(LangOpts) << ")\n";
-  OS << "    return true;\n\n";
-  OS << "  S.Diag(Attr.getLoc(), diag::warn_attribute_ignored) ";
-  OS << "<< Attr;\n";
-  OS << "  return false;\n";
+  OS << "bool acceptsLangOpts(const LangOptions &LangOpts) const override {\n";
+  OS << "  return " << GenerateTestExpression(LangOpts) << ";\n";
   OS << "}\n\n";
 }
 
 static void GenerateTargetRequirements(const Record &Attr,
                                        const ParsedAttrMap &Dupes,
                                        raw_ostream &OS) {
   // If the attribute is not a target specific attribute, use the default
   // target handler.