Skip to content

Introduce a special DeclBaseName for "deinit" #10965

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 2 commits into from
Aug 1, 2017
Merged

Introduce a special DeclBaseName for "deinit" #10965

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2eb36e4
Introduce special name for destructors
ahoppen Jul 12, 2017
ebd701c
Represent the name of destructors by a special DeclBaseName
ahoppen Jul 14, 2017
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
10 changes: 6 additions & 4 deletions include/swift/AST/Decl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4866,8 +4866,6 @@ class AbstractFunctionDecl : public ValueDecl, public GenericContext {
}

public:
Identifier getName() const { return getFullName().getBaseIdentifier(); }

/// Returns the string for the base name, or "_" if this is unnamed.
StringRef getNameStr() const {
assert(!getFullName().isSpecial() && "Cannot get string for special names");
Expand Down Expand Up @@ -5200,6 +5198,8 @@ class FuncDecl final : public AbstractFunctionDecl,
TypeLoc FnRetType, DeclContext *Parent,
ClangNode ClangN = ClangNode());

Identifier getName() const { return getFullName().getBaseIdentifier(); }

bool isStatic() const {
return FuncDeclBits.IsStatic;
}
Expand Down Expand Up @@ -5648,6 +5648,8 @@ class ConstructorDecl : public AbstractFunctionDecl {
GenericParamList *GenericParams,
DeclContext *Parent);

Identifier getName() const { return getFullName().getBaseIdentifier(); }
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we actually use this one? It's always going to be Id_init.

Copy link
Member Author

Choose a reason for hiding this comment

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

I left it in for consistency. You can call getName() on all Decls that are guaranteed to be backed by Identifiers. But I don't think it's actually used. Long term goal would be to remove it once/if Id_init becomes a special name as well.


void setParameterLists(ParamDecl *selfParam, ParameterList *bodyParams);

