Skip to content

ASTScope: Take start location into account when modeling local pattern bindings #34038

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.

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 4 commits into from
Sep 25, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 3 additions & 3 deletions include/swift/AST/ASTScope.h
Original file line number Diff line number Diff line change
Expand Up @@ -344,9 +344,6 @@ class ASTScopeImpl {
virtual NullablePtr<ASTScopeImpl> insertionPointForDeferredExpansion();
virtual SourceRange sourceRangeForDeferredExpansion() const;

public:
bool isATypeDeclScope() const;

private:
virtual ScopeCreator &getScopeCreator();

Expand Down Expand Up @@ -435,6 +432,7 @@ class ASTScopeImpl {
return p->getParent().isNonNull() ? p : nullptr;
}

public:
/// The tree is organized by source location and for most nodes this is also
/// what obtaines for scoping. However, guards are different. The scope after
/// the guard else must hop into the innermoset scope of the guard condition.
Expand Down Expand Up @@ -1079,6 +1077,7 @@ class PatternEntryDeclScope final : public AbstractPatternEntryScope {

protected:
bool lookupLocalsOrMembers(DeclConsumer) const override;
bool isLabeledStmtLookupTerminator() const override;
};

class PatternEntryInitializerScope final : public AbstractPatternEntryScope {
Expand All @@ -1093,6 +1092,7 @@ class PatternEntryInitializerScope final : public AbstractPatternEntryScope {

protected:
ASTScopeImpl *expandSpecifically(ScopeCreator &scopeCreator) override;
NullablePtr<const ASTScopeImpl> getLookupParent() const override;

private:
void expandAScopeThatDoesNotCreateANewInsertionPoint(ScopeCreator &);
Expand Down
35 changes: 15 additions & 20 deletions lib/AST/ASTScopeCreation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,6 @@
using namespace swift;
using namespace ast_scope;

/// If true, nest scopes so a variable is out of scope before its declaration
/// Does not handle capture rules for local functions properly.
/// If false don't push uses down into subscopes after decls.
static const bool handleUseBeforeDef = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the flag I've been talking about!


#pragma mark source range utilities
static bool rangeableIsIgnored(const Decl *d) { return d->isImplicit(); }
static bool rangeableIsIgnored(const Expr *d) {
Expand Down Expand Up @@ -746,11 +741,11 @@ class NodeAdder
if (auto *var = patternBinding->getSingleVar())
scopeCreator.addChildrenForKnownAttributes(var, parentScope);

const bool isInTypeDecl = parentScope->isATypeDeclScope();
const bool isLocalBinding = patternBinding->getDeclContext()->isLocalContext();

const DeclVisibilityKind vis =
isInTypeDecl ? DeclVisibilityKind::MemberOfCurrentNominal
: DeclVisibilityKind::LocalVariable;
isLocalBinding ? DeclVisibilityKind::LocalVariable
: DeclVisibilityKind::MemberOfCurrentNominal;
auto *insertionPoint = parentScope;
for (auto i : range(patternBinding->getNumPatternEntries())) {
insertionPoint =
Expand All @@ -759,9 +754,12 @@ class NodeAdder
insertionPoint, patternBinding, i, vis)
.getPtrOr(insertionPoint);
}
// If in a type decl, the type search will find these,
// but if in a brace stmt, must continue under the last binding.
return isInTypeDecl ? parentScope : insertionPoint;

ASTScopeAssert(isLocalBinding || insertionPoint == parentScope,
"Bindings at the top-level or members of types should "
"not change the insertion point");
Comment on lines +758 to +760
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!


return insertionPoint;
}

NullablePtr<ASTScopeImpl> visitEnumElementDecl(EnumElementDecl *eed,
Expand Down Expand Up @@ -1041,11 +1039,13 @@ PatternEntryDeclScope::expandAScopeThatCreatesANewInsertionPoint(
scopeCreator.addChildrenForAllLocalizableAccessorsInSourceOrder(var, this);
});

ASTScopeAssert(!handleUseBeforeDef,
"next line is wrong otherwise; would need a use scope");
// In local context, the PatternEntryDeclScope becomes the insertion point, so
// that all any bindings introduecd by the pattern are in scope for subsequent
// lookups.
Comment on lines +1042 to +1044
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice and clear!

if (vis == DeclVisibilityKind::LocalVariable)
return {this, "All code that follows is inside this scope"};

return {getParent().get(), "When not handling use-before-def, succeeding "
"code just goes in the same scope as this one"};
return {getParent().get(), "Global and type members do not introduce scopes"};
}

void
Expand Down Expand Up @@ -1424,11 +1424,6 @@ AbstractPatternEntryScope::AbstractPatternEntryScope(
"out of bounds");
}

bool ASTScopeImpl::isATypeDeclScope() const {
Decl *const pd = getDeclIfAny().getPtrOrNull();
return pd && (isa<NominalTypeDecl>(pd) || isa<ExtensionDecl>(pd));
}

