From f7a7ee941a329856711f3c5e86fb309ee1111ffe Mon Sep 17 00:00:00 2001 From: Brian King Date: Sat, 17 Dec 2016 15:01:19 -0500 Subject: [PATCH 1/5] Warn if a non dynamic class declaration is overridden in an extension --- include/swift/AST/DiagnosticsSema.def | 3 +++ lib/Sema/TypeCheckDecl.cpp | 13 +++++++++++++ test/decl/inherit/override.swift | 14 ++++++++++---- 3 files changed, 26 insertions(+), 4 deletions(-) diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def index e16cc1f379b8d..a76b196d14639 100644 --- a/include/swift/AST/DiagnosticsSema.def +++ b/include/swift/AST/DiagnosticsSema.def @@ -1703,6 +1703,9 @@ 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.", + ()) ERROR(override_throws,none, "cannot override non-throwing %select{method|initializer}0 with " "throwing %select{method|initializer}0", (bool)) diff --git a/lib/Sema/TypeCheckDecl.cpp b/lib/Sema/TypeCheckDecl.cpp index 61c5585989916..9dc76d2957aa0 100644 --- a/lib/Sema/TypeCheckDecl.cpp +++ b/lib/Sema/TypeCheckDecl.cpp @@ -6072,6 +6072,19 @@ class DeclChecker : public DeclVisitor { 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(base->getDeclContext())) { + if (baseDecl->hasKnownSwiftImplementation() && + !base->isDynamic() && + override->getDeclContext()->isExtensionContext()) { + TC.diagnose(override, 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(override)) { diff --git a/test/decl/inherit/override.swift b/test/decl/inherit/override.swift index 17a8aec68d621..0509cc951ed6c 100644 --- a/test/decl/inherit/override.swift +++ b/test/decl/inherit/override.swift @@ -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 { @@ -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}} From 7dcab1255db4ab51f07504b1df89446ccc3b8723 Mon Sep 17 00:00:00 2001 From: Brian King Date: Sun, 18 Dec 2016 21:31:06 -0500 Subject: [PATCH 2/5] Update PR comments --- lib/Sema/TypeCheckDecl.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/Sema/TypeCheckDecl.cpp b/lib/Sema/TypeCheckDecl.cpp index 9dc76d2957aa0..dad6f5e9b15cc 100644 --- a/lib/Sema/TypeCheckDecl.cpp +++ b/lib/Sema/TypeCheckDecl.cpp @@ -6075,9 +6075,10 @@ class DeclChecker : public DeclVisitor { // 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(base->getDeclContext())) { + if (auto *baseDecl = dyn_cast(base->getDeclContext())) { if (baseDecl->hasKnownSwiftImplementation() && !base->isDynamic() && override->getDeclContext()->isExtensionContext()) { From 7d8d2757223c2c795d198ab5ce569c550880370b Mon Sep 17 00:00:00 2001 From: Brian King Date: Sat, 14 Jan 2017 00:27:48 -0500 Subject: [PATCH 3/5] Only generate a warning in swift 3 --- include/swift/AST/DiagnosticsSema.def | 3 +++ lib/Sema/TypeCheckDecl.cpp | 5 ++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def index a76b196d14639..85e9b3928b8d7 100644 --- a/include/swift/AST/DiagnosticsSema.def +++ b/include/swift/AST/DiagnosticsSema.def @@ -1706,6 +1706,9 @@ ERROR(override_ownership_mismatch,none, 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)) diff --git a/lib/Sema/TypeCheckDecl.cpp b/lib/Sema/TypeCheckDecl.cpp index dad6f5e9b15cc..213a8d1d54226 100644 --- a/lib/Sema/TypeCheckDecl.cpp +++ b/lib/Sema/TypeCheckDecl.cpp @@ -6082,7 +6082,10 @@ class DeclChecker : public DeclVisitor { if (baseDecl->hasKnownSwiftImplementation() && !base->isDynamic() && override->getDeclContext()->isExtensionContext()) { - TC.diagnose(override, diag::override_class_declaration_in_extension); + // 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); } } From 2e1264f245a9323483bef2c890699aabb3ea15d7 Mon Sep 17 00:00:00 2001 From: Brian King Date: Sat, 14 Jan 2017 00:28:32 -0500 Subject: [PATCH 4/5] Add a compatibily test for swift 3 --- test/Compatibility/override.swift | 11 +++++++++++ test/decl/inherit/override.swift | 2 +- 2 files changed, 12 insertions(+), 1 deletion(-) create mode 100644 test/Compatibility/override.swift diff --git a/test/Compatibility/override.swift b/test/Compatibility/override.swift new file mode 100644 index 0000000000000..ddfde0ef79f63 --- /dev/null +++ b/test/Compatibility/override.swift @@ -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}} +} diff --git a/test/decl/inherit/override.swift b/test/decl/inherit/override.swift index 0509cc951ed6c..8248fcf11d218 100644 --- a/test/decl/inherit/override.swift +++ b/test/decl/inherit/override.swift @@ -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 {} From 4495fde240c25d23afa8bfedd8f4846e3efaafdb Mon Sep 17 00:00:00 2001 From: Brian King Date: Tue, 17 Jan 2017 09:43:42 -0500 Subject: [PATCH 5/5] Remove periods from the warnings to be more consistent with other warnings --- include/swift/AST/DiagnosticsSema.def | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def index 85e9b3928b8d7..a502bed4c4111 100644 --- a/include/swift/AST/DiagnosticsSema.def +++ b/include/swift/AST/DiagnosticsSema.def @@ -1704,10 +1704,10 @@ ERROR(override_ownership_mismatch,none, "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.", + "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.", + "cannot override a non-dynamic class declaration from an extension", ()) ERROR(override_throws,none, "cannot override non-throwing %select{method|initializer}0 with "