Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

if (!@available) doesn't work and has confusing diagnostic #32786

Open Quuxplusone opened 7 years ago

Quuxplusone commented 7 years ago
Bugzilla Link PR33814
Status NEW
Importance P enhancement
Reported by Nico Weber (nicolasweber@gmx.de)
Reported on 2017-07-17 07:32:36 -0700
Last modified on 2018-08-27 22:46:39 -0700
Version unspecified
Hardware PC Linux
CC arphaman@gmail.com, erik.pilkington@gmail.com, llvm-bugs@lists.llvm.org, rodger.combs@gmail.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

We're using @available some in chrome, and I'm getting feedback that people are confused about writing if (!@available. That's kind of understandable, here's the current experience:

thakis@thakis:~/src/chrome/src$ cat test.mm void f() { if (!@available(macos 10.10, )) { return; } // 10.10-only code here } thakis@thakis:~/src/chrome/src$ third_party/llvm-build/Release+Asserts/bin/clang -c test.mm -target x86_64-apple-darwin test.mm:2:8: warning: @available does not guard availability here; use if (@available) instead [-Wunsupported-availability-guard] if (!@available(macos 10.10, )) { ^

It'd be cool if this pattern just worked somehow (full CFG-awareness likely isn't needed).

Even just if (!@available()) {} else {} so that the else block gets the available OS would be nice.

If that isn't feasible, making the error message more explicit might help.

(But maybe just having the docs is good enough too, most people who are confused about this haven't read the docs we added on Friday yet.)

Quuxplusone commented 7 years ago

This was actually the subject of a long internal bikeshed, I was in favor of using the full CFG, but many wanted to keep the check really simple and lexical.

The problem with supporting early exits like this is that it implies that this check is more powerful than it actually is, which would likely make things even more confusing. There is probably a slippery slope argument "If this works then why doesn't that work?" that permitting early exits might start. I agree that the current implementation falls down a bit here though, if (!@available()) {} else {...} seems like it could be reasonable though.

Quuxplusone commented 6 years ago
I tried doing something similar with __builtin_available in C++ code. My case
wasn't actually guarding any API usage; I was just returning false early pre-
10.11 in a "check if feature [not API] is available" function. I ended up doing
this:

if (__builtin_available(macOS 10.11, *)) {
  // Dummy branch
} else {
  // [log about how we're bailing on the check]
  return false;
}

That quashed the Wunsupported-availability-guard warning, and seems to generate
the expected code, but it's awfully weird to have to do.