Introduce special decl names #9989

ahoppen · 2017-05-30T21:04:33Z

After a long way we're finally here: The PR to introduce special names for subscripts and fix SR-2575.

Special DeclNames represent names that do not have an identifier in the surface language. This implies serializing the information about whether a name is special together with its identifier (if it is not special) in both the module file and the swift lookup table.

ahoppen · 2017-05-30T21:05:48Z

@swift-ci please test

ahoppen · 2017-05-31T14:22:19Z

@swift-ci Please test

swift-ci · 2017-05-31T14:23:33Z

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

swift-ci · 2017-05-31T14:25:54Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 8105cb6ddff4d68cf255525ecd410d4fedb2964d
Test requested by - @ahoppen

jrose-apple

Looking pretty good—all the comments are small. I think you could have split this out a little more (a big chunk of the Serialization and ClangImporter changes don't depend on special names existing yet), but it's all right.

We do still have one issue for down the road, which is that there's no syntax for #selector(getter:NSDictionary.subscript), which would refer to a property named "subscript". Maybe we'll have to go with something like #selector(getter:NSDictionary[])

jrose-apple · 2017-06-06T00:49:45Z

lib/AST/ASTMangler.cpp

-    appendIdentifier(decl->getBaseName().getIdentifier().str());
+    // FIXME: Should a mangled subscript name contain the string "subscript"?
+    switch (decl->getBaseName().getKind()) {
+      case DeclBaseName::Kind::Normal:


Nitpick: Swift/LLVM style doesn't indent case labels.

jrose-apple · 2017-06-06T00:59:18Z

lib/ClangImporter/SwiftLookupTable.h

@@ -45,6 +45,102 @@ class TypedefNameDecl;

 namespace swift {

+/// A name from a SwiftLookupTable that has not been deserialized into a
+/// DeclBaseName yet


Nitpick: missing period.

jrose-apple · 2017-06-06T01:01:16Z

lib/ClangImporter/SwiftLookupTable.h

+  }
+
+  /// Return a string representation of the name that can be used for sorting
+  StringRef sortStr() const {


Nitpick: I think this should be private for now. This partially comes from me still not being a fan of using the "str" abbreviation with any kind of description.

(also, missing period again)

This was meant to be in sync with DeclBaseName, so I'll change the name to userFacingName. Shall we still make it private or keep it public then?

jrose-apple · 2017-06-06T01:03:36Z

lib/IRGen/IRGenDebugInfo.cpp

@@ -400,8 +400,7 @@ class IRGenDebugInfoImpl : public IRGenDebugInfo {
        }

        SmallVector<char, 64> Buf;
-        // TODO: Handle special names
-        StringRef Name = (VD->getBaseName().getIdentifier().str() +
+        StringRef Name = (VD->getBaseName().userFacingName() +
                          Twine(Kind)).toStringRef(Buf);


@adrian-prantl, this is the no-functionality-change implementation, and I think it does make sense to land that first, but it's worth considering whether this makes sense to begin with. Subscripts can be overloaded.

jrose-apple · 2017-06-06T01:06:11Z

lib/Parse/ParseSIL.cpp

+    switch (P.Tok.getKind()) {
+    case tok::kw_subscript:
+      P.consumeToken();
+      FullName.push_back(DeclBaseName::createSubscript());


@gottesmm or @swiftix, is this correct behavior for finding "subscript" in a textual SIL dotted path? Or is that expected to be treated as a regular identifier? Or can it just not come up?

@gottesmm @swiftix Just a friendly ping as the question above still stands

I don't know if it answers the question but maybe I'll just comment on what's going on here:

Previously, subscripts were backed by the identifier "subscript" so no distinction between properties named "subscript" and proper subscripts was necessary. Now we need distinguish between the two. As per #6848 (comment) I decided to represent a property named "subscript" as "`subscript`" in a SIL dotted path while plain "subscript" (without the backticks) still refers to a proper subscript.

So what I'm doing here is checking if a component in a SIL dotted path is the keyword subscript in order to translate it to a special DeclBaseName for subscripts. If "subscript" is spelled in backticks, the token kind will be "backtick", the default branch is taken and parseSILIdentifier will parse "`subscript`" as an identifier named "subscript", producing a dotted path that refers to a property named "subscript".

@jckarter says this solution sounds good.

jrose-apple · 2017-06-06T01:13:22Z

lib/Serialization/Serialization.cpp

@@ -85,16 +85,21 @@ namespace {
    using offset_type = unsigned;

    hash_value_type ComputeHash(key_type_ref key) {
-      assert(!key.empty());


Please keep this assertion in the Normal branch; it's caught bugs before. :-)

jrose-apple · 2017-06-06T01:14:04Z

lib/Serialization/Serialization.cpp

-  IdentifiersToWrite.push_back(ident.getIdentifier());
-  return id;
+  switch (ident.getKind()) {
+    case DeclBaseName::Kind::Normal: {


Nitpick: indented switch again.

jrose-apple · 2017-06-06T01:15:02Z

tools/SourceKit/lib/SwiftLang/SwiftDocSupport.cpp

@@ -1138,6 +1138,7 @@ void SwiftLangSupport::getDocInfo(llvm::MemoryBuffer *InputBuf,
    if (Error)
      Consumer.failed("Error occurred");
    return;
+


Spurious change?

jrose-apple · 2017-06-06T01:15:24Z

validation-test/Sema/type_checker_crashers_fixed/rdar27017206.swift

@@ -1,4 +1,4 @@
-// RUN: not --crash %target-swift-frontend %s -typecheck
+// RUN: not %target-swift-frontend %s -typecheck
 // REQUIRES: asserts


Nitpick: No more "REQUIRES: asserts" needed.

jrose-apple · 2017-06-06T01:19:11Z

test/Sema/subscripting.swift

@@ -0,0 +1,16 @@
+// RUN: %target-run-simple-swift | %FileCheck %s


Nitpick: test/Sema isn't supposed to have execution tests. Please move this to test/Interpreter (which has a number of non-interpreter execution tests at the moment) and add "REQUIRES: executable_test".

ahoppen · 2017-06-30T16:45:02Z

@swift-ci Please test

swift-ci · 2017-06-30T16:45:23Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 3d1f71627f63b90c293e213c1f258a49b6652e78
Test requested by - @ahoppen

swift-ci · 2017-06-30T16:46:03Z

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

ahoppen · 2017-07-06T11:37:34Z

Resolved merge conflicts

@swift-ci Please test

swift-ci · 2017-07-06T11:38:55Z

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

swift-ci · 2017-07-06T11:39:44Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 710dd93a332ae1512a91171408fc06a8400c2afd
Test requested by - @ahoppen

ahoppen · 2017-07-07T07:55:53Z

@jrose-apple I've addressed all the comments in your last review and the PR is ready from my side. Could you review the changes if you've got time?

jrose-apple

Just some small comments on this last version, but other than these this looks good to me! No need to wait for me after fixing this round.

jrose-apple · 2017-07-07T15:17:42Z

include/swift/AST/Identifier.h

@@ -206,15 +208,45 @@ namespace swift {

 /// Wrapper that may either be an Identifier or a special name
 /// (e.g. for subscripts)
+///
+/// The raw values of this enum are used in serialization by both


This comment should no longer be necessary, right? (Also, it belongs on the enum, not the class!)

I'm still using them for serialisation in ClangImporter but you said that they don't need to be stable for that, correct?

Yep, that's correct.

jrose-apple · 2017-07-07T15:18:40Z

include/swift/AST/KnownIdentifiers.def

@@ -90,7 +90,6 @@ IDENTIFIER(simd)
 IDENTIFIER(some)
 IDENTIFIER(storage)
 IDENTIFIER(stringValue)
-IDENTIFIER(subscript)


jrose-apple · 2017-07-07T15:20:34Z

lib/AST/ASTMangler.cpp

@@ -540,8 +540,15 @@ void ASTMangler::appendDeclName(const ValueDecl *decl) {
        break;
    }
  } else if (decl->hasName()) {
-    // TODO: Handle special names
-    appendIdentifier(decl->getBaseName().getIdentifier().str());
+    // FIXME: Should a mangled subscript name contain the string "subscript"?


Something @eeckstein may have an opinion on, but can be changed in a follow-up commit.

I wanted to start a thread about that once the changes have been landed

jrose-apple · 2017-07-07T15:26:58Z

test/SILGen/vtables.swift

+
+// CHECK-LABEL: sil_vtable SubscriptAsFunction {
+// CHECK-NOT:     #SubscriptAsFunction.subscript
+// CHECK:         #SubscriptAsFunction.`subscript`!1


There should be a SIL parser test for this too, even if just a basic one.

jrose-apple · 2017-07-07T15:30:12Z

lib/Serialization/Serialization.cpp

-      out << key.getIdentifier().str();
+      endian::Writer<little> writer(out);
+      switch (key.getKind()) {
+        case DeclBaseName::Kind::Normal:


Nitpick: LLVM style does not indent switch cases.

Special DeclNames represent names that do not have an identifier in the surface language. This implies serializing the information about whether a name is special together with its identifier (if it is not special) in both the module file and the swift lookup table.

ahoppen · 2017-07-11T18:30:46Z

@swift-ci Please test

swift-ci · 2017-07-11T18:31:41Z

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

swift-ci · 2017-07-11T18:34:04Z

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

…iptExprs. Interactions with the rest of the type checker cause use of functions named `subscript` with backtick-quotes to become ambiguous. This is more fully and robustly solved in master by swiftlang#9989, but that's too invasive a change for 4.0, and we need a spot fix to prevent source compatibility regressions such as SR-5513.

ahoppen requested a review from jrose-apple May 30, 2017 21:04

ahoppen force-pushed the introduce-special-declnames branch from 8105cb6 to 3d1f716 Compare May 31, 2017 14:22

jrose-apple reviewed Jun 6, 2017

View reviewed changes

ahoppen force-pushed the introduce-special-declnames branch 2 times, most recently from 6ff4737 to 710dd93 Compare June 30, 2017 16:44

ahoppen force-pushed the introduce-special-declnames branch from 710dd93 to ded6aed Compare July 6, 2017 11:37

jrose-apple approved these changes Jul 7, 2017

View reviewed changes

ahoppen added 2 commits July 11, 2017 19:04

Add test cases for special subscript name

2dd670b

ahoppen force-pushed the introduce-special-declnames branch from ded6aed to 2dd670b Compare July 11, 2017 17:04

ahoppen merged commit ec6fc4d into swiftlang:master Jul 11, 2017

ahoppen deleted the introduce-special-declnames branch July 12, 2017 14:12

jckarter mentioned this pull request Jul 20, 2017

[4.0] Don't consider KeyPathApplication operations when looking up non-SubscriptExprs. #11089

Merged

ahoppen mentioned this pull request Jul 14, 2017

[SR-2575] Crash on strHex.subscript(p) (instead of strHex[p]) #45180

Closed

		@@ -0,0 +1,16 @@
		// RUN: %target-run-simple-swift \| %FileCheck %s

Introduce special decl names #9989

Introduce special decl names #9989

Uh oh!

Conversation

ahoppen commented May 30, 2017

Uh oh!

ahoppen commented May 30, 2017

Uh oh!

ahoppen commented May 31, 2017

Uh oh!

swift-ci commented May 31, 2017

Uh oh!

swift-ci commented May 31, 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!

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!

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 Jun 30, 2017

Uh oh!

swift-ci commented Jun 30, 2017

Uh oh!

swift-ci commented Jun 30, 2017

Uh oh!

ahoppen commented Jul 6, 2017

Uh oh!

swift-ci commented Jul 6, 2017

Uh oh!

swift-ci commented Jul 6, 2017

Uh oh!

ahoppen commented Jul 7, 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!

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 11, 2017

Uh oh!