[Sema] Disallow override of dynamic Self return with non-dynamic.

[gregomni]

... Unless the subclass where the override occurs is final. Because this change causes previously working code to fail to compile, this new diagnostic is only enabled in swift 5 mode.

(There are examples of this pattern in the existing tests, e.g. test/stdlib/TestMeasurement.swift baseUnit(), which continues to build without problems thus serving as tests for the "only swift 5 mode" portion of this change.)

Resolves SR-695.

[gregomni]

@swift-ci Please smoke test

[slavapestov]

I think this change is correct, however I think it's worth digging a bit deeper here.

If I override a method and the override has a completely unrelated return type from the base method, we flag that as an error already. So why is dynamic Self different here? Are we stripping off dynamic Self in the below logic when we go to match the return types? I think it would be better if we ensure that a dynamic Self mismatch is handled just like any other return type mismatch, and then add back the current broken behavior as a special case for when the language version is not 5.

[slavapestov]

I see, so adjustFunctionTypeForOverride() substitutes the DynamicSelfType with the concrete Self type, and this happens before we check for matching types. What if it didn't do that? We already allow overrides to return a subtype of the base method eg,

class Base {
  func f() -> Base
}

class Derived : Base {
  override func f() -> Derived
}

What if adjustFunctionTypeForOverride() left the DynamicSelfType in place, and we handled it as part of the subtype check?

Remember that Self is really more like DynamicSelfType<ThisClass>. So if I have

class Base {
  func f() -> Self
}

class Derived : Base {
  override func f() -> Self
}

You're going to end up checking if DynamicSelfType<Derived> is a subtype of DynamicSelfType<Base>, and for the purposes of this check, you can say yes, it is a subtype.

Similarly, Derived is not a subtype of DynamicSelfType<Base>, unless Derived is final.

Also this should work too, where the override returns Self but the base method does not:

class Base {
  func f() -> Base
}

class Derived : Base {
  override func f() -> Self 
}

You should add tests that cover all of these cases in both -swift-version 4 and -swift-version 5 modes.

[slavapestov]

You need to test the old behavior in test/Compatibility/self.swift or similar, too.

[gregomni]

Will do!

[slavapestov]

The semicolon is not needed here

[gregomni]

Yep! In my defense, it's copy-pasted from the original issue sample code.

[gregomni]

I think you mean adjustSuperclassMemberDeclType() instead of adjustFunctionTypeForOverride(). The former calls type->replaceCovariantResultType(), which substitutes out the dynamic self result type. Keeping the dynamic self result instead and just dealing with it during comparisons has a couple structural difficulties:

1. adjustSuperclassMemberDeclType() is also called from TypeConverter::getConstantOverrideInfo() which needs to lower things to SIL types. I haven't traced through that, but I assume it's likely it may actually need the non-dynamic result type instead. I guess we could make adjust...() take an extra bool param /* shouldReplaceDynamicType = */ true in that case.

2. I'm concerned that the major result of distinguishing -> Self from -> Subclass in the overload checking is that then the superclass could define a -> Self method, and the subclass could define an identically named -> Subclass method that was NOT an override (as it is currently forced to be, both before and after this change). It seems potentially dangerous and weird to everything further downstream of here to have two possible available members that are identical in all of name/arg types/(lowered) result type. But I guess it currently IS possible to do that all in one class, and it looks like the compiler just complains that every possible use is ambiguous. It seems like poor form to provide more ways of letting the user get themselves into that situation, though.

3. Since Derived should be a valid subtype of DynamicSelfType<Base> iff Derived is final, and (in your last example) DynamicSelfType<Derived> should be a valid subtype of Base, we're going to be stripping DynamicSelfType from our result types in many cases anyway. There's no straightforward identical structure for the comparison.

In short, I'm not seeing the win here. :)

[slavapestov]

Ok in that case, go ahead and keep your current approach. Please add a comment explaining that dynamic Self is stripped out, so must be checked explicitly, and add test cases for the various cases I talked about above.

[gregomni]

Did those things, thanks!

@swift-ci Please smoke test