#pragma mark new operators
void *ASTScopeImpl::operator new(size_t bytes, const ASTContext &ctx,
unsigned alignment) {
Expand Down
22 changes: 22 additions & 0 deletions lib/AST/ASTScopeLookup.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,24 @@ bool GenericTypeOrExtensionScope::areMembersVisibleFromWhereClause() const {
return isa<ProtocolDecl>(decl) || isa<ExtensionDecl>(decl);
}

#pragma mark custom lookup parent behavior

NullablePtr<const ASTScopeImpl>
PatternEntryInitializerScope::getLookupParent() const {
auto parent = getParent().get();
assert(parent->getClassName() == "PatternEntryDeclScope");

// Lookups from inside a pattern binding initializer skip the parent
// scope that introduces bindings bound by the pattern, since we
// want this to work:
//
// func f(x: Int) {
// let x = x
// print(x)
// }
return parent->getLookupParent();
}

#pragma mark looking in locals or members - locals

bool GenericParamScope::lookupLocalsOrMembers(DeclConsumer consumer) const {
Expand Down Expand Up @@ -510,6 +528,10 @@ bool CaseStmtBodyScope::isLabeledStmtLookupTerminator() const {
return false;
}

bool PatternEntryDeclScope::isLabeledStmtLookupTerminator() const {
return false;
}

llvm::SmallVector<LabeledStmt *, 4>
ASTScopeImpl::lookupLabeledStmts(SourceFile *sourceFile, SourceLoc loc) {
// Find the innermost scope from which to start our search.
Expand Down
10 changes: 5 additions & 5 deletions test/NameLookup/scope_map_top_level.swift
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ var i: Int = b.my_identity()
// CHECK-EXPANDED-NEXT: `-NominalTypeBodyScope {{.*}}, [4:11 - 4:13]
// CHECK-EXPANDED-NEXT: `-TopLevelCodeScope {{.*}}, [6:1 - 21:28]
// CHECK-EXPANDED-NEXT: `-BraceStmtScope {{.*}}, [6:1 - 21:28]
// CHECK-EXPANDED-NEXT: |-PatternEntryDeclScope {{.*}}, [6:5 - 6:15] entry 0 'a'
// CHECK-EXPANDED-NEXT: `-PatternEntryInitializerScope {{.*}}, [6:15 - 6:15] entry 0 'a'
// CHECK-EXPANDED-NEXT: `-PatternEntryDeclScope {{.*}}, [6:5 - 21:28] entry 0 'a'
// CHECK-EXPANDED-NEXT: |-PatternEntryInitializerScope {{.*}}, [6:15 - 6:15] entry 0 'a'
// CHECK-EXPANDED-NEXT: `-TopLevelCodeScope {{.*}}, [8:1 - 21:28]
// CHECK-EXPANDED-NEXT: `-BraceStmtScope {{.*}}, [8:1 - 21:28]
// CHECK-EXPANDED-NEXT: `-GuardStmtScope {{.*}}, [8:1 - 21:28]
Expand All @@ -46,9 +46,9 @@ var i: Int = b.my_identity()
// CHECK-EXPANDED-NEXT: `-BraceStmtScope {{.*}}, [11:18 - 11:19]
// CHECK-EXPANDED-NEXT: `-TopLevelCodeScope {{.*}}, [13:1 - 21:28]
// CHECK-EXPANDED-NEXT: `-BraceStmtScope {{.*}}, [13:1 - 21:28]
// CHECK-EXPANDED-NEXT: |-PatternEntryDeclScope {{.*}}, [13:5 - 13:9] entry 0 'c'
// CHECK-EXPANDED-NEXT: `-PatternEntryInitializerScope {{.*}}, [13:9 - 13:9] entry 0 'c'
// CHECK-EXPANDED-NEXT: |-TypeAliasDeclScope {{.*}}, [15:1 - 15:15]
// CHECK-EXPANDED-NEXT: `-PatternEntryDeclScope {{.*}}, [13:5 - 21:28] entry 0 'c'
// CHECK-EXPANDED-NEXT: |-PatternEntryInitializerScope {{.*}}, [13:9 - 13:9] entry 0 'c'
// CHECK-EXPANDED-NEXT: |-TypeAliasDeclScope {{.*}}, [15:1 - 15:15]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: It seems like the TypeAliasDeclScope got overindented along the way.

// CHECK-EXPANDED-NEXT: |-ExtensionDeclScope {{.*}}, [17:14 - 19:1]
// CHECK-EXPANDED-NEXT: `-ExtensionBodyScope {{.*}}, [17:15 - 19:1]
// CHECK-EXPANDED-NEXT: `-AbstractFunctionDeclScope {{.*}}, [18:3 - 18:43] 'my_identity()'
Expand Down