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

[beccadax]

Previous ID	SR-11588
Radar	rdar://problem/55792988
Original Reporter	@beccadax
Type	Improvement
Status	In Progress
Resolution

Additional Detail from JIRA

Votes	0
Component/s	Compiler
Labels	Improvement, DerivedConformance, StarterBug
Assignee	@JGiola
Priority	Medium

md5: 023a941f08cf5b893763b728c33c0c83

relates to:

• SR-14665 Synthesised Equatable may be incorrect for manually implemented Comparable

Issue Description:

Types that have a custom == almost surely also require a custom Hashable conformance (i.e. the synthesized one will be wrong). Ideally, we wouldn’t synthesize if there’s a custom equatable, but that’s not source-stable. The compiler should emit a warning if it synthesizes a Hashable conformance for a type whose ==(Self, Self) -> Bool implementation is not also synthesized.

This should be relatively simple to do: in the "full Hashable derivation" part of DerivedConformanceEquatableHashable.cpp, after we have decided that we are definitely going to derive an implementation, find the == operator in the type's Equatable conformance and see if it isImplicit(). If it is, emit a warning.

[JGiola]

Hi all, I want to take a chance on this issue but I have two question.

1. I don’t totally grasp the Sema logic and so there are some places where I can take inspiration on how I can get the type Equatable conformance?

2. There is already a possible text for the warning or I can come up with a proposal and change it then in the PR?

[beccadax]

Thanks for taking this on! To answer your questions:

1. You'll want to use TypeChecker::conformsToProtocol() to get the conformance. There are examples of this function in DerivedConformanceEquatableHashable.cpp; they usually just check whether there is a conformance and then throw it away, but they might help you figure out how to turn the variables you have into the arguments it needs. For examples where we actually use getWitnessByName() on the conformance it returns, look at the fetchProtocolInitWitness and finishDictionaryExpr() helpers in CSApply.cpp—these look up initializers, but looking up an ==(_:_:) method should be pretty similar.

2. You'll need to add a new warning to DiagnosticsSema.def. Since you're the one implementing this feature, use diagnostics that make sense to you. 🙂 docs/Diagnostics.md has good guidelines for Swift's diagnostic style.

(If you want to know what I'd do, I would probably diagnose the warning at the location of the user's == function. The wording would be along the lines of "automatically generated 'Hashable' implementation for type %0 may not match the behavior of custom '==' operator"; I'd probably also include a note saying "add a custom 'hash(into:)' method" with a fix-it that inserts a stub implementation. But that's just me.)

A couple other things:

• Your change won't be accepted without tests. test/Sema/enum_conformance_synthesis.swift contains similar tests and is already set up to verify diagnostics; the "Diagnostic Verifier" section of docs/Diagnostics.md explains how to test for diagnostics, and there are a few examples in that file already (although none with fix-its). Make sure you include both examples where it should and shouldn't diagnose, and make sure it works properly with relevant language features like extensions and conditional conformances. If you want to test this feature's behavior when some of the declarations are in another file, test/Sema/Inputs/enum_conformance_synthesis_other.swift is also a part of this test.

• Once you've created a pull request, you'll need to get it reviewed and tested by Swift CI. Unfortunately, only committers can formally assign reviewers or command the CI bot, but if you post a comment mentioning @brentdax (me), @stephentyrone (who filed the original Radar asking for this warning), and @jrose-apple (who is generally helpful and has lots of opinions about diagnostics), one of us can get the ball rolling for you.

Good luck, and let me know if you need any help!

[stephentyrone]

One more thing to add: it's worth doing the other direction as well; types with a custom Hashable conformance should also have a custom Equatable.

[JGiola]

I’ve arrived to a solution to print the warning and the note, and I have a minimal test suite to check that in fact it is working.

Can I open a PR in WIP to show the solution for confirmation and then for getting guidance in the future on missing test cases? Or is better to fill out all the cases and then open an official PR?

[JGiola]

@stephentyrone for the warning in the other direction is better to track it on a different issue/PR or I can expand the scope of this one?

[beccadax]

Please do open a draft PR and post the link here.

(If you want to add the other warning too, you might as well use this bug report and PR to do it. No need to create more SRs and PRs to update.)

[JGiola]

I have opened #27801 but I discovered that is crashing on me when the type is an enum, and I’m trying to find why...

[JGiola]

Anyone knows who can help me on this PR? I’m unable to find how to check if the conformance is not in my module and so skip the witness search, or if I’m doing something wrong in the checks

[JGiola]

Hi brentdax (JIRA User), sorry to ping but I'm stuck on this and i don't know where I can find information on how to apply the correct check to avoid the crash described in the pull request. Do you know someone that can help me point in the right direction?

Thank you