This is an archive of the discontinued LLVM Phabricator instance.

[ObjC Availability] Warn upon unguarded use of partially available declaration
ClosedPublic

Authored by erik.pilkington on Jul 31 2016, 4:28 PM.

Details

Summary

This patch implements support for diagnostics for @available violations. This is done with a traversal of the AST after semantic analysis of a function where a partially available (based on availability attributes) use of a declaration is found. For example:

if (@available(macos 10.12, *))
  fn_10_12();
else
  fn_10_12(); // clang now emits a warning here

This patch also treats both branches of if (@available(...)) checks as begin protected; being able to goto into them would break this feature. Also, -Wpartial-availability is now an alias of -Wunguarded-availability.

Up next is a patch to implement clang CodeGen support for ObjCAvailabilityCheckExpr, and another to provide fixits to suggest using @available.

This patch is part of a series that implements the feature I proposed here:
http://lists.llvm.org/pipermail/cfe-dev/2016-July/049851.html

Diff Detail

Repository
rL LLVM

Event Timeline

erik.pilkington retitled this revision from to [ObjC Availability] Warn upon unguarded use of partially available declaration.
erik.pilkington updated this object.
erik.pilkington added a subscriber: cfe-commits.
dcoughlin edited edge metadata.Aug 1 2016, 1:59 PM

It's really great to see this!

lib/Sema/SemaExpr.cpp
15200 ↗(On Diff #66256)

In Swift we don't warn for this scenario and don't require the programmer to explicitly account for the current platform. The rationale behind '*' was to make platform bringup easier. Because new platforms are typically forked from existing platforms, treating '*' as the minimum deployment target on the current platform would typically do the "right thing" on new platforms. In cases where it doesn't, the user would still get a diagnostic if she tried to use a potentially unavailable API.

If I understand it correctly, with the behavior here anyone porting code to a new platform will get a warning for each @available. Is this what you want?

erik.pilkington edited edge metadata.

This new patch removes the warning diag::warn_available_using_star_case, as per Devin's suggestion.

dcoughlin added inline comments.Aug 2 2016, 12:35 PM
lib/Sema/SemaDeclAttr.cpp
6611 ↗(On Diff #66501)

Do you still want to suppress warnings in the 'then' branch for '*'? I think there is potential value in pointing out uses of APIs that are not available on the minimum deployment target for the '*' case.

@dcoughlin: Yes, good point!

This new patch uses the enclosing version to diagnose both branches of the '*' case, as opposed to ignoring them.

manmanren edited edge metadata.Aug 4 2016, 12:03 PM

Hi Erik,

Thanks for working on this! It is great to see these patches coming.

Manman

include/clang/Sema/Sema.h
9608 ↗(On Diff #66555)

Can you explain the inputs and outputs?

lib/Sema/SemaDeclAttr.cpp
6517 ↗(On Diff #66555)

Can you add a high level description of what this class is doing to issue unguarded diagnostics?

6540 ↗(On Diff #66555)

Do we cover all usage of decls with the above three member functions?

6552 ↗(On Diff #66555)

It is easier to read if you add comment for the boolean argument.

6554 ↗(On Diff #66555)

Can you mention where they are handled?

lib/Sema/SemaExpr.cpp
110 ↗(On Diff #66555)

This looks like a refactoring of DiagnoseAvailabilityOfDecl. Is it possible to commit as a NFC patch?

test/SemaObjC/unguarded-availability.m
93 ↗(On Diff #66555)

Should we emit error here? Or should we just ignore the tighter availability scope?

erik.pilkington edited edge metadata.

This new patch rebases on top of r277887, and addresses Manman's comments.

After looking through the AST nodes, I realized I neglected to emit a diagnostic for member expressions. I also noticed that function parameters are not being handled correctly. For example:

int f(introduced_in_10_12_t x) __attribute__((availability(macos, introduced=10.12)));

Triggers -Wunguarded-availability, which is clearly incorrect. I think the fix here is just to delay the diagnostic until we know the context availability. This isn't really related to this patch, so I'll fix it in a follow up.

Outside of that I can't think of any cases of referencing a declaration in a function body that we're missing.

Thanks!

This looks pretty good. Can you add a few more testing cases for templates such as @available inside the template function, @available enclosing the template instantiation?

Cheers,
Manman

lib/Sema/SemaDeclAttr.cpp
6634 ↗(On Diff #67582)

I wonder if it is better to combine this with the above if (!E->hasVersion()) i.e
if (!E->hasVersion() || E->getVersion() <= AvailabilityStack.back())

This new patch adds some testcases for templates, and simplifies some control flow in TraverseIfStmt. This patch also removes diagnostics for instantiated templates, instead just using the pattern. This means that a dependent type that was resolved during instantiation can refer to an unavailable declaration. For example, the following now compiles cleanly (when we want a diagnostic):

void f(int);
void f(char) __attribute__((availability(macos, introduced=10.12)));

template <class T> void use_f(T p) { f(p); }

template void use_f<char>();

This is done so containers, such as vector<partially_available> can be used safely provided partially_available is safe at the point of instantiation. I think the way to improve this is in Sema::getVersionForDecl(). There we could look at function and template parameters to determine the best base version, then diagnose against it.

manmanren accepted this revision.Aug 15 2016, 3:25 PM
manmanren edited edge metadata.

This is done so containers, such as vector<partially_available> can be used safely provided partially_available is safe at the point of instantiation. I think the way to improve this is in Sema::getVersionForDecl(). There we could look at function and template parameters to determine the best base version, then diagnose against it.

Let's fix the diagnostics for dependent type in a follow-up patch. And your suggestion above sounds reasonable to me.

Thanks for the work!
Manman

This revision is now accepted and ready to land.Aug 15 2016, 3:25 PM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Oct 26 2016, 8:44 AM

Erik, are you planning on adding the later phases soon? At the moment, due to this patch, clang emits "enclose 'setAllowsAutomaticWindowTabbing:' in an @available check to silence this warning" but then errors out when trying to use @available. This is proving to be confusing to folks :-)