Skip to content

Warn if a non dynamic class declaration is overridden in an extension #6346

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

Merged
merged 5 commits into from
Jan 17, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions include/swift/AST/DiagnosticsSema.def
Original file line number Diff line number Diff line change
Expand Up @@ -1703,6 +1703,12 @@ ERROR(override_ownership_mismatch,none,
"cannot override %select{strong|weak|unowned|unowned(unsafe)}0 property "
"with %select{strong|weak|unowned|unowned(unsafe)}1 property",
(/*Ownership*/unsigned, /*Ownership*/unsigned))
ERROR(override_class_declaration_in_extension,none,
"cannot override a non-dynamic class declaration from an extension",
())
WARNING(override_class_declaration_in_extension_warning,none,
"cannot override a non-dynamic class declaration from an extension",
())
ERROR(override_throws,none,
"cannot override non-throwing %select{method|initializer}0 with "
"throwing %select{method|initializer}0", (bool))
Expand Down
17 changes: 17 additions & 0 deletions lib/Sema/TypeCheckDecl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6072,6 +6072,23 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
new (TC.Context) OverrideAttr(SourceLoc()));
}

// If the overridden method is declared in a Swift Class Declaration,
// dispatch will use table dispatch. If the override is in an extension
// warn, since it is not added to the class vtable.
//
// FIXME: Only warn if the extension is in another module, and if
// it is in the same module, update the vtable.
if (auto *baseDecl = dyn_cast<ClassDecl>(base->getDeclContext())) {
if (baseDecl->hasKnownSwiftImplementation() &&
!base->isDynamic() &&
override->getDeclContext()->isExtensionContext()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make this conditional on !TC.Context.isSwiftVersion3(), or turn it into a warning?

To test both the Swift 3 and 4 behavior, have test/decl/inherit/override.swift pass -swift-version 4 in its RUN line, and add a new test/Compatibility/override.swift which tests the Swift 3 behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh thanks, I did not consider compatibility. I should be able to get a fix up soon.

// For compatibility, only generate a warning in Swift 3
TC.diagnose(override, (TC.Context.isSwiftVersion3()
? diag::override_class_declaration_in_extension_warning
: diag::override_class_declaration_in_extension));
TC.diagnose(base, diag::overridden_here);
}
}
// If the overriding declaration is 'throws' but the base is not,
// complain.
if (auto overrideFn = dyn_cast<AbstractFunctionDecl>(override)) {
Expand Down
11 changes: 11 additions & 0 deletions test/Compatibility/override.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// RUN: %target-typecheck-verify-swift -parse-as-library -swift-version 3

class A {
@objc func objcVirtualFunction() { } // expected-note{{overridden declaration is here}}
}

class B : A { }

extension B {
override func objcVirtualFunction() { } // expected-warning{{cannot override a non-dynamic class declaration from an extension}}
}
16 changes: 11 additions & 5 deletions test/decl/inherit/override.swift
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// RUN: %target-typecheck-verify-swift -parse-as-library
// RUN: %target-typecheck-verify-swift -parse-as-library -swift-version 4

@objc class ObjCClassA {}
@objc class ObjCClassB : ObjCClassA {}
Expand All @@ -7,8 +7,11 @@ class A {
func f1() { } // expected-note{{overridden declaration is here}}
func f2() -> A { } // expected-note{{overridden declaration is here}}

@objc func f3() { }
@objc func f4() -> ObjCClassA { }
@objc func f3() { } // expected-note{{overridden declaration is here}}
@objc func f4() -> ObjCClassA { } // expected-note{{overridden declaration is here}}

dynamic func f3D() { }
dynamic func f4D() -> ObjCClassA { }
}

extension A {
Expand All @@ -25,8 +28,11 @@ extension B {
func f1() { } // expected-error{{declarations in extensions cannot override yet}}
func f2() -> B { } // expected-error{{declarations in extensions cannot override yet}}

override func f3() { }
override func f4() -> ObjCClassB { }
override func f3() { } // expected-error{{cannot override a non-dynamic class declaration from an extension}}
override func f4() -> ObjCClassB { } // expected-error{{cannot override a non-dynamic class declaration from an extension}}

override func f3D() { }
override func f4D() -> ObjCClassB { }

func f5() { } // expected-error{{declarations in extensions cannot override yet}}
func f6() -> A { } // expected-error{{declarations in extensions cannot override yet}}
Expand Down