-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[SR-11588]Warn about derived Hashable implementation if there’s a custom Equatable #27801
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
base: main
Are you sure you want to change the base?
Conversation
WARNING(hashable_customEquatable, none, | ||
"Automatically generated 'Hashable' implementation for type %0 " | ||
"may not match the behavior of custom '==' operator", | ||
(Type)) |
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.
- Can you give the diagnostic identifier a more descriptive name? Also, please use lowercase words only. For example:
synthesized_hashable_may_not_match_custom_equatable
. - Diagnostic text must start with a lowercase letter.
- Can you move the argument (aka
(Type)
) to the line above since it can fit there?
"may not match the behavior of custom '==' operator", | ||
(Type)) | ||
NOTE(add_custom_hashable, none, | ||
"Add a custom 'hash(into:)' method", ()) |
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.
Same here as well - this should start with a lowercase letter.
ConformanceDecl->diagnose(diag::type_does_not_conform, | ||
Nominal->getDeclaredType(), | ||
hashableProto->getDeclaredType()); | ||
auto *DC = ConformanceDecl->getDeclContext(); |
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.
Nit: Maybe conformanceDC
?
Nominal->getDeclaredType(), | ||
hashableProto->getDeclaredType()); | ||
auto *DC = ConformanceDecl->getDeclContext(); | ||
auto type = Nominal->getDeclaredType(); |
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.
Nit: nominalTy
/nominalType
?
Please run
Can you share the stack trace? |
cc @lorentey |
I’ve run it on the file but it has changed a lot of formatting in the file apart the block I’ve added. It’s okay to add all the changes or is better to revert all the other changes and keep only my formatted block? |
Here it is. 1. Swift version 5.1.1-dev (LLVM f4c2b4eca6, Swift 32c1b05168)
2. While type-checking protocol conformance to 'Hashable' (in module 'Swift') for type 'TestRawType' (declared at [/swift-source/swift/test/Sema/implementation-only-import-in-decls.swift:65:8 - line:68:1] RangeText="enum TestRawType: IntLike { // expected-error {{cannot use struct 'IntLike' here; 'BADLibrary' has been imported as implementation-only}}
case x = 1
// FIXME: expected-error@-1 {{cannot use conformance of 'IntLike' to 'Equatable' here; 'BADLibrary' has been imported as implementation-only}}
")
0 swiftc 0x000000010d1be0c5 llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 37
1 swiftc 0x000000010d1bd355 llvm::sys::RunSignalHandlers() + 85
2 swiftc 0x000000010d1be6b8 SignalHandler(int) + 264
3 libsystem_platform.dylib 0x00007fff57d2bb5d _sigtramp + 29
4 libsystem_platform.dylib 0x00007ffee841d608 _sigtramp + 2423200456
5 libsystem_c.dylib 0x00007fff57be56a6 abort + 127
6 libsystem_c.dylib 0x00007fff57bae20d basename_r + 0
7 swiftc 0x0000000109fb1b77 swift::RootProtocolConformance::getWitnessDeclRef(swift::ValueDecl*) const + 167
8 swiftc 0x0000000109fb03dd swift::ProtocolConformance::getWitnessDeclRef(swift::ValueDecl*) const + 93
9 swiftc 0x0000000109fb0366 swift::ProtocolConformanceRef::getWitnessByName(swift::Type, swift::DeclName) const + 198
10 swiftc 0x0000000109117245 swift::DerivedConformance::deriveHashable(swift::ValueDecl*) + 1093
11 swiftc 0x00000001093205bb swift::TypeChecker::deriveProtocolRequirement(swift::DeclContext*, swift::NominalTypeDecl*, swift::ValueDecl*) + 315
12 swiftc 0x0000000109320323 swift::ConformanceChecker::resolveWitnessViaDerivation(swift::ValueDecl*) + 275
13 swiftc 0x0000000109320f11 swift::ConformanceChecker::resolveWitnessTryingAllStrategies(swift::ValueDecl*) + 289
14 swiftc 0x00000001093221e1 swift::ConformanceChecker::resolveValueWitnesses() + 401
15 swiftc 0x000000010931a8a9 swift::ConformanceChecker::checkConformance(swift::MissingWitnessDiagnosisKind) + 553
16 swiftc 0x00000001093185dc swift::MultiConformanceChecker::checkIndividualConformance(swift::NormalProtocolConformance*, bool) + 3292
17 swiftc 0x0000000109317620 swift::MultiConformanceChecker::checkAllConformances() + 144
18 swiftc 0x00000001093242fb swift::TypeChecker::checkConformancesInContext(swift::DeclContext*, swift::IterableDeclContext*) + 1035
19 swiftc 0x0000000109479cc6 typeCheckFunctionsAndExternalDecls(swift::SourceFile&, swift::TypeChecker&) + 470
20 swiftc 0x000000010947a459 swift::performTypeChecking(swift::SourceFile&, swift::TopLevelContext&, swift::OptionSet<swift::TypeCheckingFlags, unsigned int>, unsigned int, unsigned int, unsigned int, unsigned int, unsigned int) + 1017
21 swiftc 0x0000000107e68735 swift::CompilerInstance::parseAndTypeCheckMainFileUpTo(swift::SourceFile::ASTStage_t, swift::OptionSet<swift::TypeCheckingFlags, unsigned int>) + 965
22 swiftc 0x0000000107e66bd0 swift::CompilerInstance::parseAndCheckTypesUpTo(swift::CompilerInstance::ImplicitImports const&, swift::SourceFile::ASTStage_t) + 384
23 swiftc 0x0000000107e66007 swift::CompilerInstance::performSemaUpTo(swift::SourceFile::ASTStage_t) + 807
24 swiftc 0x0000000107e6606a swift::CompilerInstance::performSema() + 26
25 swiftc 0x00000001078f3509 performCompile(swift::CompilerInstance&, swift::CompilerInvocation&, llvm::ArrayRef<char const*>, int&, swift::FrontendObserver*, swift::UnifiedStatsReporter*) + 457
26 swiftc 0x00000001078f2589 swift::performFrontend(llvm::ArrayRef<char const*>, char const*, void*, swift::FrontendObserver*) + 2121
27 swiftc 0x00000001077dfee4 run_driver(llvm::StringRef, llvm::ArrayRef<char const*>) + 292
28 swiftc 0x00000001077df1f3 main + 1875
29 libdyld.dylib 0x00007fff57b403d5 start + 1
30 libdyld.dylib 0x0000000000000013 start + 2823552063 |
equalsName); | ||
|
||
auto equalsDeclaration = witness.getDecl(); | ||
if (!equalsDeclaration->isImplicit()) { |
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.
if (!equalsDeclaration->isImplicit()) { | |
if (equalsDeclaration && !equalsDeclaration->isImplicit()) { |
just in case we don't have a decl. Also, check if it fixes that crash
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.
I added the check but the crash remains, but I’ve noted that I’ve missed a line in the stack trace Assertion failed: (!witnessDC->isInnermostContextGeneric()), function getWitnessDeclRef, file /swift-source/swift/lib/AST/ProtocolConformance.cpp, line 947. Stack dump:
Seems that I’m using something wrong in getting the witness of the ==
declaration. But I don’t get what is wrong.
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.
Can you run the compiler with the debugger attached (copy the giant command-line dump you saw in the backtrace, then put lldb
in front of it and --
after the executable name, then execute it and use the command run
) and then, once it crashes, use the command e witnessDC->dumpContext()
to see what the witnessDC is? That might give us a better idea of what's going wrong.
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.
No my bad, discard this message 😳
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.
In that point it seems that witnessDC is:
(lldb) e witnessDC->dumpContext()
0x118815ca0 Module name=Swift
0x118815d60 FileUnit file="/swift-source/build/Ninja-RelWithDebInfoAssert+swift-DebugAssert/swift-macosx-x86_64/lib/swift/macosx/Swift.swiftmodule/x86_64.swiftmodule"
0x118044b40 AbstractFunctionDecl name===(_:_:) : <T where T : RawRepresentable, T.RawValue : Equatable> (T, T) -> Bool
I've tried to hunt up the stack to gather some other context, these are the variables inside my block in DerivedConformanceEquatableHashable.cpp
e eqConformance->dump()
(normal_conformance type=TestRawType protocol=Equatable
(value req===(_:_:) witness=Swift.(file).==))
e nominalTy->dump()
(enum_type decl=main.(file).TestRawType@/swift-source/swift/test/Sema/implementation-only-import-in-decls.swift:65:13)
After stepping in in the getWitnessByName function I found these:
e type.dump()
(enum_type decl=main.(file).TestRawType@/swift-source/swift/test/Sema/implementation-only-import-in-decls.swift:65:13)
e name.getBaseName()
(swift::DeclBaseName) $0 = {
Ident = (Pointer = "==")
}
(lldb) e getConcrete()->dump()
(normal_conformance type=TestRawType protocol=Equatable
(value req===(_:_:) witness=Swift.(file).==))
e witness
(swift::Witness) $1 = {
storage = {
llvm::pointer_union_detail::PointerUnionMembers<llvm::PointerUnion<swift::ValueDecl *, swift::Witness::StoredWitness *>, llvm::PointerIntPair<void *, 1, int, llvm::pointer_union_detail::PointerUnionUIntTraits<swift::ValueDecl *, swift::Witness::StoredWitness *>, llvm::PointerIntPairInfo<void *, 1, llvm::pointer_union_detail::PointerUnionUIntTraits<swift::ValueDecl *, swift::Witness::StoredWitness *> > >, 0, swift::ValueDecl *, swift::Witness::StoredWitness *> = {
llvm::pointer_union_detail::PointerUnionMembers<llvm::PointerUnion<swift::ValueDecl *, swift::Witness::StoredWitness *>, llvm::PointerIntPair<void *, 1, int, llvm::pointer_union_detail::PointerUnionUIntTraits<swift::ValueDecl *, swift::Witness::StoredWitness *>, llvm::PointerIntPairInfo<void *, 1, llvm::pointer_union_detail::PointerUnionUIntTraits<swift::ValueDecl *, swift::Witness::StoredWitness *> > >, 1, swift::Witness::StoredWitness *> = {
llvm::pointer_union_detail::PointerUnionMembers<llvm::PointerUnion<swift::ValueDecl *, swift::Witness::StoredWitness *>, llvm::PointerIntPair<void *, 1, int, llvm::pointer_union_detail::PointerUnionUIntTraits<swift::ValueDecl *, swift::Witness::StoredWitness *>, llvm::PointerIntPairInfo<void *, 1, llvm::pointer_union_detail::PointerUnionUIntTraits<swift::ValueDecl *, swift::Witness::StoredWitness *> > >, 2> = {
Val = (Value = 4700503004)
}
}
}
}
}
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.
I’ve recreated the problem, it seems that if the Equatable
conformance is declared in a different module, and the type extend that, the eqConformance
is not valid to generate a witness.
Anyone knows how can I catch that? Using lldb I haven’t find much differences between the tests that I can use to disambiguate them and the methods of eqConformance
don’t seems to have that particular catch available, or at least not to my eyes.
I think that the correct flow is that in the if that checks that the conformance exists must check even if the conformance is inside my module before finding the witness (even because is nonsensical to add a warning in another module right?).
686e192
to
cc6e113
Compare
8f73bfb
to
dffb024
Compare
@DougGregor sorry to ping, but I've read in the CODE_OWNERS file that you are associated with the I‘ve tried to look everywhere but I’m not seeing anything useful for a check against it :( |
6c76ba5
to
afa184f
Compare
@swift-ci please smoke test |
|
||
enum ImportedConformance: ImplicitEquatable { | ||
case x = 1 | ||
} |
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.
If you don't already have something like this, I'd suggest adding test cases like these (plus cross-file and perhaps cross-module variants):
protocol DefaultedImplementationProtocol: Equatable {}
extension DefaultedImplementationProtocol {
static func == (lhs: Self, rhs: Self) -> Bool { true }
}
struct DefaultedImplementation: DefaultedImplementationProtocol, Hashable {
let a: Int
}
protocol ConditionalImplementationProtocol {}
extension ConditionalImplementationProtocol where Self: Equatable {
static func == (lhs: Self, rhs: Self) -> Bool { true }
}
struct ConditionalImplementation: ConditionalImplementationProtocol, Hashable {
let a: Int
}
protocol ConditionalHashableImplementationProtocol {}
extension ConditionalHashableImplementationProtocol where Self: Equatable {
static func == (lhs: Self, rhs: Self) -> Bool { true }
}
extension ConditionalHashableImplementationProtocol where Self: Hashable {
func hash(into hasher: Hasher) {}
}
struct ConditionalHashableImplementation: ConditionalHashableImplementationProtocol, Hashable {
let a: Int
}
I haven't included expected-warning in these test cases because I haven't thought much about whether we should have warnings in them (and whether there might be cases, like "all of the properties are requirements of the protocol, so I assume Equatable checks them", where we shouldn't warn); you should look at the change's behavior in these cases and similar ones to make sure you're happy with it.
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.
(Also, typo: I think the "z" is in the wrong place in this filename.)
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.
Thanks for the catch on the typo in the filename! I've added your example cases for not forgetting them. I will iterate over them and add the correct variant for out of file and cross module. But I'm always stuck on the crasher that I'm getting for asking the witness declaration of the equals function when is cross module for an enum. I don't know what check I must execute to avoid the !witnessDC->isInnermostContextGeneric()
assertion in the AST/ProtocolConformance.cpp
file.
@beccadax hi, sorry to ping you, I've finally solved the issue and on my mac all the tests are running fine. There is more changes I have to do? |
@swift-ci please test |
Build failed |
Build failed |
I've rebased the repo and run all the tests with |
@swift-ci please test |
@@ -5675,6 +5675,11 @@ ERROR(override_nsobject_hash_error,none, | |||
"'NSObject.hash(into:)' is not overridable; " | |||
"did you mean to override 'NSObject.hash'?", ()) | |||
|
|||
WARNING(synthesized_hashable_may_not_match_custom_equatable, none, | |||
"automatically generated 'Hashable' implementation for type %0 " |
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.
"automatically generated 'Hashable' implementation for type %0 " | |
"synthesized 'Hashable' implementation for type %0 " |
Is there a reason to avoid the commonly used word 'synthesized' here?
@@ -5675,6 +5675,11 @@ ERROR(override_nsobject_hash_error,none, | |||
"'NSObject.hash(into:)' is not overridable; " | |||
"did you mean to override 'NSObject.hash'?", ()) | |||
|
|||
WARNING(synthesized_hashable_may_not_match_custom_equatable, none, | |||
"automatically generated 'Hashable' implementation for type %0 " | |||
"may not match the behavior of custom '==' operator", (Type)) |
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.
"may not match the behavior of custom '==' operator", (Type)) | |
"may not match behavior of custom '==' operator", (Type)) |
(Reduced written register doesn't use a definite article here.)
More pertinently, is there a word to describe what is meant better than "match"? To me, a simple reading would suggest that 'Hashable' should behave like '==', and of course that doesn't make sense; a reader might ignore the warning because why would they want two disparate things (a protocol and an operator) to "match"?
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.
"be compatible with" might be more precise, but that's still not really actionable for the user unless we spell out what "compatible" actually means. I'm not sure how verbose we want this warning to be, however. Ideally we'd be able to just stick in a reference to some relevant documentation or something.
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.
Maybe, “aligned”? Instead of “behavior,” maybe “semantics”? Not sure but I think we can get a little closer with some wordsmithing.
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.
So if we were going to really spell it out precisely, what would we say? Let's start with that and try to back off. Something like
"Any two values that compare equal with ==
must also have the same hash; because you have provided a custom ==
implementation for %0, you should also provide a custom implementation of Hashable
."
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.
Yeah, you beat me to the punch. I was going to suggest something similar:
"custom 'Equatable' conformance without custom 'Hashable' conformance may cause equivalent values to have different hashes" (fixit: insert missing declarations required by 'Hashable')
[Edit: or, "custom 'Equatable' conformance with synthesized 'Hashable' conformance may cause unexpected behavior; equivalent values must have same hash"]
@@ -1022,6 +1022,27 @@ ValueDecl *DerivedConformance::deriveHashable(ValueDecl *requirement) { | |||
if (checkAndDiagnoseDisallowedContext(requirement)) | |||
return nullptr; | |||
|
|||
// Warn the user that having a custom == almost surely require a |
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.
// Warn the user that having a custom == almost surely require a | |
// Warn the user that having a custom == almost surely requires a |
@@ -1022,6 +1022,27 @@ ValueDecl *DerivedConformance::deriveHashable(ValueDecl *requirement) { | |||
if (checkAndDiagnoseDisallowedContext(requirement)) | |||
return nullptr; | |||
|
|||
// Warn the user that having a custom == almost surely require a | |||
// custom Hashable conformance even if, for source stability reasons, | |||
// it will be synthesized |
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.
// it will be synthesized | |
// it will be synthesized. |
Build failed |
Build failed |
252b0b4
to
76da5ac
Compare
This comment has been minimized.
This comment has been minimized.
@swift-ci please smoke test |
@swift-ci please test |
This is a first approach to add the desired warning for custom
Equatable
implementation duringHashable
synthesis.It’s working for struct just fine but it crashes when running
test/Sema/implementation-only-import-in-decls.swift
for some reason it don’t likeenum
types.Other improvements before merging are increase the test coverage in
test/Sema/diag_custom_equatable_syntheszied_hashable.swift
and replicate the same warning in the opposite direction for customHashable
implementation duringEquatable
synthesis.Resolves SR-11588.