-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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|" | ||
"declared in %4 cannot be overriden from extension}1", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
@@ -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; " | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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", ()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
WARNING(hashvalue_implementation,none, | ||
"'Hashable.hashValue' is deprecated as a protocol requirement; " | ||
"conform type %0 to 'Hashable' by implementing 'hash(into:)' instead", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()); | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
There was a problem hiding this comment.
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."