Skip to content

Suggest @objc for overrides of declarations from/in extensions. #13417

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 2 commits into from
Dec 14, 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
7 changes: 5 additions & 2 deletions include/swift/AST/DiagnosticsSema.def
Original file line number Diff line number Diff line change
Expand Up @@ -1795,10 +1795,13 @@ NOTE(overridden_near_match_here,none,
"potential overridden %0 %1 here",
(DescriptiveDeclKind, DeclName))
ERROR(override_decl_extension,none,
"overriding declarations %select{in extensions|from extensions}0 "
"is not supported", (bool))
"overriding %select{|non-@objc }0declarations "
"%select{in extensions|from extensions}0 is not supported", (bool, bool))
NOTE(overridden_here,none,
"overridden declaration is here", ())
NOTE(overridden_here_can_be_objc,none,
"add '@objc' to make this declaration overridable", ())

ERROR(override_objc_type_mismatch_method,none,
"overriding method with selector %0 has incompatible type %1",
(ObjCSelector, Type))
Expand Down
2 changes: 1 addition & 1 deletion include/swift/AST/RequirementEnvironment.h
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//===--- RequirementEnvironment.h - Swift Language Type ASTs ----*- C++ -*-===//
//===--- RequirementEnvironment.h - Requirement Environments ----*- C++ -*-===//
//
// This source file is part of the Swift.org open source project
//
Expand Down
2 changes: 1 addition & 1 deletion lib/AST/RequirementEnvironment.cpp
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//===--- RequirementEnvironment.cpp - ASTContext Implementation -----------===//
//===--- RequirementEnvironment.cpp - Requirement Environments ------------===//
//
// This source file is part of the Swift.org open source project
//
Expand Down
15 changes: 12 additions & 3 deletions lib/Sema/TypeCheckDecl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6689,9 +6689,18 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
if ((base->getDeclContext()->isExtensionContext() ||
override->getDeclContext()->isExtensionContext()) &&
!base->isObjC() && !isKnownObjC) {
TC.diagnose(override, diag::override_decl_extension,
!override->getDeclContext()->isExtensionContext());
TC.diagnose(base, diag::overridden_here);
bool baseCanBeObjC = TC.canBeRepresentedInObjC(base);
TC.diagnose(override, diag::override_decl_extension, baseCanBeObjC,
!base->getDeclContext()->isExtensionContext());
if (baseCanBeObjC) {
SourceLoc insertionLoc =
override->getAttributeInsertionLoc(/*forModifier=*/false);
TC.diagnose(base, diag::overridden_here_can_be_objc)
.fixItInsert(insertionLoc, "@objc ");
} else {
TC.diagnose(base, diag::overridden_here);
}

return true;
}

Expand Down
20 changes: 20 additions & 0 deletions lib/Sema/TypeCheckType.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3959,6 +3959,26 @@ bool TypeChecker::isRepresentableInObjC(const SubscriptDecl *SD,
return Result;
}

bool TypeChecker::canBeRepresentedInObjC(const ValueDecl *decl) {
if (!Context.LangOpts.EnableObjCInterop)
return false;

if (auto func = dyn_cast<AbstractFunctionDecl>(decl)) {
Optional<ForeignErrorConvention> errorConvention;
return isRepresentableInObjC(func, ObjCReason::MemberOfObjCMembersClass,
errorConvention);
}

if (auto var = dyn_cast<VarDecl>(decl))
return isRepresentableInObjC(var, ObjCReason::MemberOfObjCMembersClass);

if (auto subscript = dyn_cast<SubscriptDecl>(decl))
return isRepresentableInObjC(subscript,
ObjCReason::MemberOfObjCMembersClass);

return false;
}

void TypeChecker::diagnoseTypeNotRepresentableInObjC(const DeclContext *DC,
Type T,
SourceRange TypeRange) {
Expand Down
2 changes: 2 additions & 0 deletions lib/Sema/TypeChecker.h
Original file line number Diff line number Diff line change
Expand Up @@ -2279,6 +2279,8 @@ class TypeChecker final : public LazyResolver {
bool isRepresentableInObjC(const VarDecl *VD, ObjCReason Reason);
bool isRepresentableInObjC(const SubscriptDecl *SD, ObjCReason Reason);

bool canBeRepresentedInObjC(const ValueDecl *decl);

void diagnoseTypeNotRepresentableInObjC(const DeclContext *DC,
Type T, SourceRange TypeRange);

Expand Down
4 changes: 2 additions & 2 deletions test/Compatibility/attr_override.swift
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ class A {
set { }
}

func overriddenInExtension() {} // expected-note {{overridden declaration is here}}
func overriddenInExtension() {} // expected-note {{overri}}
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a typo?

}

class B : A {
Expand Down Expand Up @@ -140,7 +140,7 @@ class B : A {
}

extension B {
override func overriddenInExtension() {} // expected-error{{overriding declarations in extensions is not supported}}
override func overriddenInExtension() {} // expected-error{{overri}}
}

struct S {
Expand Down
18 changes: 17 additions & 1 deletion test/attr/attr_objc_override.swift
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// RUN: %target-typecheck-verify-swift
// RUN: %target-typecheck-verify-swift -swift-version 4
//
// REQUIRES: objc_interop

Expand All @@ -8,16 +8,32 @@ class MixedKeywordsAndAttributes { // expected-note{{in declaration of 'MixedKey
}

@objc class ObjCClass {}
struct SwiftStruct { }

class A {
@objc subscript (a: ObjCClass) -> ObjCClass { // expected-note{{overridden declaration here has type '(ObjCClass) -> ObjCClass'}}
get { return ObjCClass() }
}

func bar() { } // expected-note{{add '@objc' to make this declaration overridable}}{{3-3=@objc }}
}

extension A {
func foo() { } // expected-note{{add '@objc' to make this declaration overridable}}{{3-3=@objc }}
func wibble(_: SwiftStruct) { } // expected-note{{overridden declaration is here}}
}

class B : A {
// Objective-C
@objc subscript (a: ObjCClass) -> AnyObject { // expected-error{{overriding keyed subscript with incompatible type '(ObjCClass) -> AnyObject'}}
get { return self }
}

override func foo() { } // expected-error{{overriding non-@objc declarations from extensions is not supported}}

override func wibble(_: SwiftStruct) { } // expected-error{{overriding declarations in extensions is not supported}}
}

extension B {
override func bar() { } // expected-error{{overriding non-@objc declarations from extensions is not supported}}
}
4 changes: 2 additions & 2 deletions test/attr/attr_override.swift
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ class A {
set { }
}

func overriddenInExtension() {} // expected-note {{overridden declaration is here}}
func overriddenInExtension() {} // expected-note {{overr}}
}

class B : A {
Expand Down Expand Up @@ -140,7 +140,7 @@ class B : A {
}

extension B {
override func overriddenInExtension() {} // expected-error{{overriding declarations in extensions is not supported}}
override func overriddenInExtension() {} // expected-error{{overr}}
}

struct S {
Expand Down
4 changes: 2 additions & 2 deletions test/decl/class/classes.swift
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ class B : A {
func g() -> (B, B) { return (B(), B()) } // expected-error {{declaration 'g()' cannot override more than one superclass declaration}}
override func h() -> (A, B) { return (B(), B()) } // expected-note {{'h()' previously overridden here}}
override func h() -> (B, A) { return (B(), B()) } // expected-error {{'h()' has already been overridden}}
func i() {} // expected-error {{overriding declarations from extensions is not supported}}
func i() {} // expected-error {{overri}}
override func j() -> Int { return 0 }
func j() -> Float { return 0.0 }
func k() -> Float { return 0.0 }
Expand Down Expand Up @@ -39,7 +39,7 @@ class A {
}
}
extension A {
func i() {} // expected-note{{overridden declaration is here}}
func i() {} // expected-note{{overri}}
}
func f() {
let x = B()
Expand Down
28 changes: 14 additions & 14 deletions test/decl/func/static_func.swift
Original file line number Diff line number Diff line change
Expand Up @@ -61,30 +61,30 @@ extension E {
}

class C {
static func f1() {} // expected-note 3{{overridden declaration is here}}
static func f1() {} // expected-note 3{{overri}}
class func f2() {}
class func f3() {}
class func f4() {} // expected-note {{overridden declaration is here}}
class func f5() {} // expected-note {{overridden declaration is here}}
class func f4() {} // expected-note {{overri}}
class func f5() {} // expected-note {{overri}}
static final func f6() {} // expected-error {{static declarations are already final}} {{10-16=}}
final class func f7() {} // expected-note 3{{overridden declaration is here}}
final class func f7() {} // expected-note 3{{overri}}
}

extension C {
static func ef1() {}
class func ef2() {} // expected-note {{overridden declaration is here}}
class func ef3() {} // expected-note {{overridden declaration is here}}
class func ef4() {} // expected-note {{overridden declaration is here}}
class func ef5() {} // expected-note {{overridden declaration is here}}
class func ef2() {} // expected-note {{overri}}
class func ef3() {} // expected-note {{overri}}
class func ef4() {} // expected-note {{overri}}
class func ef5() {} // expected-note {{overri}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Or is this a magic string somehow?

Copy link
Contributor

Choose a reason for hiding this comment

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

If the rest of the diagnostic text is ancillary, you can match on a known prefix with the verifier.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a string that's common to both the "we have Objective-C interop" and "we don't have Objective-C interop" cases. If I tried to match the whole thing, the tests would fail on either Linux or Darwin, because the produce different diagnostics depending on whether that particular declaration could be made @objc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ha, smart :)

}

class C_Derived : C {
override static func f1() {} // expected-error {{cannot override static method}}
override class func f2() {}
class override func f3() {}

override class func ef2() {} // expected-error {{overriding declarations from extensions is not supported}}
class override func ef3() {} // expected-error {{overriding declarations from extensions is not supported}}
override class func ef2() {} // expected-error {{not supported}}
class override func ef3() {} // expected-error {{not supported}}
override static func f7() {} // expected-error {{static method overrides a 'final' class method}}
}

Expand All @@ -98,11 +98,11 @@ class C_Derived3 : C {
}

extension C_Derived {
override class func f4() {} // expected-error {{overriding declarations in extensions is not supported}}
class override func f5() {} // expected-error {{overriding declarations in extensions is not supported}}
override class func f4() {} // expected-error {{not supported}}
class override func f5() {} // expected-error {{not supported}}

override class func ef4() {} // expected-error {{overriding declarations in extensions is not supported}}
class override func ef5() {} // expected-error {{overriding declarations in extensions is not supported}}
override class func ef4() {} // expected-error {{not supported}}
class override func ef5() {} // expected-error {{not supported}}
}

protocol P {
Expand Down
24 changes: 12 additions & 12 deletions test/decl/inherit/override.swift
Original file line number Diff line number Diff line change
Expand Up @@ -4,22 +4,22 @@
@objc class ObjCClassB : ObjCClassA {}

class A {
func f1() { } // expected-note{{overridden declaration is here}}
func f2() -> A { } // expected-note{{overridden declaration is here}}
func f1() { } // expected-note{{overri}}
func f2() -> A { } // expected-note{{overri}}

@objc func f3() { } // expected-note{{overridden declaration is here}}
@objc func f4() -> ObjCClassA { } // expected-note{{overridden declaration is here}}
@objc var v1: Int { return 0 } // expected-note{{overridden declaration is here}}
@objc var v2: Int { return 0 } // expected-note{{overridden declaration is here}}
@objc var v3: Int = 0 // expected-note{{overridden declaration is here}}
@objc func f3() { } // expected-note{{overri}}
@objc func f4() -> ObjCClassA { } // expected-note{{overri}}
@objc var v1: Int { return 0 } // expected-note{{overri}}
@objc var v2: Int { return 0 } // expected-note{{overri}}
@objc var v3: Int = 0 // expected-note{{overri}}

dynamic func f3D() { } // expected-error{{'dynamic' instance method 'f3D()' must also be '@objc'}}{{3-3=@objc }}
dynamic func f4D() -> ObjCClassA { } // expected-error{{'dynamic' instance method 'f4D()' must also be '@objc'}}{{3-3=@objc }}
}

extension A {
func f5() { } // expected-note{{overridden declaration is here}}
func f6() -> A { } // expected-note{{overridden declaration is here}}
func f5() { } // expected-note{{overri}}
func f6() -> A { } // expected-note{{overri}}

@objc func f7() { }
@objc func f8() -> ObjCClassA { }
Expand All @@ -28,8 +28,8 @@ extension A {
class B : A { }

extension B {
func f1() { } // expected-error{{overriding declarations in extensions is not supported}}
func f2() -> B { } // expected-error{{overriding declarations in extensions is not supported}}
func f1() { } // expected-error{{overri}}
func f2() -> B { } // expected-error{{overri}}

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}}
Expand All @@ -46,7 +46,7 @@ extension B {
override func f3D() { }
override func f4D() -> ObjCClassB { }

func f5() { } // expected-error{{overriding declarations in extensions is not supported}}
func f5() { } // expected-error{{overridi}}
func f6() -> A { } // expected-error{{overriding declarations in extensions is not supported}}

@objc override func f7() { }
Expand Down
4 changes: 2 additions & 2 deletions test/decl/var/properties.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1121,7 +1121,7 @@ class OwnershipBadSub : OwnershipBase {

// <rdar://problem/17391625> Swift Compiler Crashes when Declaring a Variable and didSet in an Extension
class rdar17391625 {
var prop = 42 // expected-note {{overridden declaration is here}}
var prop = 42 // expected-note {{overri}}
}

extension rdar17391625 {
Expand All @@ -1137,7 +1137,7 @@ class rdar17391625derived : rdar17391625 {

extension rdar17391625derived {
// Not a stored property, computed because it is an override.
override var prop: Int { // expected-error {{overriding declarations in extensions is not supported}}
override var prop: Int { // expected-error {{overri}}
didSet {
}
}
Expand Down