SourceLoc getConstructorLoc() const { return getNameLoc(); }
Expand Down Expand Up @@ -5839,8 +5841,8 @@ class ConstructorDecl : public AbstractFunctionDecl {
class DestructorDecl : public AbstractFunctionDecl {
ParameterList *SelfParameter;
public:
DestructorDecl(Identifier NameHack, SourceLoc DestructorLoc,
ParamDecl *selfDecl, DeclContext *Parent);
DestructorDecl(SourceLoc DestructorLoc, ParamDecl *selfDecl,
DeclContext *Parent);

void setSelfDecl(ParamDecl *selfDecl);

Expand Down
13 changes: 12 additions & 1 deletion include/swift/AST/Identifier.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,8 @@ class DeclBaseName {
public:
enum class Kind: uint8_t {
Normal,
Subscript
Subscript,
Destructor
};

private:
Expand All @@ -223,6 +224,8 @@ class DeclBaseName {
/// This is an implementation detail that should never leak outside of
/// DeclName.
static void *SubscriptIdentifierData;
/// As above, for special destructor DeclNames.
static void *DestructorIdentifierData;

Identifier Ident;

Expand All @@ -235,9 +238,15 @@ class DeclBaseName {
return DeclBaseName(Identifier((const char *)SubscriptIdentifierData));
}

static DeclBaseName createDestructor() {
return DeclBaseName(Identifier((const char *)DestructorIdentifierData));
}

Kind getKind() const {
if (Ident.get() == SubscriptIdentifierData) {
return Kind::Subscript;
} else if (Ident.get() == DestructorIdentifierData) {
return Kind::Destructor;
} else {
return Kind::Normal;
}
Expand Down Expand Up @@ -273,6 +282,8 @@ class DeclBaseName {
return getIdentifier().str();
case Kind::Subscript:
return "subscript";
case Kind::Destructor:
return "deinit";
}
}

Expand Down
1 change: 0 additions & 1 deletion include/swift/AST/KnownIdentifiers.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ IDENTIFIER(decode)
IDENTIFIER(decodeIfPresent)
IDENTIFIER(Decoder)
IDENTIFIER(decoder)
IDENTIFIER(deinit)
IDENTIFIER(Element)
IDENTIFIER(Encodable)
IDENTIFIER(encode)
Expand Down
7 changes: 5 additions & 2 deletions include/swift/Serialization/ModuleFormat.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ const uint16_t VERSION_MAJOR = 0;
/// in source control, you should also update the comment to briefly
/// describe what change you made. The content of this comment isn't important;
/// it just ensures a conflict if two people change the module format.
const uint16_t VERSION_MINOR = 353; // Last change: count inherited conformances
const uint16_t VERSION_MINOR = 354; // Last change: special destructor names

using DeclID = PointerEmbeddedInt<unsigned, 31>;
using DeclIDField = BCFixed<31>;
Expand Down Expand Up @@ -345,7 +345,8 @@ using OptionalTypeKindField = BCFixed<2>;
// VERSION_MAJOR.
enum class DeclNameKind: uint8_t {
Normal,
Subscript
Subscript,
Destructor
};

// These IDs must \em not be renumbered or reordered without incrementing
Expand All @@ -359,6 +360,8 @@ enum SpecialIdentifierID : uint8_t {
OBJC_HEADER_MODULE_ID,
/// Special value for the special subscript name
SUBSCRIPT_ID,
/// Special value for the special destructor name
DESTRUCTOR_ID,

/// The number of special Identifier IDs. This value should never be encoded;
/// it should only be used to count the number of names above. As such, it
Expand Down
2 changes: 2 additions & 0 deletions lib/AST/ASTMangler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -548,6 +548,8 @@ void ASTMangler::appendDeclName(const ValueDecl *decl) {
case DeclBaseName::Kind::Subscript:
appendIdentifier("subscript");
break;
case DeclBaseName::Kind::Destructor:
llvm_unreachable("Destructors are not mangled using appendDeclName");
}
} else {
assert(AllowNamelessEntities && "attempt to mangle unnamed decl");
Expand Down
32 changes: 20 additions & 12 deletions lib/AST/Decl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,7 +420,7 @@ bool Decl::isPrivateStdlibDecl(bool whitelistProtocols) const {
if (auto AFD = dyn_cast<AbstractFunctionDecl>(D)) {
// Hide '~>' functions (but show the operator, because it defines
// precedence).
if (AFD->getNameStr() == "~>")
Copy link
Member Author

Choose a reason for hiding this comment

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

The only reference to the ~> operator I could find was in Policy.swift which stated that it is a workaround for rdar://problem/14011860. Is that still up to date? GenericDispatch.swift seems to test it but it's used nowhere in the standard library.

Copy link
Contributor

@jrose-apple jrose-apple Jul 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Aha, yes, that's gone now that we have protocol extensions. Still, removing it entirely should probably be a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I'll have a look how much of it can be removed later on.

if (isa<FuncDecl>(AFD) && AFD->getNameStr() == "~>")
return true;

// If it's a function with a parameter with leading underscore, it's a
Expand Down Expand Up @@ -2557,8 +2557,7 @@ ClassDecl::ClassDecl(SourceLoc ClassLoc, Identifier Name, SourceLoc NameLoc,
}

DestructorDecl *ClassDecl::getDestructor() {
auto name = getASTContext().Id_deinit;
auto results = lookupDirect(name);
auto results = lookupDirect(DeclBaseName::createDestructor());
assert(!results.empty() && "Class without destructor?");
assert(results.size() == 1 && "More than one destructor?");
return cast<DestructorDecl>(results.front());
Expand Down Expand Up @@ -4572,7 +4571,20 @@ ObjCSelector AbstractFunctionDecl::getObjCSelector(
}

auto &ctx = getASTContext();
auto baseName = getName();

Identifier baseName;
if (isa<DestructorDecl>(this)) {
// Deinitializers are always called "dealloc".
return ObjCSelector(ctx, 0, ctx.Id_dealloc);
} else if (auto func = dyn_cast<FuncDecl>(this)) {
// Otherwise cast this to be able to access getName()
baseName = func->getName();
} else if (auto ctor = dyn_cast<ConstructorDecl>(this)) {
baseName = ctor->getName();
} else {
llvm_unreachable("Unknown subclass of AbstractFunctionDecl");
}

auto argNames = getFullName().getArgumentNames();

// Use the preferred name if specified
Expand All @@ -4596,11 +4608,6 @@ ObjCSelector AbstractFunctionDecl::getObjCSelector(
}
}

// Deinitializers are always called "dealloc".
if (isa<DestructorDecl>(this)) {
return ObjCSelector(ctx, 0, ctx.Id_dealloc);
}


// If this is a zero-parameter initializer with a long selector
// name, form that selector.
Expand Down Expand Up @@ -4960,9 +4967,10 @@ bool ConstructorDecl::isObjCZeroParameterWithLongSelector() const {
return params->get(0)->getInterfaceType()->isVoid();
}

DestructorDecl::DestructorDecl(Identifier NameHack, SourceLoc DestructorLoc,
ParamDecl *selfDecl, DeclContext *Parent)
: AbstractFunctionDecl(DeclKind::Destructor, Parent, NameHack, DestructorLoc,
DestructorDecl::DestructorDecl(SourceLoc DestructorLoc, ParamDecl *selfDecl,
DeclContext *Parent)
: AbstractFunctionDecl(DeclKind::Destructor, Parent,
DeclBaseName::createDestructor(), DestructorLoc,
/*Throws=*/false, /*ThrowsLoc=*/SourceLoc(),
/*NumParameterLists=*/1, nullptr) {
setSelfDecl(selfDecl);
Expand Down
2 changes: 1 addition & 1 deletion lib/AST/DeclContext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -806,7 +806,7 @@ unsigned DeclContext::printContext(raw_ostream &OS, unsigned indent) const {
break;
case DeclContextKind::AbstractFunctionDecl: {
auto *AFD = cast<AbstractFunctionDecl>(this);
OS << " name=" << AFD->getName();
OS << " name=" << AFD->getFullName();
if (AFD->hasInterfaceType())
OS << " : " << AFD->getInterfaceType();
else
Expand Down
2 changes: 2 additions & 0 deletions lib/AST/Identifier.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ using namespace swift;

void *DeclBaseName::SubscriptIdentifierData =
&DeclBaseName::SubscriptIdentifierData;
void *DeclBaseName::DestructorIdentifierData =
&DeclBaseName::DestructorIdentifierData;

raw_ostream &llvm::operator<<(raw_ostream &OS, Identifier I) {
if (I.get() == nullptr)
Expand Down
5 changes: 5 additions & 0 deletions lib/ClangImporter/SwiftLookupTable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,8 @@ DeclBaseName SerializedSwiftName::toDeclBaseName(ASTContext &Context) const {
return Context.getIdentifier(Name);
case DeclBaseName::Kind::Subscript:
return DeclBaseName::createSubscript();
case DeclBaseName::Kind::Destructor:
return DeclBaseName::createDestructor();
}
}

Expand Down Expand Up @@ -829,6 +831,9 @@ void SwiftLookupTable::dump() const {
case DeclBaseName::Kind::Subscript:
llvm::errs() << " subscript:\n";
break;
case DeclBaseName::Kind::Destructor:
llvm::errs() << " deinit:\n";
break;
}
const auto &entries = LookupTable.find(baseName)->second;
for (const auto &entry : entries) {
Expand Down
2 changes: 2 additions & 0 deletions lib/ClangImporter/SwiftLookupTable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@ struct SerializedSwiftName {
return Name;
case DeclBaseName::Kind::Subscript:
return "subscript";
case DeclBaseName::Kind::Destructor:
return "deinit";
}
}

Expand Down
4 changes: 2 additions & 2 deletions lib/Parse/ParseDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5768,8 +5768,8 @@ parseDeclDeinit(ParseDeclOptions Flags, DeclAttributes &Attributes) {
auto *SelfDecl = ParamDecl::createUnboundSelf(DestructorLoc, CurDeclContext);

Scope S(this, ScopeKind::DestructorBody);
auto *DD = new (Context) DestructorDecl(Context.Id_deinit, DestructorLoc,
SelfDecl, CurDeclContext);
auto *DD = new (Context) DestructorDecl(DestructorLoc, SelfDecl,
CurDeclContext);

// Parse the body.
if (Tok.is(tok::l_brace)) {
Expand Down
7 changes: 4 additions & 3 deletions lib/ParseSIL/ParseSIL.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,9 +434,6 @@ bool SILParser::parseSILIdentifier(Identifier &Result, SourceLoc &Loc,
// A binary operator can be part of a SILDeclRef.
Result = P.Context.getIdentifier(P.Tok.getText());
break;
case tok::kw_deinit:
Result = P.Context.Id_deinit;
break;
case tok::kw_init:
Result = P.Context.Id_init;
break;
Expand Down Expand Up @@ -1148,6 +1145,10 @@ bool SILParser::parseSILDottedPathWithoutPound(ValueDecl *&Decl,
P.consumeToken();
FullName.push_back(DeclBaseName::createSubscript());
break;
case tok::kw_deinit:
P.consumeToken();
FullName.push_back(DeclBaseName::createDestructor());
break;
default:
if (parseSILIdentifier(Id, diag::expected_sil_constant))
return true;
Expand Down
2 changes: 2 additions & 0 deletions lib/SIL/SILFunctionType.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1621,6 +1621,8 @@ static SelectorFamily getSelectorFamily(SILDeclRef c) {
return getSelectorFamily(declName.getIdentifier());
case DeclBaseName::Kind::Subscript:
return SelectorFamily::None;
case DeclBaseName::Kind::Destructor:
return SelectorFamily::None;
}
}
return SelectorFamily::None;
Expand Down
3 changes: 2 additions & 1 deletion lib/SIL/SILPrinter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,8 @@ static void printValueDecl(ValueDecl *Decl, raw_ostream &OS) {

if (Decl->isOperator()) {
OS << '"' << Decl->getBaseName() << '"';
} else if (Decl->getBaseName() == "subscript") {
} else if (Decl->getBaseName() == "subscript" ||
Decl->getBaseName() == "deinit") {
Copy link
Member Author

@ahoppen ahoppen Jul 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not quite sure if we should escape deinit in SIL if used as a normal identifier. For subscripts we needed to, since otherwise the names of subscript getters/setters could have clashed with the getters/setters of instance variables named "subscript". AFAIK destructors in SIL always have the suffix !destructor so there's no name clash. Should we strive for consistency between the two keywords or simplicity on deinit? See the changes in vtables.swift for an example of how the dotted paths look like.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still inclined to escape all keywords, always, but cc @swiftix, @jckarter.

Copy link
Member Author

Choose a reason for hiding this comment

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

@swiftix @jckarter Any opinion on whether "deinit" should be escaped or not?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I forgot @swiftix was out on vacation. He'll be back tomorrow.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Jordan. Escaping all keywords seems like the right conservative answer.

OS << '`' << Decl->getBaseName() << '`';
} else {
OS << Decl->getBaseName();
Expand Down
7 changes: 3 additions & 4 deletions lib/Sema/CSDiag.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2532,6 +2532,9 @@ diagnoseUnviableLookupResults(MemberLookupResult &result, Type baseObjTy,
if (memberName.getBaseName().getKind() == DeclBaseName::Kind::Subscript) {
diagnose(loc, diag::type_not_subscriptable, baseObjTy)
.highlight(baseRange);
} else if (memberName.getBaseName() == "deinit") {
// Specialised diagnostic if trying to access deinitialisers
diagnose(loc, diag::destructor_not_accessible).highlight(baseRange);
} else if (auto metatypeTy = baseObjTy->getAs<MetatypeType>()) {
auto instanceTy = metatypeTy->getInstanceType();
tryTypoCorrection();
Expand Down Expand Up @@ -2691,10 +2694,6 @@ diagnoseUnviableLookupResults(MemberLookupResult &result, Type baseObjTy,

return;
}
case MemberLookupResult::UR_DestructorInaccessible: {
diagnose(nameLoc, diag::destructor_not_accessible);
return;
}
}
}

Expand Down
5 changes: 0 additions & 5 deletions lib/Sema/CSSimplify.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3206,11 +3206,6 @@ performMemberLookup(ConstraintKind constraintKind, DeclName memberName,
// reasonable choice.
auto addChoice = [&](ValueDecl *cand, bool isBridged,
bool isUnwrappedOptional) {
// Destructors cannot be referenced manually
if (isa<DestructorDecl>(cand)) {
result.addUnviable(cand, MemberLookupResult::UR_DestructorInaccessible);
return;
}
// If the result is invalid, skip it.
TC.validateDecl(cand);
if (cand->isInvalid()) {
Expand Down
3 changes: 1 addition & 2 deletions lib/Sema/CodeSynthesis.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2185,8 +2185,7 @@ void TypeChecker::addImplicitDestructor(ClassDecl *CD) {

auto *selfDecl = ParamDecl::createSelf(CD->getLoc(), CD);

auto *DD = new (Context) DestructorDecl(Context.Id_deinit, CD->getLoc(),
selfDecl, CD);
auto *DD = new (Context) DestructorDecl(CD->getLoc(), selfDecl, CD);

DD->setImplicit();

Expand Down
3 changes: 0 additions & 3 deletions lib/Sema/ConstraintSystem.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -840,9 +840,6 @@ struct MemberLookupResult {

/// The member is inaccessible (e.g. a private member in another file).
UR_Inaccessible,

// A type's destructor cannot be referenced
UR_DestructorInaccessible,
};

/// This is a list of considered, but rejected, candidates, along with a
Expand Down
4 changes: 1 addition & 3 deletions lib/Sema/TypeCheckProtocol.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4969,12 +4969,10 @@ static void recordConformanceDependency(DeclContext *DC,
Conformance->getDeclContext()->getParentModule())
return;

auto &Context = DC->getASTContext();

// FIXME: 'deinit' is being used as a dummy identifier here. Really we
// don't care about /any/ of the type's members, only that it conforms to
// the protocol.
tracker->addUsedMember({Adoptee, Context.Id_deinit},
tracker->addUsedMember({Adoptee, DeclBaseName::createDestructor()},
DC->isCascadingContextForLookup(InExpression));
}

Expand Down
6 changes: 4 additions & 2 deletions lib/Serialization/Deserialization.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1623,6 +1623,8 @@ DeclBaseName ModuleFile::getDeclBaseName(IdentifierID IID) {
llvm_unreachable("Cannot get DeclBaseName of special module id");
case SUBSCRIPT_ID:
return DeclBaseName::createSubscript();
case serialization::DESTRUCTOR_ID:
return DeclBaseName::createDestructor();
case NUM_SPECIAL_IDS:
llvm_unreachable("implementation detail only");
}
Expand Down Expand Up @@ -1805,6 +1807,7 @@ ModuleDecl *ModuleFile::getModule(ModuleID MID) {
return clangImporter->getImportedHeaderModule();
}
case SUBSCRIPT_ID:
case DESTRUCTOR_ID:
llvm_unreachable("Modules cannot be named with special names");
case NUM_SPECIAL_IDS:
llvm_unreachable("implementation detail only");
Expand Down Expand Up @@ -3550,8 +3553,7 @@ ModuleFile::getDeclChecked(DeclID DID, Optional<DeclContext *> ForcedContext) {
if (declOrOffset.isComplete())
return declOrOffset;

auto dtor = createDecl<DestructorDecl>(ctx.Id_deinit, SourceLoc(),
/*selfpat*/nullptr, DC);
auto dtor = createDecl<DestructorDecl>(SourceLoc(), /*selfpat*/nullptr, DC);
declOrOffset = dtor;

configureGenericEnvironment(dtor, genericEnvID);
Expand Down
2 changes: 2 additions & 0 deletions lib/Serialization/ModuleFile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,8 @@ class ModuleFile::DeclTableInfo {
}
case static_cast<uint8_t>(DeclNameKind::Subscript):
return {DeclBaseName::Kind::Subscript, StringRef()};
case static_cast<uint8_t>(DeclNameKind::Destructor):
return {DeclBaseName::Kind::Destructor, StringRef()};
default:
llvm_unreachable("Unknown DeclNameKind");
}
Expand Down
Loading