Skip to content

[Diagnostics] Add specific warning for overriden NSObject.hash(into:) #35465

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
Jan 24, 2021
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
10 changes: 8 additions & 2 deletions include/swift/AST/DiagnosticsSema.def
Original file line number Diff line number Diff line change
Expand Up @@ -2454,8 +2454,10 @@ NOTE(overridden_near_match_here,none,
"potential overridden %0 %1 here",
(DescriptiveDeclKind, DeclName))
ERROR(override_decl_extension,none,
"overriding %select{|non-@objc }0declarations "
"%select{in extensions|from extensions}0 is not supported", (bool, bool))
"%select{|non-@objc}0 %2 %3 %select{"
"is declared in extension of %4 and cannot be overriden|"
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a typo: the correct spelling is "overridden."

"declared in %4 cannot be overriden from extension}1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Likewise here, same typo.

(bool, bool, DescriptiveDeclKind, DeclName, DeclName))
NOTE(overridden_here,none,
"overridden declaration is here", ())
NOTE(overridden_here_can_be_objc,none,
Expand Down Expand Up @@ -5351,6 +5353,10 @@ ERROR(override_nsobject_hashvalue_error,none,
"'NSObject.hashValue' is not overridable; "
"did you mean to override 'NSObject.hash'?", ())

ERROR(override_nsobject_hash_error,none,
"`NSObject.hash(into:)` is not overridable; "
Copy link
Collaborator

Choose a reason for hiding this comment

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

Backticks are not used for code voice in diagnostics; please use single quotes instead (see the other diagnostics).

"subclasses can customize hashing by overriding the `hash` property", ())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason that this message is worded differently from the one for overriding hashValue? Typically, diagnostic messages use a concise style, and I think the question posed above is appropriate here ("did you mean to override 'NSObject.hash'?").


WARNING(hashvalue_implementation,none,
"'Hashable.hashValue' is deprecated as a protocol requirement; "
"conform type %0 to 'Hashable' by implementing 'hash(into:)' instead",
Expand Down
32 changes: 26 additions & 6 deletions lib/Sema/TypeCheckDeclOverride.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -734,6 +734,20 @@ static bool isNSObjectHashValue(ValueDecl *baseDecl) {
return false;
}

/// Returns true if the given declaration is for the `NSObject.hash(into:)`
/// function.
static bool isNSObjectHashMethod(ValueDecl *baseDecl) {
auto baseFunc = dyn_cast<FuncDecl>(baseDecl);
if (!baseFunc)
return false;

if (auto classDecl = baseFunc->getDeclContext()->getSelfClassDecl()) {
ASTContext &ctx = baseDecl->getASTContext();
return baseFunc->getBaseName() == ctx.Id_hash && classDecl->isNSObject();
}
return false;
}

namespace {
/// Class that handles the checking of a particular declaration against
/// superclass entities that it could override.
Expand Down Expand Up @@ -995,11 +1009,13 @@ static void checkOverrideAccessControl(ValueDecl *baseDecl, ValueDecl *decl,
!baseHasOpenAccess &&
baseDecl->getModuleContext() != decl->getModuleContext() &&
!isa<ConstructorDecl>(decl)) {
// NSObject.hashValue was made non-overridable in Swift 5; one should
// override NSObject.hash instead.
// NSObject.hashValue and NSObject.hash(into:) is not overridable;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Grammar: it should read "are not overridable".

// one should override NSObject.hash instead.
if (isNSObjectHashValue(baseDecl)) {
diags.diagnose(decl, diag::override_nsobject_hashvalue_error)
.fixItReplace(SourceRange(decl->getNameLoc()), "hash");
} else if (isNSObjectHashMethod(baseDecl)) {
diags.diagnose(decl, diag::override_nsobject_hash_error);
} else {
diags.diagnose(decl, diag::override_of_non_open,
decl->getDescriptiveKind());
Expand Down Expand Up @@ -1791,16 +1807,20 @@ static bool checkSingleOverride(ValueDecl *override, ValueDecl *base) {
(isa<ExtensionDecl>(base->getDeclContext()) ||
isa<ExtensionDecl>(override->getDeclContext())) &&
!base->isObjC()) {
// Suppress this diagnostic for overrides of a non-open NSObject.hashValue
// property; these are diagnosed elsewhere. An error message complaining
// Suppress this diagnostic for overrides of a non-open NSObject.Hashable
Copy link
Collaborator

Choose a reason for hiding this comment

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

Grammar: should read "overrides of non-open NSObject Hashable interfaces" (i.e., delete "a" and remove the dot between "NSObject" and "Hashable".

// interfaces; these are diagnosed elsewhere. An error message complaining
// about extensions would be misleading in this case; the correct fix is to
// override NSObject.hash instead.
if (isNSObjectHashValue(base) &&
if ((isNSObjectHashValue(base) || isNSObjectHashMethod(base)) &&
!base->hasOpenAccess(override->getDeclContext()))
return true;

bool baseCanBeObjC = canBeRepresentedInObjC(base);
auto nominal = base->getDeclContext()->getSelfNominalTypeDecl();
diags.diagnose(override, diag::override_decl_extension, baseCanBeObjC,
!isa<ExtensionDecl>(base->getDeclContext()));
!isa<ExtensionDecl>(base->getDeclContext()),
override->getDescriptiveKind(), override->getName(),
nominal->getName());
// If the base and the override come from the same module, try to fix
// the base declaration. Otherwise we can wind up diagnosing into e.g. the
// SDK overlay modules.
Expand Down
2 changes: 1 addition & 1 deletion test/ClangImporter/objc-cross-module-override.swift
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
import ImageInitializers

final class MyImage : Image {
// CHECK: overriding non-@objc declarations from extensions is not supported
// CHECK: non-@objc initializer 'init(imageLiteralResourceName:)' is declared in extension of 'Image' and cannot be overriden
// Make sure we aren't emitting a fixit into the extant module...
// CHECK-NOT: add '@objc' to make this declaration overridable
// CHECK: ImageInitializers.Image:{{.*}}: note: overridden declaration is here
Expand Down
4 changes: 4 additions & 0 deletions test/ClangImporter/objc_override.swift
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,10 @@ class MyHashableNSObject: NSObject {
// expected-error@-1 {{'NSObject.hashValue' is not overridable; did you mean to override 'NSObject.hash'?}}
return 0
}

override func hash(into hasher: inout Hasher) {
// expected-error@-1 {{`NSObject.hash(into:)` is not overridable; subclasses can customize hashing by overriding the `hash` property}}
}
}

// rdar://problem/47557376
Expand Down
6 changes: 3 additions & 3 deletions test/attr/attr_objc_override.swift
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,11 @@ class B : A {
get { return self } // expected-error{{subscript getter with Objective-C selector 'objectForKeyedSubscript:' conflicts with subscript getter from superclass 'A'}}
}

override func foo() { } // expected-error{{overriding non-@objc declarations from extensions is not supported}}
override func foo() { } // expected-error{{non-@objc instance method 'foo()' is declared in extension of 'A' and cannot be overriden}}

override func wibble(_: SwiftStruct) { } // expected-error{{overriding declarations in extensions is not supported}}
override func wibble(_: SwiftStruct) { } // expected-error{{instance method 'wibble' is declared in extension of 'A' and cannot be overriden}}
}

extension B {
override func bar() { } // expected-error{{overriding non-@objc declarations from extensions is not supported}}
override func bar() { } // expected-error{{non-@objc instance method 'bar()' declared in 'A' cannot be overriden from extension}}
}
12 changes: 6 additions & 6 deletions test/decl/func/static_func.swift
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,8 @@ class C_Derived : C {
override class func f2() {}
class override func f3() {}

override class func ef2() {} // expected-error {{not supported}}
class override func ef3() {} // expected-error {{not supported}}
override class func ef2() {} // expected-error {{cannot be overriden}}
class override func ef3() {} // expected-error {{cannot be overriden}}
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 {{not supported}}
class override func f5() {} // expected-error {{not supported}}
override class func f4() {} // expected-error {{cannot be overriden}}
class override func f5() {} // expected-error {{cannot be overriden}}

override class func ef4() {} // expected-error {{not supported}}
class override func ef5() {} // expected-error {{not supported}}
override class func ef4() {} // expected-error {{cannot be overriden}}
class override func ef5() {} // expected-error {{cannot be overriden}}
}

protocol P { // expected-note{{extended type declared here}}
Expand Down
4 changes: 2 additions & 2 deletions test/decl/inherit/override.swift
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ extension B {
override func f3D() { }
override func f4D() -> ObjCClassB { }

func f5() { } // expected-error{{overridi}}
func f6() -> A { } // expected-error{{overriding declarations in extensions is not supported}}
func f5() { } // expected-error{{overri}}
func f6() -> A { } // expected-error{{instance method 'f6()' is declared in extension of 'A' and cannot be overriden}}

@objc override func f7() { }
@objc override func f8() -> ObjCClassA { }
Expand Down
2 changes: 1 addition & 1 deletion test/decl/init/basic_init.swift
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ class InitSubclass: InitClass {}
// expected-note@-1{{'init(baz:)' previously overridden here}}
// expected-note@-2{{'init(bar:)' previously overridden here}}
extension InitSubclass {
convenience init(arg: Bool) {} // expected-error{{overriding non-@objc declarations from extensions is not supported}}
convenience init(arg: Bool) {} // expected-error{{non-@objc initializer 'init(arg:)' declared in 'InitClass' cannot be overriden from extension}}
convenience override init(baz: Int) {}
// expected-error@-1 {{'init(baz:)' has already been overridden}}
// expected-error@-2 {{cannot override a non-dynamic class declaration from an extension}}
Expand Down
3 changes: 1 addition & 2 deletions test/stdlib/NSObject-hashing.swift
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ class Foo: NSObject {
}

override func hash(into hasher: inout Hasher) {
// expected-error@-1 {{overriding non-open instance method outside of its defining module}}
// expected-error@-2 {{overriding declarations in extensions is not supported}}
// expected-error@-1 {{`NSObject.hash(into:)` is not overridable; subclasses can customize hashing by overriding the `hash` property}}
}
}