[SR-11905] Objective-C interop allows creation of undefined behavior

[DevAndArtist]

Previous ID	SR-11905
Radar	rdar://problem/57711780
Original Reporter	@DevAndArtist
Type	Bug

Environment

Apple Swift version 5.1.2 (swiftlang-1100.0.278 clang-1100.0.33.9)

Additional Detail from JIRA

Votes	1
Component/s	Compiler
Labels	Bug, StarterBug
Assignee	faical (JIRA)
Priority	Medium

md5: df1a976b8eb12c28ba3696248c7605f6

Issue Description:

In pure Swift this wouldn't be possible because we shouldn't be able to override a property of a class we import and don't own.

extension UITabBarController {
  open override var shouldAutorotate: Bool {
    return true
  }
}

However due to Objective-C interop the swift compiler does not complain about this extension and let us create an undefined behavior at runtime.

If the name of a method declared in a category is the same as a method in the original class, or a method in another category on the same class (or even a superclass), the behavior is undefined as to which method implementation is used at runtime. This is less likely to be an issue if you’re using categories with your own classes, but can cause problems when using categories to add methods to standard Cocoa or Cocoa Touch classes.

Source: https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/CustomizingExistingClasses/CustomizingExistingClasses.html

[beccadax]

@swift-ci create

[CodaFi]

Given source compatibility constraints, the best we can do is warn. This is sorta a StarterBug for the Decl Checker. Ping me here for more details.

[swift-ci]

Comment by Faiçal (JIRA)

Hi @CodaFi! Happy to take this on. Would be grateful for any pointer you could give me to get started 🙂.

[CodaFi]

faical (JIRA User) There's a big condition soup in OverrideMatcher::checkOverride that needs to be updated. Detecting whether a decl has been ClangImport'ed into Swift is simple (just ask Decl::hasClangNode). So the logic is roughly: If the type matches, and the decl context for the declaration is an extension, and the extended decl context is ClangImport'ed, then warn. You'll need to do this for subscripts, variables, and functions - you can probably catch-all by just doing it for ValueDecls and using the descriptive decl kind to render a nice diagnostic.

Finally, you'll need some kind of syntax to silence this warning. There's precedent in allowing bogus overrides as long as the user spells them with parens around the type, so we can probably just suggest that in a note carrying a fixit. That is, this ought to warn:

extension UITabBarController {
  open override var shouldAutorotate: Bool { // warning: extension declares override of Objective-C property 'shouldAutorotate'; this will cause unstable runtime behavior
                                                                           // note: add parentheses to silence this warning
    return true
  }
}

This shouldn't

extension UITabBarController {
  open override var shouldAutorotate: (Bool) {
    return true
  }
}

[CodaFi]

@DevAndArtist If you'll forgive a little language-lawyering, technically this behavior is not undefined - the documentation is a little extreme here. Load-time ordering is consistent across runs of an application, and the runtime guarantees that somebody's category or class is going to win. The unstable bit is who that actually is at runtime.