Skip to content

Kill validateDeclForNameLookup Harder #27172

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 8 commits into from
Sep 18, 2019
Merged

Kill validateDeclForNameLookup Harder #27172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2ab5ea8
Define UnderlyingTypeRequest
CodaFi Sep 13, 2019
278ead3
Resugar the structural type
CodaFi Sep 13, 2019
dceb357
Improve the TypeAlias{Type|Decl} dump
CodaFi Sep 13, 2019
5e34169
Separate computing interface types and underlying types
CodaFi Sep 13, 2019
d5c014b
Remove validateDeclForNameLookup
CodaFi Sep 13, 2019
c254f4d
Setting the underlying type should set the structural type too
CodaFi Sep 13, 2019
6b7fbc9
Break some cycles
CodaFi Sep 18, 2019
4f6951c
Change a cycle condition
CodaFi Sep 18, 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
20 changes: 15 additions & 5 deletions include/swift/AST/Decl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2947,6 +2947,8 @@ class OpaqueTypeDecl : public GenericTypeDecl {
/// TypeAliasDecl's always have 'MetatypeType' type.
///
class TypeAliasDecl : public GenericTypeDecl {
friend class UnderlyingTypeRequest;

/// The location of the 'typealias' keyword
SourceLoc TypeAliasLoc;

Expand Down Expand Up @@ -2974,20 +2976,28 @@ class TypeAliasDecl : public GenericTypeDecl {

void setTypeEndLoc(SourceLoc e) { TypeEndLoc = e; }

TypeLoc &getUnderlyingTypeLoc() {
return UnderlyingTy;
/// Retrieve the TypeRepr corresponding to the parsed underlying type.
TypeRepr *getUnderlyingTypeRepr() const {
return UnderlyingTy.getTypeRepr();
}
const TypeLoc &getUnderlyingTypeLoc() const {
return UnderlyingTy;
void setUnderlyingTypeRepr(TypeRepr *repr) {
UnderlyingTy = repr;
}


/// Retrieve the interface type of the underlying type.
Type getUnderlyingType() const;
void setUnderlyingType(Type type);

/// For generic typealiases, return the unbound generic type.
UnboundGenericType *getUnboundGenericType() const;

/// Retrieve a sugared interface type containing the structure of the interface
/// type before any semantic validation has occured.
Type getStructuralType() const;

/// Set the interface type of this typealias declaration from the underlying type.
void computeType();

bool isCompatibilityAlias() const {
return Bits.TypeAliasDecl.IsCompatibilityAlias;
}
Expand Down
30 changes: 29 additions & 1 deletion include/swift/AST/TypeCheckRequests.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,6 +434,9 @@ class RequirementRequest :
// Source location
SourceLoc getNearestLoc() const;

// Cycle handling.
void noteCycleStep(DiagnosticEngine &diags) const;

// Separate caching.
bool isCached() const;
Optional<Requirement> getCachedResult() const;
Expand Down Expand Up @@ -1120,6 +1123,9 @@ class InferredGenericSignatureRequest :
SourceLoc getNearestLoc() const {
return SourceLoc();
}

// Cycle handling.
void noteCycleStep(DiagnosticEngine &diags) const;
};

void simple_display(llvm::raw_ostream &out, const TypeLoc source);
Expand Down Expand Up @@ -1174,13 +1180,35 @@ class GenericSignatureRequest :
llvm::Expected<GenericSignature *>
evaluate(Evaluator &evaluator, GenericContext *value) const;

public:
public:
// Separate caching.
bool isCached() const { return true; }
Optional<GenericSignature *> getCachedResult() const;
void cacheResult(GenericSignature *value) const;
};

/// Compute the interface type of the underlying definition type of a typealias declaration.
class UnderlyingTypeRequest :
public SimpleRequest<UnderlyingTypeRequest,
Type(TypeAliasDecl *),
CacheKind::SeparatelyCached> {
public:
using SimpleRequest::SimpleRequest;

private:
friend SimpleRequest;

// Evaluation.
llvm::Expected<Type> evaluate(Evaluator &evaluator,
TypeAliasDecl *decl) const;

public:
// Caching.
bool isCached() const { return true; }
Optional<Type> getCachedResult() const;
void cacheResult(Type value) const;
};

// Allow AnyValue to compare two Type values, even though Type doesn't
// support ==.
template<>
Expand Down
2 changes: 2 additions & 0 deletions include/swift/AST/TypeCheckerTypeIDZone.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,5 +132,7 @@ SWIFT_REQUEST(TypeChecker, SynthesizeAccessorRequest,
SeparatelyCached, NoLocationInfo)
SWIFT_REQUEST(TypeChecker, TypeCheckFunctionBodyUntilRequest,
bool(AbstractFunctionDecl *, SourceLoc), Cached, NoLocationInfo)
SWIFT_REQUEST(TypeChecker, UnderlyingTypeRequest, Type(TypeAliasDecl *),
SeparatelyCached, NoLocationInfo)
SWIFT_REQUEST(TypeChecker, USRGenerationRequest, std::string(const ValueDecl *),
Cached, NoLocationInfo)
10 changes: 8 additions & 2 deletions lib/AST/ASTDumper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -616,9 +616,9 @@ namespace {
void visitTypeAliasDecl(TypeAliasDecl *TAD) {
printCommon(TAD, "typealias");
PrintWithColorRAII(OS, TypeColor) << " type='";
if (TAD->getUnderlyingTypeLoc().getType()) {
if (auto underlying = TAD->getUnderlyingType()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This kicks off a request :(

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'm going to add an RAII type to catch all of these.

PrintWithColorRAII(OS, TypeColor)
<< TAD->getUnderlyingTypeLoc().getType().getString();
<< underlying.getString();
} else {
PrintWithColorRAII(OS, TypeColor) << "<<<unresolved>>>";
}
Expand Down Expand Up @@ -3378,6 +3378,12 @@ namespace {
void visitTypeAliasType(TypeAliasType *T, StringRef label) {
printCommon(label, "type_alias_type");
printField("decl", T->getDecl()->printRef());
PrintWithColorRAII(OS, TypeColor) << " underlying='";
if (auto underlying = T->getSinglyDesugaredType()) {
PrintWithColorRAII(OS, TypeColor) << underlying->getString();
} else {
PrintWithColorRAII(OS, TypeColor) << "<<<unresolved>>>";
}
if (T->getParent())
printRec("parent", T->getParent());

Expand Down
4 changes: 2 additions & 2 deletions lib/AST/ASTPrinter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2276,7 +2276,7 @@ void PrintAST::visitTypeAliasDecl(TypeAliasDecl *decl) {
printGenericDeclGenericParams(decl);
});
bool ShouldPrint = true;
Type Ty = decl->getUnderlyingTypeLoc().getType();
Type Ty = decl->getUnderlyingType();

// If the underlying type is private, don't print it.
if (Options.SkipPrivateStdlibDecls && Ty && Ty.isPrivateStdlibType())
Expand All @@ -2294,7 +2294,7 @@ void PrintAST::visitTypeAliasDecl(TypeAliasDecl *decl) {
// preserving sugar.
llvm::SaveAndRestore<GenericEnvironment*> setGenericEnv(Options.GenericEnv,
decl->getGenericEnvironment());
printTypeLoc(decl->getUnderlyingTypeLoc());
printTypeLoc(TypeLoc(decl->getUnderlyingTypeRepr(), Ty));
printGenericDeclGenericRequirements(decl);
}
}
Expand Down
5 changes: 4 additions & 1 deletion lib/AST/ASTWalker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,10 @@ class Traversal : public ASTVisitor<Traversal, Expr*, Stmt*,
return true;
}

return doIt(TAD->getUnderlyingTypeLoc());
if (auto typerepr = TAD->getUnderlyingTypeRepr())
if (doIt(typerepr))
return true;
return false;
}

bool visitOpaqueTypeDecl(OpaqueTypeDecl *OTD) {
Expand Down
62 changes: 34 additions & 28 deletions lib/AST/Decl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3527,34 +3527,42 @@ SourceRange TypeAliasDecl::getSourceRange() const {
return { TypeAliasLoc, getNameLoc() };
}

void TypeAliasDecl::setUnderlyingType(Type underlying) {
setValidationToChecked();
void TypeAliasDecl::computeType() {
assert(!hasInterfaceType());

// Set the interface type of this declaration.
ASTContext &ctx = getASTContext();

auto *genericSig = getGenericSignature();
SubstitutionMap subs;
if (genericSig)
subs = genericSig->getIdentitySubstitutionMap();

Type parent;
auto parentDC = getDeclContext();
if (parentDC->isTypeContext())
parent = parentDC->getDeclaredInterfaceType();
auto sugaredType = TypeAliasType::get(this, parent, subs, getUnderlyingType());
setInterfaceType(MetatypeType::get(sugaredType, ctx));
}

Type TypeAliasDecl::getUnderlyingType() const {
return evaluateOrDefault(getASTContext().evaluator,
UnderlyingTypeRequest{const_cast<TypeAliasDecl *>(this)},
Type());
}

void TypeAliasDecl::setUnderlyingType(Type underlying) {
// lldb creates global typealiases containing archetypes
// sometimes...
if (underlying->hasArchetype() && isGenericContext())
underlying = underlying->mapTypeOutOfContext();
UnderlyingTy.setType(underlying);

// FIXME -- if we already have an interface type, we're changing the
// underlying type. See the comment in the ProtocolDecl case of
// validateDecl().
if (!hasInterfaceType()) {
// Set the interface type of this declaration.
ASTContext &ctx = getASTContext();

auto *genericSig = getGenericSignature();
SubstitutionMap subs;
if (genericSig)
subs = genericSig->getIdentitySubstitutionMap();

Type parent;
auto parentDC = getDeclContext();
if (parentDC->isTypeContext())
parent = parentDC->getDeclaredInterfaceType();
auto sugaredType = TypeAliasType::get(this, parent, subs, underlying);
setInterfaceType(MetatypeType::get(sugaredType, ctx));
}
getASTContext().evaluator.cacheOutput(
StructuralTypeRequest{const_cast<TypeAliasDecl *>(this)},
std::move(underlying));
getASTContext().evaluator.cacheOutput(
UnderlyingTypeRequest{const_cast<TypeAliasDecl *>(this)},
std::move(underlying));
}

UnboundGenericType *TypeAliasDecl::getUnboundGenericType() const {
Expand All @@ -3571,12 +3579,10 @@ UnboundGenericType *TypeAliasDecl::getUnboundGenericType() const {
}

Type TypeAliasDecl::getStructuralType() const {
assert(!getGenericParams());

auto &context = getASTContext();
return evaluateOrDefault(context.evaluator,
StructuralTypeRequest { const_cast<TypeAliasDecl *>(this) },
Type());
return evaluateOrDefault(
context.evaluator,
StructuralTypeRequest{const_cast<TypeAliasDecl *>(this)}, Type());
}

Type AbstractTypeParamDecl::getSuperclass() const {
Expand Down
2 changes: 1 addition & 1 deletion lib/AST/DiagnosticEngine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,7 @@ static bool isInterestingTypealias(Type type) {
// Compatibility aliases are only interesting insofar as their underlying
// types are interesting.
if (aliasDecl->isCompatibilityAlias()) {
auto underlyingTy = aliasDecl->getUnderlyingTypeLoc().getType();
auto underlyingTy = aliasDecl->getUnderlyingType();
return isInterestingTypealias(underlyingTy);
}

Expand Down
16 changes: 9 additions & 7 deletions lib/AST/GenericSignatureBuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3785,10 +3785,13 @@ PotentialArchetype *GenericSignatureBuilder::realizePotentialArchetype(

static Type getStructuralType(TypeDecl *typeDecl) {
if (auto typealias = dyn_cast<TypeAliasDecl>(typeDecl)) {
if (auto resolved = typealias->getUnderlyingTypeLoc().getType())
return resolved;

return typealias->getStructuralType();
// When we're computing requirement signatures, the structural type
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the indentation here might be a bit funky

// suffices. Otherwise we'll potentially try to validate incomplete
// requirements.
auto *proto = dyn_cast_or_null<ProtocolDecl>(typealias->getDeclContext()->getAsDecl());
if (proto && proto->isComputingRequirementSignature())
return typealias->getStructuralType();
Copy link
Contributor

Choose a reason for hiding this comment

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

Here too

return typealias->getUnderlyingType();
}

return typeDecl->getDeclaredInterfaceType();
Expand Down Expand Up @@ -4196,11 +4199,10 @@ ConstraintResult GenericSignatureBuilder::expandConformanceRequirement(
out << start;
out << type->getFullName() << " == ";
if (auto typealias = dyn_cast<TypeAliasDecl>(type)) {
if (auto underlyingTypeRepr =
typealias->getUnderlyingTypeLoc().getTypeRepr())
if (auto underlyingTypeRepr = typealias->getUnderlyingTypeRepr())
underlyingTypeRepr->print(out);
else
typealias->getUnderlyingTypeLoc().getType().print(out);
typealias->getUnderlyingType().print(out);
} else {
type->print(out);
}
Expand Down
2 changes: 2 additions & 0 deletions lib/AST/Module.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,8 @@ void BuiltinUnit::LookupCache::lookupValue(
const_cast<BuiltinUnit*>(&M));
TAD->setUnderlyingType(Ty);
TAD->setAccess(AccessLevel::Public);
TAD->setValidationToChecked();
TAD->computeType();
Entry = TAD;
}
}
Expand Down
13 changes: 7 additions & 6 deletions lib/AST/NameLookup.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1813,7 +1813,7 @@ resolveTypeDeclsToNominal(Evaluator &evaluator,
// Recognize Swift.AnyObject directly.
if (typealias->getName().is("AnyObject")) {
// TypeRepr version: Builtin.AnyObject
if (auto typeRepr = typealias->getUnderlyingTypeLoc().getTypeRepr()) {
if (auto typeRepr = typealias->getUnderlyingTypeRepr()) {
if (auto compound = dyn_cast<CompoundIdentTypeRepr>(typeRepr)) {
auto components = compound->getComponents();
if (components.size() == 2 &&
Expand All @@ -1825,9 +1825,10 @@ resolveTypeDeclsToNominal(Evaluator &evaluator,
}

// Type version: an empty class-bound existential.
if (auto type = typealias->getUnderlyingTypeLoc().getType()) {
if (type->isAnyObject())
anyObject = true;
if (typealias->hasInterfaceType()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The hasInterfaceType() check should not be necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Requesting the underlying type potentially calls back into validation. This is also justified because the interface type separation changes mean that AnyObject declarations will always have their interface type set by the time we see them.

if (auto type = typealias->getUnderlyingType())
if (type->isAnyObject())
anyObject = true;
}
}

Expand Down Expand Up @@ -2091,15 +2092,15 @@ DirectlyReferencedTypeDecls UnderlyingTypeDeclsReferencedRequest::evaluate(
Evaluator &evaluator,
TypeAliasDecl *typealias) const {
// Prefer syntactic information when we have it.
if (auto typeRepr = typealias->getUnderlyingTypeLoc().getTypeRepr()) {
if (auto typeRepr = typealias->getUnderlyingTypeRepr()) {
return directReferencesForTypeRepr(evaluator, typealias->getASTContext(),
typeRepr, typealias);
}

// Fall back to semantic types.
// FIXME: In the long run, we shouldn't need this. Non-syntactic results
// should be cached.
if (auto type = typealias->getUnderlyingTypeLoc().getType()) {
if (auto type = typealias->getUnderlyingType()) {
return directReferencesForType(type);
}

Expand Down
37 changes: 37 additions & 0 deletions lib/AST/TypeCheckRequests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,14 @@ SourceLoc RequirementRequest::getNearestLoc() const {
return owner.getLoc();
}

void RequirementRequest::noteCycleStep(DiagnosticEngine &diags) const {
// For now, the GSB does a better job of describing the exact structure of
// the cycle.
//
// FIXME: We should consider merging the circularity handling the GSB does
// into this request. See rdar://55263708
}

MutableArrayRef<RequirementRepr> WhereClauseOwner::getRequirements() const {
if (auto genericParams = source.dyn_cast<GenericParamList *>()) {
return genericParams->getRequirements();
Expand Down Expand Up @@ -822,3 +830,32 @@ void GenericSignatureRequest::cacheResult(GenericSignature *value) const {
auto *GC = std::get<0>(getStorage());
GC->GenericSigAndBit.setPointerAndInt(value, true);
}

//----------------------------------------------------------------------------//
// GenericSignatureRequest computation.
//----------------------------------------------------------------------------//

void InferredGenericSignatureRequest::noteCycleStep(DiagnosticEngine &d) const {
// For now, the GSB does a better job of describing the exact structure of
// the cycle.
//
// FIXME: We should consider merging the circularity handling the GSB does
// into this request. See rdar://55263708
}

//----------------------------------------------------------------------------//
// IsImplicitlyUnwrappedOptionalRequest computation.
//----------------------------------------------------------------------------//

Optional<Type>
UnderlyingTypeRequest::getCachedResult() const {
auto *typeAlias = std::get<0>(getStorage());
if (auto type = typeAlias->UnderlyingTy.getType())
return type;
return None;
}

void UnderlyingTypeRequest::cacheResult(Type value) const {
auto *typeAlias = std::get<0>(getStorage());
typeAlias->UnderlyingTy.setType(value);
}
2 changes: 1 addition & 1 deletion lib/ClangImporter/ClangImporter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2659,7 +2659,7 @@ void ClangModuleUnit::getTopLevelDecls(SmallVectorImpl<Decl*> &results) const {
auto alias = dyn_cast<TypeAliasDecl>(importedDecl);
if (!alias || !alias->isCompatibilityAlias()) continue;

auto aliasedTy = alias->getUnderlyingTypeLoc().getType();
auto aliasedTy = alias->getUnderlyingType();
ext = nullptr;
importedDecl = nullptr;

Expand Down
Loading