Use DeclName instead of Identifier in various locations #6305

ahoppen · 2016-12-15T11:52:11Z

This is a preparation to use special DeclNames that don't carry identifiers e.g. for subscripts, initializers, and destructors which shall at some point fix SR-2575 as outlined in #5537.

This is a preparation to use special DeclNames that don't carry identifiers e.g. for subscripts, initializers, and destructors.

ahoppen · 2016-12-15T11:54:20Z

Please test with following pull request:
apple/swift-lldb#106

@swift-ci Please test

jrose-apple

I got part-way through this and realized I'm very uncomfortable with it. Currently the distinction between Identifier and DeclName makes it clear where a name may be compound or not. At the same time, the distinction we're going for—between "magic names" and plain old strings—also makes sense. Maybe we'd do better with a third type between Identifier and DeclName. As is, this change makes it too easy to accidentally start passing compound names places that would have been statically rejected.

jrose-apple · 2016-12-15T17:00:11Z

include/swift/AST/Identifier.h

@@ -276,13 +276,26 @@ class DeclName {
  /// Retrieve the 'base' name, i.e., the name that follows the introducer,
  /// such as the 'foo' in 'func foo(x:Int, y:Int)' or the 'bar' in
  /// 'var bar: Int'.
-  Identifier getBaseName() const {
+  Identifier getIdentifier() const {


This isn't really correct. You should only be able to call getIdentifier on a simple DeclName, since compound ones have multiple identifiers. And then I'd consider naming it something different too.

jrose-apple · 2016-12-15T17:00:36Z

include/swift/AST/Identifier.h

+  DeclName getBaseName() const {
+    DeclName base;
+    if (auto compound = SimpleOrCompound.dyn_cast<CompoundDeclName*>()) {
+     base = compound->BaseName;


Nitpick: wrong indentation here.

jrose-apple · 2016-12-15T17:02:18Z

include/swift/AST/Identifier.h

@@ -386,6 +403,14 @@ class DeclName {
    return lhs.compare(lhs) >= 0;
  }

+  bool operator ==(StringRef other) {


Please do not add these members. Anything that operates only on the base name wouldn't be a correct == anyway.

jrose-apple · 2016-12-15T17:03:01Z

include/swift/AST/Identifier.h

@@ -395,6 +420,16 @@ class DeclName {
  StringRef getString(llvm::SmallVectorImpl<char> &scratch,
                      bool skipEmptyArgumentNames = false) const;

+  /// Return the string representation of the name
+  StringRef str() const {


This is just not correct; it drops the compound part on the floor. Please remove these.

jrose-apple · 2016-12-15T17:03:47Z

include/swift/AST/Mangle.h

@@ -152,7 +152,7 @@ class Mangler {
  void mangleFunctionType(AnyFunctionType *fn, unsigned uncurryingLevel);
  void mangleProtocolList(ArrayRef<ProtocolDecl*> protocols);
  void mangleProtocolList(ArrayRef<Type> protocols);
-  void mangleIdentifier(Identifier ident,
+  void mangleDeclName(DeclName name,
                        OperatorFixity fixity = OperatorFixity::NotOperator);


Nitpick: please keep the parameters aligned.

jrose-apple · 2016-12-15T17:06:28Z

include/swift/Serialization/ModuleFile.h

@@ -703,7 +703,7 @@ class ModuleFile : public LazyMemberLoader {
  Type getType(serialization::TypeID TID);

  /// Returns the identifier with the given ID, deserializing it if needed.
-  Identifier getIdentifier(serialization::IdentifierID IID);
+  Identifier getIdentifier(serialization::DeclNameID DID);


Either this should return DeclNames or IdentifierID should remain as is.

jrose-apple · 2016-12-15T17:06:58Z

include/swift/Serialization/ModuleFormat.h

@@ -66,7 +66,7 @@ using TypeIDField = DeclIDField;
 using TypeIDWithBitField = BCFixed<32>;

 // IdentifierID must be the same as DeclID because it is stored in the same way.
-using IdentifierID = DeclID;
+using DeclNameID = DeclID;
 using IdentifierIDField = DeclIDField;


…and these two should always be consistent.

ahoppen · 2016-12-15T20:22:24Z

I rather thought of this as a way to maybe also allow compound DeclNames in places where it isn't currently allowed. I don't know the current status but afaik compound DeclNames for variables may come in handy to assign functions to variables and still be able to call them with named parameters, e.g.

var foo(bar, baz): (Int, Int) -> Int = …
foo(bar: 1, baz: 5)

For some Decls, compound DeclNames definitely don't make sense, but I think this can be enforced by only offering a constructor that takes an Identifier and then extracting the base identifier of the stored DeclName in a method getIdentifier.

Let's maybe just do a case study on the various Decls a Swift AST can have and their status in the PR:

ImportDecl/ExtensionDecl/PatternBindingDecl/EnumCaseDecl/TopLevelCodeDecl/IfConfigDecl: Don't use the DeclName
PrecedenceGroupDecl: Simple name enforced by constructor, getIdentifier still needs to be written
- Compound DeclNames don't make sense to me here
- Side note: Why isn't this a TypeDecl like ModuleDecl is one?
OperatorDecl: Has its own name storage of type Identifier and doesn't use the DeclName property in the Decl superclass
- seems weird to me, but otherwise I'd go for: Simple name enforced by the constructor, write getIdentifier
- Support of compound DeclNames doesn't make sense to me
TypeDecl: Simple name enforced by constructor, getIdentifier exists
- compound DeclNames don't make sense to me
VarDecl: I think that a compound DeclName could come in handy as described above
ParamDecl: Simple name enforced by constructor, getIdentifier exists
- Could maybe be extended to compound DeclNames to allow passing parameter closures with named parameters, though this would mess up the whole concept of DeclNames consisting of Identifiers
SubscriptDecl: Special DeclName needed
AbstractFunction: DeclName needed
EnumElement: Currently simple DeclName enforced by the constructor, getIdentifier exists.
- We could argue though that we would want to allow overloading of operators, e.g. case foo(x: Int), foo(y: Int). I think there even was a swift-evolution discussion about this recently, don't know for sure though

I think that, in particular if we want to go for the extensions I have described above, we can say that the ratio of compound DeclNames and simple DeclNames is about 50/50. Furthermore, for all other cases the usage of simple Identifiers is enforced because the respective Decls only take an Identifier in their constructors. I don't actually think we lose a lot of static checking here.

The “unsafe” method getIdentifier on DeclName is currently invoked at ~75 places in the source code, maybe we could try to limit these usages a bit further.

ahoppen · 2017-01-05T21:40:56Z

@jrose-apple Just a friendly ping for feedback on my last comment. My gut tells me that a thorough distinction between compound and simple DeclNames will at the moment cause more trouble than it solves at the moment because conversions from DeclName to Identifier would be needed. Or could you maybe point out a few concrete source locations where you think strong simple name typing would be helpful?

jrose-apple · 2017-01-06T00:36:21Z

Sorry, I've been avoiding this a bit since I think we don't want to take such a drastic change until after the branch for 3.1 on the 16th. Let's see…

I think I'd rather not look ahead to the future of making VarDecls use compound names. This refactoring should not include any language changes in it directly. With or without that, though, there are still plenty of places where Identifier is the right thing to use. Maybe another type isn't the way to go, but using DeclName for everything definitely isn't what I want.

Side note: Why isn't this a TypeDecl like ModuleDecl is one?

ModuleDecl is a TypeDecl because it has members that can be looked up like any other members. That's really the only reason; for a long time ModuleDecl wasn't a TypeDecl, and before that it wasn't even a Decl at all.

Has its own name storage of type Identifier and doesn't use the DeclName property in the Decl superclass

Not all Decls have names. The DeclName property lives on ValueDecl, not Decl. (Admittedly not all ValueDecls have names either, nor are they all values—accessors don't fit either category even though they're modeled as FuncDecls.)

ahoppen · 2017-01-16T19:47:19Z

The new, more conservative approach is now being tracked in #6848.

Use DeclName instead of Identifier in various locations

2eb95d4

This is a preparation to use special DeclNames that don't carry identifiers e.g. for subscripts, initializers, and destructors.

ahoppen mentioned this pull request Dec 15, 2016

Use DeclNames instead of Identifiers in more places and consider special DeclNames apple/swift-lldb#106

Closed

ahoppen mentioned this pull request Dec 15, 2016

[TypeChecker] Issue error message when accessing subscript using keyword #5537

Closed

ahoppen requested a review from jrose-apple December 15, 2016 15:13

jrose-apple suggested changes Dec 15, 2016

View reviewed changes

ahoppen mentioned this pull request Jan 16, 2017

Introduce special DeclNames for subscripts #6848

Closed

ahoppen closed this Jan 16, 2017

ahoppen deleted the SR-2575-2-Identifier-to-DeclName branch June 26, 2017 17:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Use DeclName instead of Identifier in various locations #6305

Use DeclName instead of Identifier in various locations #6305

Uh oh!

ahoppen commented Dec 15, 2016

Uh oh!

ahoppen commented Dec 15, 2016

Uh oh!

jrose-apple left a comment

Uh oh!

jrose-apple Dec 15, 2016

Uh oh!

jrose-apple Dec 15, 2016

Uh oh!

jrose-apple Dec 15, 2016

Uh oh!

jrose-apple Dec 15, 2016

Uh oh!

jrose-apple Dec 15, 2016

Uh oh!

jrose-apple Dec 15, 2016

Uh oh!

jrose-apple Dec 15, 2016

Uh oh!

ahoppen commented Dec 15, 2016 •

edited

Loading

Uh oh!

ahoppen commented Jan 5, 2017

Uh oh!

jrose-apple commented Jan 6, 2017 •

edited

Loading

Uh oh!

ahoppen commented Jan 16, 2017

Uh oh!

Uh oh!

Use DeclName instead of Identifier in various locations #6305

Use DeclName instead of Identifier in various locations #6305

Uh oh!

Conversation

ahoppen commented Dec 15, 2016

Uh oh!

ahoppen commented Dec 15, 2016

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

jrose-apple Dec 15, 2016

Choose a reason for hiding this comment

Uh oh!

jrose-apple Dec 15, 2016

Choose a reason for hiding this comment

Uh oh!

jrose-apple Dec 15, 2016

Choose a reason for hiding this comment

Uh oh!

jrose-apple Dec 15, 2016

Choose a reason for hiding this comment

Uh oh!

jrose-apple Dec 15, 2016

Choose a reason for hiding this comment

Uh oh!

jrose-apple Dec 15, 2016

Choose a reason for hiding this comment

Uh oh!

jrose-apple Dec 15, 2016

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Dec 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahoppen commented Jan 5, 2017

Uh oh!

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

Uh oh!

Uh oh!

ahoppen commented Jan 16, 2017

Uh oh!

Uh oh!

ahoppen commented Dec 15, 2016 •

edited

Loading

jrose-apple commented Jan 6, 2017 •

edited

Loading