Allow non-mutable properties and subscripts returning Self #23991

johnno1962 · 2019-04-12T20:22:41Z

Hi Apple,

further eases restrictions on recent PRs #22863 and #23485 to allow properties and subscripts that return Self, provided they are not settable. Will need to be cherrypicked into 5.1 cc @slavapestov.

Resolves #52726.

slavapestov

Looks like a good start, thanks for digging into this.

slavapestov · 2019-04-14T11:39:00Z

lib/AST/Type.cpp

@@ -2297,9 +2297,15 @@ static bool matches(CanType t1, CanType t2, TypeMatchOptions matchMode,
  }

  // Class-to-class.
-  if (matchMode.contains(TypeMatchFlags::AllowOverride))
-    if (t2->isExactSuperclassOf(t1))
+  if (matchMode.contains(TypeMatchFlags::AllowOverride)) {


Since this wasn't necessary for method overrides to work, I suspect the right change is to actually look at the caller of this method in TypeCheckDeclOverride.cpp. I bet we're calling eraseDynamicSelfType() somewhere for method types but not property types. Can you take a look?

slavapestov · 2019-04-14T11:40:05Z

lib/Sema/CSApply.cpp

@@ -888,6 +888,13 @@ namespace {
            }
          }
        }
+        else if (isa<DynamicSelfType>(member->getInterfaceType()


For methods, we also allow a return type of Self?, so properties should allow that too. This is not handled here. You can instead try member->getInterfaceType()->hasDynamicSelfType().

slavapestov · 2019-04-14T11:40:31Z

lib/Sema/ConstraintSystem.cpp

@@ -1297,6 +1297,10 @@ ConstraintSystem::getTypeOfMemberReference(
        openedType = openedType->replaceCovariantResultType(baseObjTy, 2);
      }
    }
+    else if (isa<DynamicSelfType>(value->getInterfaceType()


Same comment as in CSApply

slavapestov · 2019-04-14T11:41:28Z

test/type/self.swift

@@ -110,7 +110,7 @@ class B: A<Int> {
    let copy = super.copy() as! Self // supported
    return copy
  }
-  override var copied: Self { // expected-error {{'Self' is only available in a protocol or as the result of a method in a class; did you mean 'B'?}}
+  override var copied: Self {


You mentioned you got your end-to-end examples to compile and run. It would be great if you would also add some SILGen and execution tests (look at test/Interpreter/dynamic_self.swift) to make sure the functionality does not regress.

slavapestov · 2019-04-14T11:57:35Z

lib/Sema/TypeCheckType.cpp

@@ -1056,6 +1058,13 @@ static Type SelfAllowedBySE0068(TypeResolution resolution,
    bool isTypeAliasInClass = insideClass &&
      options.is(TypeResolverContext::TypeAliasDecl);

+    if (isMutablePropertyOrSubscriptOfClass) {


Ok, so unfortunately I still need to nitpick this method. I'll try to explain what the problem is.

Before you implemented SE-0068, we would return DynamicSelfType in a very specific case -- when options.getBaseContext() == TypeResolverContext::DynamicSelfResult was true. This got set in a somewhat round-about way: when type checking a function definition, we would look into the result TypeRepr (a TypeRepr is the representation of an unchecked type -- think of it as an AST for types written in source). If the TypeRepr was an identifier named "Self", or an optional of an identifier named "Self", we would set this flag.

For declarations other than methods, or if a method had a return type that did not exactly match this pattern (for example, (Self, Self) or Self.Type or () -> Self), options.getBaseContext() == TypeResolverContext::DynamicSelfResult was false.

Now since you're allowing Self to be written in the type of an immutable property or subscript unconditionally, you're going to be able to get properties with types like:

var foo: (Self, Self) { ... } var foo: () -> Self { ... } var foo: Self.Type { ... }

The code in CSApply does not handle these cases; it can only construct a member reference to something with a type of Self or Self?. We can fix that with enough effort, but you'll still have to ban code like:

var foo: (Self) -> () { ... } var foo: Array<Self> { ... }

Where Self is not in covariant position.

I now think the right way to go about that is to not try to guess in TypeCheckType.cpp whether Self is allowed or not. Instead, when you're inside a class, always build a DynamicSelfType. Then when checking declarations in DeclChecker in TypeCheckDecl.cpp, enforce the various rules for each kind of decl:

TypeAliasDecl cannot have Self in its type at all, unless its in local context

FuncDecl can only have Self as its return type

VarDecl can only have Self as its type if its immutable and computed

Same for SubscriptDecl and its element type

For nested nominal types this check would need to ban Self from appearing in the generic signature; this is invalid for instance:

class Outer { class Inner<S> where S : Sequence, S.Element == Self { ... } }

So I would implement a new function, like checkDynamicSelfResult(), that is called on class members from inside the various DeclChecker visitor methods. It would implement the rules and emit diagnostics for each invalid case.

Then you can get rid of TypeResolverContext::DynamicSelfResult altogether, and unify the two spots in TypeCheckType.cpp where you produce a DynamicSelfType.

slavapestov · 2019-04-14T12:01:51Z

lib/Sema/TypeCheckType.cpp

@@ -1056,6 +1058,13 @@ static Type SelfAllowedBySE0068(TypeResolution resolution,
    bool isTypeAliasInClass = insideClass &&
      options.is(TypeResolverContext::TypeAliasDecl);

+    if (isMutablePropertyOrSubscriptOfClass) {
+      if (auto prop = dyn_cast_or_null<ValueDecl>
+          (dc->getInnermostDeclarationDeclContext()))


How does this work for VarDecls? A VarDecl is not a DeclContext.

slavapestov · 2019-04-14T12:02:44Z

test/type/self.swift

@@ -77,7 +77,7 @@ class A<T> {
    return copy
  }

-  var copied: Self { // expected-error {{'Self' is only available in a protocol or as the result of a method in a class; did you mean 'A'?}}
+  var copied: Self {


You need tests showing that mutable properties and subscripts are rejected

johnno1962 · 2019-04-14T14:21:39Z

Thanks for the tips. I've pushed another commit which takes things forward from where they were yesterday when I was a little over-zealous in tidying up the code. Things generally work except I can't get the covariant applied type right when properties or subscripts returning self are used from a derived class.

For example using this code:

class A0<T, S> {
  var i = 88
  required init() {}
  var foo: (Self) -> () { return { (a: Self) in } }

  func copy() -> Self {
    let copy = Self.init()
    return copy
  }

  var copied: Self {
    let copy = Self.init()
    return copy
  }
  open subscript (i: Int) -> Self {
    return Self.init()
  }
}

protocol Prot3 {
  static func +(x: Self, y: Self) -> Int
}

class B: A0<Int, Double>, Prot3 {
  var j = 77
  static func + (x: B, y: B) -> Int {
    return 99
  }
}

print(B().copied.j)

The error I get is:

TYPE MISMATCH IN ARGUMENT 0 OF APPLY AT expression at [j.swift:81:7 - line:81:18] RangeText="B().copied."
  argument value:   %17 = begin_borrow %14 : $A0<Int, Double>       // user: %18
  parameter type: $B

or for print(B()[0].j)

TYPE MISMATCH IN ARGUMENT 0 OF APPLY AT expression at [j.swift:81:7 - line:81:14] RangeText="B()[0]."
  argument value:   %21 = begin_borrow %18 : $A0<Int, Double>       // user: %22
  parameter type: $B

Whereas print(B().copy().j) works of course, so what is the difference? This is probably going to need to be fixed in two places but where????? I hear what you're saying about Self in non-covariant position in non-mutable properties getting through. That may mean we have to give up.

slavapestov · 2019-04-14T18:15:19Z

The type mismatch is happening because you did not emit a CovariantReturnConversionExpr. It should be easy to add one in the right place.

Also please don’t give up on diagnosing invalid Self usages :) if you want help I can do part or all of that refactoring. It’s long overdue anyway; I’ve wanted to get rid of FuncDecl::hasDynamicSelf() and the code that computes it for a while.

slavapestov · 2019-04-15T14:58:00Z

The merge conflict is because a branch in CSApply was taken for TypeDecl and VarDecl. The TypeDecl case didn’t make a lot of sense and necessitated a complementary hack in SILGen. You can ignore the new code and resolve the conflict triviallyZ

johnno1962 · 2019-04-15T15:25:01Z

I've fixed the conflict and I think I've taken things about as far as I can. After juggling the pieces for longer might be reasonable I was able to get CovariantReturnConversionExpr working in the finish (Thanks for the tip - on a Sunday no less) and it solved the type mismatch I was seeing. There may be other problems & I'm building a toolchain for more detailed testing. I'll not take on the "non-Covariant position, Self" refactor if you don't mind as it seems you have a pretty clear idea about how to go about it and I really don't have a clue!

johnno1962 · 2019-04-15T16:42:03Z

There is one known problem, if you have a subscript override in a subclass that returns Self, the solution is seen but rejected. For:

class A0<T, S> {
    var i = "Base"
    required init() {}
    var foo: (Self) -> () { return { (a: Self) in } }

    func copy() -> Self {
        let copy = Self.init()
        return copy
    }

    var copied: Self {
        get {
            let copy = Self.init()
            return copy
        }
    }
    open subscript (i: Int) -> Self {
        get {
            return Self.init()
        }
    }
}

protocol Prot3 {
    static func +(x: Self, y: Self) -> String
}

class B: A0<Int, Double>, Prot3 {
    var j = "Derived"
    static func + (x: B, y: B) -> String {
        return x.j + x.j
    }
    override func copy() -> Self {
        let copy = Self.init()
        return copy
    }
    override var copied: Self {
        get {
            return copy() as! Self
        }
    }
//    override subscript (i: Int) -> Self {
//        return Self.init()
//    }
}

print(B()[0][0].j + B().copied.copied.j)
print(B()[0][0] + B().copied.copied)

If you uncomment the subscript in the derived class it fails with:

j.swift:92:10: error: cannot subscript a value of type 'B' with an argument of type 'Int'
print(B()[0][0] + B().copied.copied)
      ~~~^~~
j.swift:86:12: note: found this candidate
  override subscript (i: Int) -> Self {
           ^

I was seeing this on Friday and thought it was fixed but evidently not. It seems you can only define a subscript returning Self on a class that has a generic signature at the moment. I'd love to know why!

johnno1962 · 2019-04-16T17:27:09Z

Hi @slavapestov, I found the constraint which is failing and causing subscript not to be recognised on non-generic classes. It is here: https://github.com/apple/swift/blob/master/lib/Sema/CSGen.cpp#L1105. If I comment this out things are OK. No sign of why generic classes aren't affected.

johnno1962 · 2019-04-16T21:48:41Z

The trail leads here: https://github.com/apple/swift/blob/master/lib/Sema/CSSimplify.cpp#L2222. The code doesn't seem to be prepared for DynamicSelf so perhaps it is best to give up on the constraint.

johnno1962 · 2019-04-17T16:19:11Z

Hi @slavapestov, I've pushed an ultimately deeply unsatisfactory hack to stop solutions for subscripts on non-generic classes being rejected. I spent quite a bit of time digging into the constraint system as I mentioned above but the simpler hack is probably the best route with the most localised effect. In the end the following was sufficient to replicate the problem:

class Z {
  subscript (i: Int) -> Self {
    return self as! Self
  }
}

print(Z()[0])

slavapestov · 2019-04-17T21:00:28Z

I think the problem is that the non-generic case is not stripping off the DynamicSelfType somewhere, eg in getTypeOfMemberReference(). I don't want to add a boolean parameter to addConstraint(); imagine the effect it would have on code if such parameters accumulate over time.

slavapestov · 2019-04-17T21:02:13Z

lib/AST/Type.cpp

@@ -2285,7 +2285,7 @@ static bool matches(CanType t1, CanType t2, TypeMatchOptions matchMode,

  // Class-to-class.
  if (matchMode.contains(TypeMatchFlags::AllowOverride))
-    if (t2->isExactSuperclassOf(t1))
+    if (t2->eraseDynamicSelfType()->isExactSuperclassOf(t1))


This wasn't necessary for method overrides, so it shouldn't be necessary to add this to make variable and subscript overrides work.

This might be the same underlying problem as subscript overrides not working in generic classes

Ah, figured this one out. You need to teach TypeBase::adjustSuperclassMemberDeclType() about SubscriptDecl and VarDecl returning Self too.

I feel like the bit we want to factor out here that comes up in several places is the check for FuncDecl/ConstructorDecl/VarDecl/SubscriptDecl followed by a call to replaceCovariantResultType() with the right uncurry level. But we can clean that up later.

After a few attempts I was unable to adapt TypeBase::adjustSuperclassMemberDeclType() for properties and subscripts.

slavapestov · 2019-04-17T21:06:24Z

I'm going to build your branch and poke around a bit to see what's going on.

slavapestov · 2019-04-17T21:47:05Z

@johnno1962 I found the problem -- ConstraintSystem::getEffectiveOverloadType() has to be updated for subscripts returning Self (and properties too; please test this case as well). Here's a diff for ConstraintSystem.cpp that seems to fix the problem, but please test it more thoroughly:

@@ -1445,6 +1449,13 @@ Type ConstraintSystem::getEffectiveOverloadType(const OverloadChoice &overload,
 
       if (doesStorageProduceLValue(subscript, overload.getBaseType(), useDC))
         elementTy = LValueType::get(elementTy);
+      else {
+        Type selfType = overload.getBaseType()->getRValueType()
+            ->getMetatypeInstanceType()
+            ->lookThroughAllOptionalTypes();
+        if (elementTy->hasDynamicSelfType())
+          elementTy = elementTy->replaceCovariantResultType(selfType, 0);
+      }
 
       // See ConstraintSystem::resolveOverload() -- optional and dynamic
       // subscripts are a special case, because the optionality is

slavapestov · 2019-04-17T21:52:53Z

@johnno1962 Something else we can look at is making sure that 'self' has DynamicSelfType inside the getter of a property or subscript returning Self. This will allow you to write:

class C {
  var foo: Self {
    return self
  }
}

johnno1962 · 2019-04-18T13:42:54Z

HUGE THANKS @slavapestov, your fix works and I've updated the last commit to remove my "hack". At this point I have to had this over to someone who actually knows what they are doing. What started as a brainwave a month ago has rather ballooned into quite a time commitment. I've gone about Self detection in the wrong way to get things working but can someone else please pick up writing checkDynamicSelfResult() as you mention. Sorry about this.

johnno1962 · 2019-04-24T11:19:22Z

Tests are running at the moment so this is ready to merge if you have no other changes?

johnno1962 · 2019-04-29T11:24:03Z

Hi @slavapestov, do you have time to wrap this PR up? I've pushed a few additional tests where you can see the current error reporting. It's getting late in the day to merge with 5.1 so perhaps you could cherrypick the narrow fix #23853 banning aliases and we can look at this more risky change for 5.2. Can you run a source compatibility test for me please?

slavapestov · 2019-04-29T17:08:12Z

@johnno1962 Sure, I'll take this over. Thank you!

slavapestov · 2019-05-16T19:31:45Z

I haven't forgotten about this. I'm going to wrap it up soon and see what I can cherry pick to 5.1.

johnno1962 · 2019-05-16T20:03:36Z

Thanks @slavapestov. If you don’t have time to tidy it up #23853 is the more conservative option.

johnno1962 · 2019-06-03T13:46:13Z

lib/Sema/TypeCheckType.cpp

      Type SelfType = nominal->getSelfInterfaceType();
-      if (insideClass)
+      if (nominalDC->getSelfClassDecl() != nullptr)


Just a final idle thought @slavapestov. Should this line be:

if (nominalDC->getSelfClassDecl() && !nominalDC->getSelfClassDecl()->isFinal())

It could help with this problem: #22863 (comment)

slavapestov · 2019-06-20T19:17:18Z

Ok, I'm working on this now.

johnno1962 · 2019-06-20T20:37:54Z

Great news. There shouldn’t bee too much left to do except tidying things up. Should I close this PR? I'm trying to get extension ProtocolA: ProtocolB working at the moment. Are you available for the odd question?

johnno1962 · 2019-07-18T16:40:51Z

Superseded by #25742

johnno1962 mentioned this pull request Apr 12, 2019

Ban Self in typealias for now #23853

Merged

johnno1962 changed the title ~~Allow non-mutable properties and subscripts returning Self~~ [DNM] Allow non-mutable properties and subscripts returning Self Apr 13, 2019

slavapestov reviewed Apr 14, 2019

View reviewed changes

johnno1962 force-pushed the Self-fallback-settables branch 7 times, most recently from 900b090 to 213c068 Compare April 15, 2019 14:46

johnno1962 force-pushed the Self-fallback-settables branch from 213c068 to 87ac78e Compare April 15, 2019 15:16

johnno1962 changed the title ~~[DNM] Allow non-mutable properties and subscripts returning Self~~ Allow non-mutable properties and subscripts returning Self Apr 17, 2019

slavapestov reviewed Apr 17, 2019

View reviewed changes

johnno1962 force-pushed the Self-fallback-settables branch 2 times, most recently from 8749015 to f33ebb5 Compare April 18, 2019 11:32

johnno1962 force-pushed the Self-fallback-settables branch 2 times, most recently from 1cf6d4d to b8fc0bd Compare April 18, 2019 13:50

johnno1962 force-pushed the Self-fallback-settables branch from d9e9f4d to d45b6f6 Compare April 19, 2019 21:52

johnno1962 added 15 commits April 26, 2019 18:30

Story so far

f061124

Some progress

2479d10

Make properties and subscripts returning Self overridable.

e89e520

Seems to work

e4ddc11

Remove debug messages

2a15e65

Tidy-up

e3fc66c

Revert some tidy-ups.

ed758a9

CovariantReturnConversionExpr required.

075b82c

Resolve failing constraint for subscripts returning Self

f7606e8

Move validation of DynamicSelf to Decl checks.

10d15c0

Resolve conflict

f0503a3

Handle Optionals

3eff1dd

Update tests

1d4f8ba

Dynamic self in properties and subscripts

502f298

A couple of comments

1bec9df

johnno1962 force-pushed the Self-fallback-settables branch from 4c43dd1 to 1bec9df Compare April 26, 2019 17:31

More additional tests

8bf308b

johnno1962 commented Jun 3, 2019

View reviewed changes

slavapestov mentioned this pull request Jun 26, 2019

Clean up some old DynamicSelfType debt now that we have an implementation of SE-0068 #25742

Merged

johnno1962 closed this Jul 18, 2019

johnno1962 mentioned this pull request Apr 12, 2019

[SR-10326] 'Self' is only available in a protocol or as the result of a method in a class; did you mean 'A'? #52726

Closed

Allow non-mutable properties and subscripts returning Self #23991

Allow non-mutable properties and subscripts returning Self #23991

Conversation

johnno1962 commented Apr 12, 2019 • edited by AnthonyLatsis Loading

slavapestov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnno1962 commented Apr 14, 2019 • edited Loading

slavapestov commented Apr 14, 2019

slavapestov commented Apr 15, 2019

johnno1962 commented Apr 15, 2019 • edited Loading

johnno1962 commented Apr 15, 2019 • edited Loading

johnno1962 commented Apr 16, 2019

johnno1962 commented Apr 16, 2019 • edited Loading

johnno1962 commented Apr 17, 2019 • edited Loading

slavapestov commented Apr 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slavapestov commented Apr 17, 2019

slavapestov commented Apr 17, 2019

slavapestov commented Apr 17, 2019

johnno1962 commented Apr 18, 2019 • edited Loading

johnno1962 commented Apr 24, 2019

johnno1962 commented Apr 29, 2019 • edited Loading

slavapestov commented Apr 29, 2019

slavapestov commented May 16, 2019

johnno1962 commented May 16, 2019

johnno1962 Jun 3, 2019 • edited Loading

Choose a reason for hiding this comment

johnno1962 Jun 3, 2019 • edited Loading

Choose a reason for hiding this comment

slavapestov commented Jun 20, 2019

johnno1962 commented Jun 20, 2019 • edited Loading

johnno1962 commented Jul 18, 2019

johnno1962 commented Apr 12, 2019 •

edited by AnthonyLatsis

Loading

johnno1962 commented Apr 14, 2019 •

edited

Loading

johnno1962 commented Apr 15, 2019 •

edited

Loading

johnno1962 commented Apr 15, 2019 •

edited

Loading

johnno1962 commented Apr 16, 2019 •

edited

Loading

johnno1962 commented Apr 17, 2019 •

edited

Loading

johnno1962 commented Apr 18, 2019 •

edited

Loading

johnno1962 commented Apr 29, 2019 •

edited

Loading

johnno1962 Jun 3, 2019 •

edited

Loading

johnno1962 Jun 3, 2019 •

edited

Loading

johnno1962 commented Jun 20, 2019 •

edited

Loading