Skip to content
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

[SR-11588]Warn about derived Hashable implementation if there’s a custom Equatable #27801

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JGiola
Copy link
Contributor

@JGiola JGiola commented Oct 20, 2019

This is a first approach to add the desired warning for custom Equatable implementation during Hashable 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 like enum 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 custom Hashable implementation during Equatable synthesis.

Resolves SR-11588.

@JGiola JGiola changed the title Warn about derived Hashable implementation if there’s a custom Equatable [WIP][SR-11588]Warn about derived Hashable implementation if there’s a custom Equatable Oct 20, 2019
WARNING(hashable_customEquatable, none,
"Automatically generated 'Hashable' implementation for type %0 "
"may not match the behavior of custom '==' operator",
(Type))
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. 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.
  2. Diagnostic text must start with a lowercase letter.
  3. 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", ())
Copy link
Collaborator

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();
Copy link
Collaborator

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();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: nominalTy/nominalType?

@theblixguy
Copy link
Collaborator

theblixguy commented Oct 20, 2019

Please run clang-format as well to ensure your changes fit within the 80-column limit!

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 like enum types.

Can you share the stack trace?

@stephentyrone
Copy link
Member

cc @lorentey

@JGiola
Copy link
Contributor Author

JGiola commented Oct 21, 2019

Please run clang-format as well to ensure your changes fit within the 80-column limit!

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?

@JGiola
Copy link
Contributor Author

JGiola commented Oct 21, 2019

Can you share the stack trace?

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()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (!equalsDeclaration->isImplicit()) {
if (equalsDeclaration && !equalsDeclaration->isImplicit()) {

just in case we don't have a decl. Also, check if it fixes that crash

Copy link
Contributor Author

@JGiola JGiola Oct 21, 2019

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.

Copy link
Contributor

@beccadax beccadax Oct 22, 2019

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.

Copy link
Contributor Author

@JGiola JGiola Oct 22, 2019

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 😳

Copy link
Contributor Author

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)
        }
      }
    }
  }
}

Copy link
Contributor Author

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

@JGiola
Copy link
Contributor Author

JGiola commented Dec 6, 2019

@DougGregor sorry to ping, but I've read in the CODE_OWNERS file that you are associated with the Sema. Do you know how can I catch that the equality conformance is implemented in another module and that I cannot get the witness for it?

I‘ve tried to look everywhere but I’m not seeing anything useful for a check against it :(

@beccadax
Copy link
Contributor

@swift-ci please smoke test


enum ImportedConformance: ImplicitEquatable {
case x = 1
}
Copy link
Contributor

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.

Copy link
Contributor

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.)

Copy link
Contributor Author

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.

@shahmishal
Copy link
Member

Please update the base branch to main by Oct 5th otherwise the pull request will be closed automatically.

  • How to change the base branch: (Link)
  • More detail about the branch update: (Link)

// it will be synthesized
auto equatableProto = C.getProtocol(KnownProtocolKind::Equatable);
auto eqConformance = TypeChecker::conformsToProtocol(nominalTy,
equatableProto, getConformanceContext());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should getConformanceContext be getParentModule instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm pulling and building it right away (probably I will finish tomorrow knowing my MacBook Pro 😄). If this will solve it you will made me the happiest man on earth!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It worked!

@JGiola JGiola force-pushed the SR-11588 branch 2 times, most recently from e5cf2b8 to 3778c30 Compare June 2, 2021 12:15
@JGiola JGiola marked this pull request as ready for review June 2, 2021 12:15
@JGiola JGiola changed the title [WIP][SR-11588]Warn about derived Hashable implementation if there’s a custom Equatable [SR-11588]Warn about derived Hashable implementation if there’s a custom Equatable Jun 2, 2021
@JGiola
Copy link
Contributor Author

JGiola commented Jun 4, 2021

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

@beccadax
Copy link
Contributor

beccadax commented Jun 6, 2021

@swift-ci please test

@swift-ci
Copy link
Collaborator

swift-ci commented Jun 6, 2021

Build failed
Swift Test Linux Platform
Git Sha - 3778c30ec33b56fd6a73050c0ebba5fd803a53a0

@swift-ci
Copy link
Collaborator

swift-ci commented Jun 6, 2021

Build failed
Swift Test OS X Platform
Git Sha - 3778c30ec33b56fd6a73050c0ebba5fd803a53a0

@JGiola
Copy link
Contributor Author

JGiola commented Aug 24, 2021

I've rebased the repo and run all the tests with utils/run-test --lit ../llvm-project/llvm/utils/lit/lit.py ../build/Ninja-RelWithDebInfoAssert/swift-macosx-$(uname -m)/test-macosx-$(uname -m) and nothing is breaking this time.

@xwu
Copy link
Collaborator

xwu commented Aug 25, 2021

@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 "
Copy link
Collaborator

@xwu xwu Aug 25, 2021

Choose a reason for hiding this comment

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

Suggested change
"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))
Copy link
Collaborator

@xwu xwu Aug 25, 2021

Choose a reason for hiding this comment

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

Suggested change
"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"?

Copy link
Member

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.

Copy link
Collaborator

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.

Copy link
Member

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

Copy link
Collaborator

@xwu xwu Aug 25, 2021

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// 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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// it will be synthesized
// it will be synthesized.

@swift-ci
Copy link
Collaborator

Build failed
Swift Test Linux Platform
Git Sha - bdc68ea659ec34c482f1c26facfb65fc90c7750e

@swift-ci
Copy link
Collaborator

Build failed
Swift Test OS X Platform
Git Sha - bdc68ea659ec34c482f1c26facfb65fc90c7750e

@WowbaggersLiquidLunch

This comment has been minimized.

@WowbaggersLiquidLunch
Copy link
Collaborator

@swift-ci please smoke test

@WowbaggersLiquidLunch
Copy link
Collaborator

@swift-ci please test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants