Skip to content

Extend transitive availability checking to initial value expressions (better version) #22460

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 11, 2019
Merged

Extend transitive availability checking to initial value expressions (better version) #22460

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ca0aeb2
Verify that a Decl's DeclContext and a DeclContext's parent are in sync
jrose-apple Feb 7, 2019
722cb83
Extend transitive availability checking to initial value expressions
jrose-apple Feb 7, 2019
19c1765
Remove unnecessary work from availability fix-its
jrose-apple Feb 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions lib/AST/ASTVerifier.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -540,11 +540,19 @@ class Verifier : public ASTWalker {
void verifyParsed(Stmt *S) {}
void verifyParsed(Pattern *P) {}
void verifyParsed(Decl *D) {
PrettyStackTraceDecl debugStack("verifying ", D);
if (!D->getDeclContext()) {
Out << "every Decl should have a DeclContext";
PrettyStackTraceDecl debugStack("verifying DeclContext", D);
Out << "every Decl should have a DeclContext\n";
abort();
}
if (auto *DC = dyn_cast<DeclContext>(D)) {
if (D->getDeclContext() != DC->getParent()) {
Out << "Decl's DeclContext not in sync with DeclContext's parent\n";
D->getDeclContext()->dumpContext();
DC->getParent()->dumpContext();
abort();
}
}
}
template<typename T>
void verifyParsedBase(T ASTNode) {
Expand Down
107 changes: 43 additions & 64 deletions lib/Sema/TypeCheckAvailability.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "TypeChecker.h"
#include "MiscDiagnostics.h"
#include "swift/AST/ASTWalker.h"
#include "swift/AST/Initializer.h"
#include "swift/AST/NameLookup.h"
#include "swift/AST/Pattern.h"
#include "swift/AST/TypeRefinementContext.h"
Expand Down Expand Up @@ -856,27 +857,41 @@ static Optional<ASTNode> findInnermostAncestor(
static const Decl *findContainingDeclaration(SourceRange ReferenceRange,
const DeclContext *ReferenceDC,
const SourceManager &SM) {
if (const Decl *D = ReferenceDC->getInnermostDeclarationDeclContext())
auto ContainsReferenceRange = [&](const Decl *D) -> bool {
if (ReferenceRange.isInvalid())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remove this check from the find_if loop by moving the early return later in the function up?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That changes behavior slightly, since in the top-level case it means we return nullptr and in the decl case we return the decl.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you're right. I think you could still test the range's validity once before calling find_if, but that might not be worth the trouble.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I can remove the other check, even though that makes things slightly less efficient. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you should remove the other check. You should either put an if (ReferenceRange.isInvalid()) return D; before the if (auto * IDC = ...) block so it never calls ContainsReferenceRange if the reference range is invalid (and perhaps assert that fact in ContainsReferenceRange), or stick with this implementation.

return false;
return SM.rangeContains(D->getSourceRange(), ReferenceRange);
};

if (const Decl *D = ReferenceDC->getInnermostDeclarationDeclContext()) {
// If we have an inner declaration context, see if we can narrow the search
// down to one of its members. This is important for properties, which don't
// count as DeclContexts of their own but which can still introduce
// availability.
if (auto *IDC = dyn_cast<IterableDeclContext>(D)) {
auto BestMember = llvm::find_if(IDC->getMembers(),
ContainsReferenceRange);
if (BestMember != IDC->getMembers().end())
return *BestMember;
}
return D;
}

// We couldn't find a suitable node by climbing the DeclContext
// hierarchy, so fall back to looking for a top-level declaration
// that contains the reference range. We will hit this case for
// top-level elements that do not themselves introduce DeclContexts,
// such as extensions and global variables. If we don't have a reference
// range, there is nothing we can do, so return null.
// We couldn't find a suitable node by climbing the DeclContext hierarchy, so
// fall back to looking for a top-level declaration that contains the
// reference range. We will hit this case for top-level elements that do not
// themselves introduce DeclContexts, such as global variables. If we don't
// have a reference range, there is nothing we can do, so return null.
if (ReferenceRange.isInvalid())
return nullptr;

SourceFile *SF = ReferenceDC->getParentSourceFile();
if (!SF)
return nullptr;

for (Decl *D : SF->Decls) {
if (SM.rangeContains(D->getSourceRange(), ReferenceRange)) {
return D;
}
}
auto BestTopLevelDecl = llvm::find_if(SF->Decls, ContainsReferenceRange);
if (BestTopLevelDecl != SF->Decls.end())
return *BestTopLevelDecl;

return nullptr;
}
Expand Down Expand Up @@ -929,9 +944,13 @@ abstractSyntaxDeclForAvailableAttribute(const Decl *ConcreteSyntaxDecl) {
// binding.
ArrayRef<PatternBindingEntry> Entries = PBD->getPatternList();
if (!Entries.empty()) {
VarDecl *VD = Entries.front().getPattern()->getSingleVar();
if (VD)
return VD;
const VarDecl *AnyVD = nullptr;
// FIXME: This is wasteful; we only need the first variable.
Entries.front().getPattern()->forEachVariable([&](const VarDecl *VD) {
AnyVD = VD;
});
if (AnyVD)
return AnyVD;
}
} else if (auto *ECD = dyn_cast<EnumCaseDecl>(ConcreteSyntaxDecl)) {
// Similar to the PatternBindingDecl case above, we return the
Expand Down Expand Up @@ -1091,35 +1110,17 @@ static void findAvailabilityFixItNodes(SourceRange ReferenceRange,
FoundVersionCheckNode =
findInnermostAncestor(ReferenceRange, SM, SearchRoot, IsGuardable);

// Find some Decl that contains the reference range. We use this declaration
// as a starting place to climb the DeclContext hierarchy to find
// places to suggest adding @available() annotations.
InnermostAncestorFinder::MatchPredicate IsDeclaration = [](
ASTNode Node, ASTWalker::ParentTy Parent) { return Node.is<Decl *>(); };

Optional<ASTNode> FoundDeclarationNode =
findInnermostAncestor(ReferenceRange, SM, SearchRoot, IsDeclaration);

const Decl *ContainingDecl = nullptr;
if (FoundDeclarationNode.hasValue()) {
ContainingDecl = FoundDeclarationNode.getValue().get<Decl *>();
}

if (!ContainingDecl) {
ContainingDecl = ReferenceDC->getInnermostMethodContext();
}

// Try to find declarations on which @available attributes can be added.
// The heuristics for finding these declarations are biased towards deeper
// nodes in the AST to limit the scope of suggested availability regions
// and provide a better IDE experience (it can get jumpy if Fix-It locations
// are far away from the error needing the Fix-It).
if (ContainingDecl) {
if (DeclarationToSearch) {
FoundMemberLevelDecl =
ancestorMemberLevelDeclForAvailabilityFixit(ContainingDecl);
ancestorMemberLevelDeclForAvailabilityFixit(DeclarationToSearch);

FoundTypeLevelDecl =
ancestorTypeLevelDeclForAvailabilityFixit(ContainingDecl);
ancestorTypeLevelDeclForAvailabilityFixit(DeclarationToSearch);
}
}

Expand Down Expand Up @@ -1408,7 +1409,7 @@ someEnclosingDeclMatches(SourceRange ReferenceRange,
// Climb the DeclContext hierarchy to see if any of the containing
// declarations matches the predicate.
const DeclContext *DC = ReferenceDC;
do {
while (true) {
auto *D = DC->getInnermostDeclarationDeclContext();
if (!D)
break;
Expand All @@ -1424,20 +1425,15 @@ someEnclosingDeclMatches(SourceRange ReferenceRange,
return true;
}

DC = DC->getParent();
} while (DC);
DC = D->getDeclContext();
}

// Search the AST starting from our innermost declaration context to see if
// if the reference is inside a property declaration but not inside an
// accessor (this can happen for the TypeRepr for the declared type of a
// property, for example).
// We can't rely on the DeclContext hierarchy climb above because properties
// do not introduce a new DeclContext. This search is potentially slow, so we
// do it last and only if the reference declaration context is a
// type or global context.

if (!ReferenceDC->isTypeContext() && !ReferenceDC->isModuleScopeContext())
return false;
// do not introduce a new DeclContext.

// Don't search for a containing declaration if we don't have a source range.
if (ReferenceRange.isInvalid())
Expand All @@ -1448,28 +1444,11 @@ someEnclosingDeclMatches(SourceRange ReferenceRange,
findContainingDeclaration(ReferenceRange, ReferenceDC, Ctx.SourceMgr);

// We may not be able to find a declaration to search if the ReferenceRange
// is invalid (i.e., we are in synthesized code).
// isn't useful (i.e., we are in synthesized code).
if (!DeclToSearch)
return false;

InnermostAncestorFinder::MatchPredicate IsDeclaration =
[](ASTNode Node, ASTWalker::ParentTy Parent) {
return Node.is<Decl *>();
};

Optional<ASTNode> FoundDeclarationNode =
findInnermostAncestor(ReferenceRange, Ctx.SourceMgr,
const_cast<Decl *>(DeclToSearch), IsDeclaration);

if (FoundDeclarationNode.hasValue()) {
const Decl *D = FoundDeclarationNode.getValue().get<Decl *>();
D = abstractSyntaxDeclForAvailableAttribute(D);
if (Pred(D)) {
return true;
}
}

return false;
return Pred(abstractSyntaxDeclForAvailableAttribute(DeclToSearch));
}

/// Returns true if the reference or any of its parents is an
Expand Down
89 changes: 87 additions & 2 deletions test/attr/attr_availability_transitive_osx.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,18 @@
// RUN: %target-typecheck-verify-swift
// RUN: %target-typecheck-verify-swift -parse-as-library
// REQUIRES: OS=macosx

// Allow referencing unavailable API in situations where the caller is marked unavailable in the same circumstances.

@available(OSX, unavailable)
func osx() {} // expected-note 2{{'osx()' has been explicitly marked unavailable here}}
@discardableResult
func osx() -> Int { return 0 } // expected-note * {{'osx()' has been explicitly marked unavailable here}}

@available(OSXApplicationExtension, unavailable)
func osx_extension() {}

@available(OSX, unavailable)
func osx_pair() -> (Int, Int) { return (0, 0) } // expected-note * {{'osx_pair()' has been explicitly marked unavailable here}}

func call_osx_extension() {
osx_extension() // OK; osx_extension is only unavailable if -application-extension is passed.
}
Expand All @@ -35,3 +39,84 @@ func osx_extension_call_osx_extension() {
func osx_extension_call_osx() {
osx() // expected-error {{'osx()' is unavailable}}
}

@available(OSX, unavailable)
var osx_init_osx = osx() // OK

@available(OSXApplicationExtension, unavailable)
var osx_extension_init_osx = osx() // expected-error {{'osx()' is unavailable}}

@available(OSX, unavailable)
var osx_inner_init_osx = { let inner_var = osx() } // OK

// FIXME: I'm not sure why this produces two errors instead of just one.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's because someone is not skipping multi-statement closures somewhere, so the body of the closure is visited again. Just a guess though.

@available(OSXApplicationExtension, unavailable)
var osx_extension_inner_init_osx = { let inner_var = osx() } // expected-error 2 {{'osx()' is unavailable}}

struct Outer {
@available(OSX, unavailable)
var osx_init_osx = osx() // OK

@available(OSX, unavailable)
lazy var osx_lazy_osx = osx() // OK

@available(OSXApplicationExtension, unavailable)
var osx_extension_init_osx = osx() // expected-error {{'osx()' is unavailable}}

@available(OSXApplicationExtension, unavailable)
var osx_extension_lazy_osx = osx() // expected-error {{'osx()' is unavailable}}

@available(OSX, unavailable)
var osx_init_multi1_osx = osx(), osx_init_multi2_osx = osx() // OK

@available(OSXApplicationExtension, unavailable)
var osx_extension_init_multi1_osx = osx(), osx_extension_init_multi2_osx = osx() // expected-error 2 {{'osx()' is unavailable}}

@available(OSX, unavailable)
var (osx_init_deconstruct1_osx, osx_init_deconstruct2_osx) = osx_pair() // OK

@available(OSXApplicationExtension, unavailable)
var (osx_extension_init_deconstruct1_osx, osx_extension_init_deconstruct2_osx) = osx_pair() // expected-error {{'osx_pair()' is unavailable}}

@available(OSX, unavailable)
var (_, osx_init_deconstruct2_only_osx) = osx_pair() // OK

@available(OSXApplicationExtension, unavailable)
var (_, osx_extension_init_deconstruct2_only_osx) = osx_pair() // expected-error {{'osx_pair()' is unavailable}}

@available(OSX, unavailable)
var (osx_init_deconstruct1_only_osx, _) = osx_pair() // OK

@available(OSXApplicationExtension, unavailable)
var (osx_extension_init_deconstruct1_only_osx, _) = osx_pair() // expected-error {{'osx_pair()' is unavailable}}

@available(OSX, unavailable)
var osx_inner_init_osx = { let inner_var = osx() } // OK

// FIXME: I'm not sure why this produces two errors instead of just one.
@available(OSXApplicationExtension, unavailable)
var osx_extension_inner_init_osx = { let inner_var = osx() } // expected-error 2 {{'osx()' is unavailable}}
}

extension Outer {
@available(OSX, unavailable)
static var also_osx_init_osx = osx() // OK

@available(OSXApplicationExtension, unavailable)
static var also_osx_extension_init_osx = osx() // expected-error {{'osx()' is unavailable}}
}

@available(OSX, unavailable)
extension Outer {
static var outer_osx_init_osx = osx() // OK
}

@available(OSX, unavailable)
struct NotOnOSX {
var osx_init_osx = osx() // OK
lazy var osx_lazy_osx = osx() // OK
var osx_init_multi1_osx = osx(), osx_init_multi2_osx = osx() // OK
var (osx_init_deconstruct1_osx, osx_init_deconstruct2_osx) = osx_pair() // OK
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be sure can you also test a few cases involving patterns like var (foo, _) = ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, good call!

var (_, osx_init_deconstruct2_only_osx) = osx_pair() // OK
var (osx_init_deconstruct1_only_osx, _) = osx_pair() // OK
}