Introduce a special DeclBaseName for "deinit" #10965

ahoppen · 2017-07-14T08:17:44Z

Problem statement

Currently, destructors are represented by the known identifier "deinit", making it impossible to have a function or instance variable named "deinit" in a class. Declaring a function named "deinit" currently results in an error about an invalid redeclaration of deinit and adding an instance variable named "deinit" crashes the compiler.

Solution

Similar to how special DeclNames were introduced for subscripts to avoid the name collision between functions named "subscript" and proper subscripts, introduce a special DeclName for destructors and remove "deinit" from the list of known identifiers, making the identifier named "deinit" and the name representing the destructor completely distinct and thus resolving the above issue.

ahoppen · 2017-07-14T08:17:58Z

@swift-ci Please test

ahoppen · 2017-07-14T08:20:57Z

lib/AST/Decl.cpp

@@ -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() == "~>")


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.

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

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

ahoppen · 2017-07-14T08:26:43Z

lib/SIL/SILPrinter.cpp

@@ -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") {


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.

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

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

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

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

ahoppen · 2017-07-14T10:50:57Z

@swift-ci Please test

swift-ci · 2017-07-14T10:51:13Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 14f1d5fb1c4d211a3b59d769b0466c583a189e37
Test requested by - @ahoppen

swift-ci · 2017-07-14T10:54:14Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 14f1d5fb1c4d211a3b59d769b0466c583a189e37
Test requested by - @ahoppen

jrose-apple

Wow, this fell out very nicely. Good work, Alex.

jrose-apple · 2017-07-14T16:14:22Z

lib/AST/ASTMangler.cpp

@@ -548,6 +548,9 @@ void ASTMangler::appendDeclName(const ValueDecl *decl) {
    case DeclBaseName::Kind::Subscript:
      appendIdentifier("subscript");
      break;
+    case DeclBaseName::Kind::Destructor:
+      // FIXME: Is it true that this is unreachable?


I'm 90% sure this is true but try defining a local type inside a destructor to make sure.

OK, thanks for the reassurance.

jrose-apple · 2017-07-14T16:16:46Z

include/swift/AST/Decl.h

@@ -5647,6 +5647,8 @@ class ConstructorDecl : public AbstractFunctionDecl {
                  GenericParamList *GenericParams, 
                  DeclContext *Parent);

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


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

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.

dabrahams · 2017-07-14T17:36:56Z

The way the PR description is phrased this sounds like some kind of troll. Just sayin' .

ahoppen · 2017-07-14T18:21:58Z

@dabrahams That's certainly not what I wanted. Thanks for the feedback. I've updated it and hope it sounds more serious now.

This name is not used yet

ahoppen · 2017-07-28T09:16:59Z

@jrose-apple Could you have another look through this? I think all the questions have been answered and your comments taken care of.

@swift-ci Please test

swift-ci · 2017-07-28T09:37:21Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 8a839ed81cafbb8de0f163d1d202d876fcdb0e01
Test requested by - @ahoppen

swift-ci · 2017-07-28T10:05:13Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 8a839ed81cafbb8de0f163d1d202d876fcdb0e01
Test requested by - @ahoppen

jrose-apple

We left off with the question about escaping in SIL, and Joe agreed with me that it would be better to escape everything. But that doesn't have to be part of this patch, which is just about deinit. Yes, this looks good now!

No longer use the known identifier `deinit` for destructors. This allows classes to have functions called `deinit`.

ahoppen · 2017-07-28T17:40:00Z

Ah, I see, I thought by all keywords you meant escaping all keywords that clashed with special decl names. I'll create another PR for it then.

@swift-ci Please test and merge

ahoppen · 2017-08-01T13:38:24Z

Test once again before merging since old test is already several days old.
Btw: Any idea why swift-ci wasn't able to merge the PR?

@swift-ci Please smoke test and merge

ahoppen · 2017-08-01T14:23:55Z

swift-ci didn't run. Let's try again

@swift-ci Please smoke test and merge

ahoppen commented Jul 14, 2017

View reviewed changes

ahoppen force-pushed the deinit-special-name branch from 14f1d5f to c13bbbe Compare July 14, 2017 09:49

ahoppen changed the title ~~WIP: Special name for deinit~~ Special name for deinit Jul 14, 2017

ahoppen requested a review from jrose-apple July 14, 2017 15:16

jrose-apple reviewed Jul 14, 2017

View reviewed changes

ahoppen force-pushed the deinit-special-name branch from c13bbbe to 5002fce Compare July 14, 2017 17:09

ahoppen changed the title ~~Special name for deinit~~ Introduce a special DeclBaseName for deinit Jul 14, 2017

ahoppen changed the title ~~Introduce a special DeclBaseName for deinit~~ Introduce a special DeclBaseName for "deinit" Jul 14, 2017

ahoppen force-pushed the deinit-special-name branch 2 times, most recently from 418ff0d to d18f196 Compare July 19, 2017 09:58

Introduce special name for destructors

2eb36e4

This name is not used yet

ahoppen force-pushed the deinit-special-name branch from d18f196 to 8a839ed Compare July 28, 2017 08:47

jrose-apple approved these changes Jul 28, 2017

View reviewed changes

Represent the name of destructors by a special DeclBaseName

ebd701c

No longer use the known identifier `deinit` for destructors. This allows classes to have functions called `deinit`.

ahoppen force-pushed the deinit-special-name branch from 8a839ed to ebd701c Compare July 28, 2017 17:20

swift-ci merged commit 17ddc58 into swiftlang:master Aug 1, 2017

ahoppen deleted the deinit-special-name branch August 1, 2017 16:32

ahoppen mentioned this pull request Aug 1, 2017

Follow up for: Introduce a special DeclBaseName for "deinit" #11287

Closed

jrose-apple mentioned this pull request Aug 1, 2017

[swift-api-digester] Update for 'deinit' as a special name. #11288

Merged

ahoppen mentioned this pull request Aug 2, 2017

Escape all keywords when printing DeclBaseNames #11309

Merged

Introduce a special DeclBaseName for "deinit" #10965

Introduce a special DeclBaseName for "deinit" #10965

Uh oh!

Conversation

ahoppen commented Jul 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem statement

Solution

Uh oh!

ahoppen commented Jul 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple Jul 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen Jul 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Jul 14, 2017

Uh oh!

swift-ci commented Jul 14, 2017

Uh oh!

swift-ci commented Jul 14, 2017

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dabrahams commented Jul 14, 2017

Uh oh!

ahoppen commented Jul 14, 2017

Uh oh!

ahoppen commented Jul 28, 2017

Uh oh!

swift-ci commented Jul 28, 2017

Uh oh!

swift-ci commented Jul 28, 2017

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Jul 28, 2017

Uh oh!

ahoppen commented Aug 1, 2017

Uh oh!

ahoppen commented Aug 1, 2017

Uh oh!

Uh oh!

ahoppen commented Jul 14, 2017 •

edited

Loading

jrose-apple Jul 14, 2017 •

edited

Loading

ahoppen Jul 14, 2017 •

edited

Loading