Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SR-695] Loophole in Self-requirement leads to crash #43310

Closed
swift-ci opened this issue Feb 9, 2016 · 6 comments
Closed

[SR-695] Loophole in Self-requirement leads to crash #43310

swift-ci opened this issue Feb 9, 2016 · 6 comments
Assignees
@gregomni
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself

Comments

@swift-ci
Copy link
Collaborator

swift-ci commented Feb 9, 2016

Previous ID SR-695
Radar None
Original Reporter DarkDust (JIRA User)
Type Bug
Status Resolved
Resolution Done
Environment

Swift version 2.2-dev (LLVM 3ebdbb2c7e, Clang f66c5bb67b, Swift 42591f7)
Apple Swift version 2.1.1 (swiftlang-700.1.101.15 clang-700.1.81)

Additional Detail from JIRA
Votes 1
Component/s Compiler
Labels Bug
Assignee @gregomni
Priority

md5: 1b9fffb15b9c311a34f50726c4f182f5

is duplicated by:

  • SR-988 Undiagnosed type error in subclass function with Self return type

Issue Description:

A StackOverflow question pointed out that there is a problem with Self-requirement in protocols that can lead to wrong results or crashes. Consider this example from the question:

// The protocol with Self requirement
protocol Narcissistic {
    func getFriend() -> Self
}

// Base class that adopts the protocol
class Mario : Narcissistic  {
    func getFriend() -> Self {
        print("Mario.getFriend()")
        return self;
    }
}

// Intermediate class that eliminates the
// Self requirement by specifying an explicit type
// (Why does the compiler allow this?)
class SuperMario : Mario {
    override func getFriend() -> SuperMario {
        print("SuperMario.getFriend()")
        return SuperMario();
    }
}

// Most specific class that defines a field whose
// (polymorphic) access will cause the world to explode
class FireFlowerMario : SuperMario {
    let fireballCount = 42

    func throwFireballs() {
        print("Throwing " + String(fireballCount) + " fireballs!")
    }
}

// Global generic function restricted to the protocol
func queryFriend<T : Narcissistic>(narcissistic: T) -> T {
    return narcissistic.getFriend()
}

// Sample client code

// Instantiate the most specific class
let m = FireFlowerMario()

// The call to the generic function is verified to return
// the same type that went in -- 'FireFlowerMario' in this case.
// But in reality, the method returns a 'SuperMario' and the
// call to 'throwFireballs' will cause arbitrary
// things to happen at runtime.
queryFriend(m).throwFireballs()

The problem here is that SuperMario.getFriend always returns a new instance of SuperMario which is correct at that point.

But when FireFlowerMario derived from SuperMario it inherited the implementation of getFriend that returns a SuperMario instance! The compiler treats the instance as a FireFlowerMario instead and tries to access the nonexistent fireballCount. This either leads to garbage output or crashes.

The compiler should be able to detect that SuperMario.getFriend does not behave as expected when subclassing and issue a warning about this.
@slavapestov
Copy link
Member

I think the problem is not even specific to protocols, we simply should not allow non-dynamic Self returns to override dynamic Self returns.

Should be an easy fix if someone wants to take a look ;-)

@lilyball
Copy link
Mannequin

lilyball mannequin commented Mar 1, 2016

I think it's reasonable to allow override func getFriend() -> SuperMario in the case where the class is marked final. The issue here isn't that the declaration specifies SuperMario where Self is expected, it's that the class can be subclassed and the method, when inherited by a subclass, will no longer work.

The benefit of allowing this is final classes can quite legitimately return a new instance of themselves instead of being forced to return self (or of having a required initializer that they use to create the new instance).

@gregomni
Copy link
Collaborator

@slavapestov Do you still feel the same way about overriding dynamic Self with non-dynamic (in a non-final class)? It is, indeed, an easy fix to give a diagnostic in that situation.

The reason why I ask is that it makes some previously legal code into a compiler error, and the importance of source compatibility has grown some since early 2016. Is this an ok thing to just PR, or is it now a language change worthy of (shudder) swift-evolution and a proposal?

@slavapestov
Copy link
Member

@gregomni we can make the more correct semantics conditional on -swift-version 5. Use ASTContext::isSwiftVersionAtLeast(5) to check. And yeah, we should not allow a non-final class to override a method with non-dynamic Self.

@gregomni
Copy link
Collaborator

#12495

@gregomni
Copy link
Collaborator

Done for swift 5

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@gregomni gregomni

Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@slavapestov @swift-ci @gregomni