Enable unguarded availability diagnostic on instantiated template functions

[hekota]

Availability diagnostic in instantiated template functions was intentionally skipped in the original commit years ago with a FIXME note.

I ran into this when working on diagnostics for HLSL. When I remove the skip, it seems to be working just fine outputting expected messages. So, unless I am missing something, I would keep it enabled and use it for checking availability in HLSL templates as well.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Helena Kotas (hekota)

Changes

Availability diagnostic in instantiated template functions was intentionally skipped in the original commit years ago with a FIXME note.

I ran into this when working on diagnostics for HLSL. When I remove the skip, it seems to be working just fine outputting expected messages. So, unless I am missing something, I would keep it enabled and use it for checking availability in HLSL templates as well.

---

Full diff: https://github.com/llvm/llvm-project/pull/91699.diff

2 Files Affected:

• (modified) clang/lib/Sema/SemaAvailability.cpp (-5)
• (modified) clang/test/SemaObjC/unguarded-availability.m (+7-4)

diff --git a/clang/lib/Sema/SemaAvailability.cpp b/clang/lib/Sema/SemaAvailability.cpp
index 846a31a796730..5b0f34d1aa3aa 100644
--- a/clang/lib/Sema/SemaAvailability.cpp
+++ b/clang/lib/Sema/SemaAvailability.cpp
@@ -928,11 +928,6 @@ void Sema::DiagnoseUnguardedAvailabilityViolations(Decl *D) {
   Stmt *Body = nullptr;
 
   if (auto *FD = D->getAsFunction()) {
-    // FIXME: We only examine the pattern decl for availability violations now,
-    // but we should also examine instantiated templates.
-    if (FD->isTemplateInstantiation())
-      return;
-
     Body = FD->getBody();
 
     if (auto *CD = dyn_cast<CXXConstructorDecl>(FD))
diff --git a/clang/test/SemaObjC/unguarded-availability.m b/clang/test/SemaObjC/unguarded-availability.m
index d0e23eabcb598..3a38c3b40c4d6 100644
--- a/clang/test/SemaObjC/unguarded-availability.m
+++ b/clang/test/SemaObjC/unguarded-availability.m
@@ -177,16 +177,19 @@ void justAtAvailable(void) {
 
 #ifdef OBJCPP
 
-int f(char) AVAILABLE_10_12;
+int f(char) AVAILABLE_10_12; // #f_char_def
 int f(int);
 
 template <class T> int use_f() {
-  // FIXME: We should warn here!
-  return f(T());
+  // expected-warning@#f_call {{'f' is only available on macOS 10.12 or newer}}
+  // expected-note@#f_char_inst {{in instantiation of function template specialization 'use_f<char>' requested here}}
+  // expected-note@#f_char_def {{'f' has been marked as being introduced in macOS 10.12 here, but the deployment target is macOS 10.9}}
+  // expected-note@#f_call {{enclose 'f' in an @available check to silence this warning}}
+  return f(T()); // #f_call
 }
 
 int a = use_f<int>();
-int b = use_f<char>();
+int b = use_f<char>(); // #f_char_inst
 
 template <class> int use_at_available() {
   if (@available(macos 10.12, *))

[coopp]

Looks fine to me. I assume this doesn't impact other test passes right? Now there is work being done when before it was doing an early out for template instantiations.

[epilk]

Could you check that we don't emit a warning if there is an availability attribute on the enclosing function or if the use is guarded by an if (@available(...)) check? IIRC that that was what I was concerned about when I wrote this. @jansvoboda11 : someone at Apple should probably review this.

[hekota]

Thanks for the quick response! I have added the test cases you requested. No additional warnings are emitted.

[llvm-beanz]

Tagging some Apple people to get an extra set of eyes here:

(cc: @jansvoboda11, @hyp, @cyndyishida, @ributzka, @rjmccall)

[llvm-beanz]

We've waited a week on this PR to see if anyone from Apple will chime in. On previous PRs we waited weeks and got no response. We've reached out on Discord too.

This PR looks sane to me and it seems to have adequate testing. I think we should move forward and if Apple comes out of the woodwork and has an issue we can revert or move swiftly to address it.

[hekota]

Thanks Chris!

[nico]

Any chance to put these behind a subgroup? This now fires in a bunch of places where it didn't before.

[llvm-beanz]

@nico, am I correct to assume those aren't false positives, just a bunch of places that didn't warn before?

[amykhuang]

here's a reduced example of what we ran into in Chrome. I guess the attribute is being applied to a function that's passed to f(), and the warning points to the call in the definition of f.

#define a(b) __attribute__((__availability__(android, introduced = b)))
#define c(b) (a(b), apply_to = function)

template <typename callback> void f(callback cb) {
  cb();
}

#pragma clang attribute push c(29)
void test() {
  f([](){});
}
#pragma clang attribute pop

[amykhuang]

nevermind, that example is working as intended