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

mininny · 2021-01-17T14:51:54Z

import Foundation
class C: NSObject {
    override func hash(into hasher: inout Hasher) { }
}

The above code produces the following errors:

tmp.swift:3:19: error: overriding non-open instance method outside of its defining module
    override func hash(into hasher: inout Hasher) { }
                  ^
tmp.swift:3:19: error: overriding declarations in extensions is not supported
    override func hash(into hasher: inout Hasher) { }
                  ^
ObjectiveC.NSObject:4:17: note: overridden declaration is here
    public func hash(into hasher: inout Hasher)

which is possibly misleading as the code doesn't have any extensions. This case should be handled together with NSObject.hashValue as invalid overrided interfaces.

I added more detailed information about the error:
'NSObject.hash(into:)' is not overridable; subclasses can customize hashing by overriding the 'hash' property
which correctly guides the user to use NSObject.hash property instead.

I also modified existing override_decl_extension error to produce better diagnostics like this:
method 'foo()' is declared in extension of 'A' and cannot be overriden
and
method 'foo()' declared in 'A' cannot be overriden from extension

Resolves SR-13593.

test/stdlib/NSObject-hashing.swift

lib/Sema/TypeCheckDeclOverride.cpp

lorentey

Looks good to me. The original hashValue error was added to make it easier for people to update (usually broken) code that depended on it being overridable. There is no such source compatibility issue with NSObject.hash(into:) (it was never overridable), but the special diagnostic is still helpful.

lib/Sema/TypeCheckDeclOverride.cpp

lorentey · 2021-01-19T21:37:55Z

tmp.swift:3:19: error: overriding declarations in extensions is not supported
    override func hash(into hasher: inout Hasher) { }

If this is supposed to mean that it isn't possible to override a member that was originally defined in an extension, then it may be worth adjusting the diagnostic text to make that clearer. (Although this seems like an irrelevant/unnecessary diagnostic when we also have the error for overriding a non-open method.)

mininny · 2021-01-20T00:36:08Z

tmp.swift:3:19: error: overriding declarations in extensions is not supported
    override func hash(into hasher: inout Hasher) { }
If this is supposed to mean that it isn't possible to override a member that was originally defined in an extension, then it may be worth adjusting the diagnostic text to make that clearer. (Although this seems like an irrelevant/unnecessary diagnostic when we also have the error for overriding a non-open method.)

I agree that we would benefit from a better message for overriding declarations in extensions is not supported this warning. As suggested in the jira, I'll add another more generic diagnostic to handle these cases. 😄

CodaFi · 2021-01-21T17:14:18Z

include/swift/AST/DiagnosticsSema.def

@@ -2454,8 +2454,9 @@ 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 method %2 %select{"


We have DescriptiveDeclKind that can print out a more accurate descriptor than method when e.g. a constructor is the subject of an override.

Also, I don't believe the @objc-ness of a method is a salient detail here.

If the method is not @objc, there is an additional diagnosis that suggests the user make the method @objc so that it can be overridden in extensions. I think adding some detail to this diagnosis as well might help. What do you think?

CodaFi · 2021-01-23T20:18:51Z

Very nice! ⛵ .

@swift-ci smoke test and merge

LucianoPAlmeida · 2021-01-23T22:30:38Z

@swift-ci Please smoke test MacOS Platform

LucianoPAlmeida · 2021-01-24T00:53:29Z

@swift-ci Please smoke test MacOS Platform

xwu

Always nice to get a more specific warning! Some post-merge comments to consider for a follow-up PR.

xwu · 2021-01-30T17:01:53Z

include/swift/AST/DiagnosticsSema.def

-      "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|"


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

xwu · 2021-01-30T17:02:25Z

include/swift/AST/DiagnosticsSema.def

-      "%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", 


Likewise here, same typo.

xwu · 2021-01-30T17:03:21Z

include/swift/AST/DiagnosticsSema.def

@@ -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; "


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

xwu · 2021-01-30T17:05:07Z

include/swift/AST/DiagnosticsSema.def

@@ -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; "
+      "subclasses can customize hashing by overriding the `hash` property", ())


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'?").

xwu · 2021-01-30T17:05:57Z

lib/Sema/TypeCheckDeclOverride.cpp

@@ -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;


Grammar: it should read "are not overridable".

xwu · 2021-01-30T17:07:46Z

lib/Sema/TypeCheckDeclOverride.cpp

@@ -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


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

LucianoPAlmeida reviewed Jan 17, 2021

View reviewed changes

test/stdlib/NSObject-hashing.swift Show resolved Hide resolved

LucianoPAlmeida reviewed Jan 18, 2021

View reviewed changes

lib/Sema/TypeCheckDeclOverride.cpp Outdated Show resolved Hide resolved

mininny force-pushed the objc-override-hash-warning branch from a26fa4a to f8a7f65 Compare January 18, 2021 12:38

LucianoPAlmeida reviewed Jan 18, 2021

View reviewed changes

lib/Sema/TypeCheckDeclOverride.cpp Outdated Show resolved Hide resolved

varungandhi-apple requested a review from lorentey January 19, 2021 17:40

lorentey approved these changes Jan 19, 2021

View reviewed changes

lib/Sema/TypeCheckDeclOverride.cpp Outdated Show resolved Hide resolved

Add specific warning for overriden NSObject.hash(into:)

f138ce0

mininny force-pushed the objc-override-hash-warning branch 2 times, most recently from 7c59931 to 9ca7764 Compare January 21, 2021 14:31

CodaFi reviewed Jan 21, 2021

View reviewed changes

Modify override_decl_extension error to be more descriptive

5c21cdf

mininny force-pushed the objc-override-hash-warning branch from 9ca7764 to 5c21cdf Compare January 22, 2021 16:05

CodaFi merged commit c216370 into swiftlang:main Jan 24, 2021

xwu reviewed Jan 30, 2021

View reviewed changes

swift-ci mentioned this pull request Jan 17, 2021

[SR-13593] Error message talks about extensions, but I haven't written any extensions #56028

Closed

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

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

Uh oh!

Conversation

mininny commented Jan 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lorentey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lorentey commented Jan 19, 2021

Uh oh!

mininny commented Jan 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi commented Jan 23, 2021

Uh oh!

LucianoPAlmeida commented Jan 23, 2021

Uh oh!

LucianoPAlmeida commented Jan 24, 2021

Uh oh!

xwu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

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

mininny commented Jan 17, 2021 •

edited

Loading