Page MenuHomePhabricator

[Sema] Always search the full function scope context if a potential availability violation is encountered

Authored by logan-5 on May 12 2021, 10:52 AM.



Always search the full function scope context if a potential availability violation is encountered. This fixes both and

Previously, lambdas inside functions would mark their own bodies for later analysis when encountering a potentially unavailable decl, without taking into consideration that the entire lambda itself might be correctly guarded inside an @available check. The same applied to inner class member functions. Blocks happened to work as expected already, since Sema::getEnclosingFunction() skips through block scopes.

This patch instead simply and conservatively marks the entire outermost function scope for search, and removes some special-case logic that prevented DiagnoseUnguardedAvailabilityViolations from traversing down into lambdas and nested functions. This correctly accounts for arbitrarily nested lambdas, inner classes, and blocks that may be inside appropriate @available checks at any ancestor level. It also treats all potential availability violations inside functions consistently, without being overly sensitive to the current DeclContext, which previously caused issues where e.g. nested struct members were warned about twice.

DiagnoseUnguardedAvailabilityViolations now has more work to do in some cases, particularly in functions with many (possibly deeply) nested lambdas and classes, but the big-O is the same, and the simplicity of the approach and the fact that it fixes at least two bugs feels like a strong win IMO.

Diff Detail

Event Timeline

logan-5 requested review of this revision.May 12 2021, 10:52 AM
logan-5 created this revision.

Thanks for the patch! I think the new checking semantics behavior provided by this patch are fine and match what the Swift compiler allows in the if #available blocks in Swift. I will do some additional testing tomorrow but I'm hoping I can approve your patch in the next few days.

Thanks very much; I look forward to any feedback!

arphaman accepted this revision.Mon, May 24, 11:23 AM

I think this is good to go, I haven't observed any issues with this patch so far in my testing. LGTM

This revision is now accepted and ready to land.Mon, May 24, 11:23 AM