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 #53993

Open
beccadax opened this issue Oct 8, 2019 · 9 comments
Assignees
Labels
compiler The Swift compiler in itself derived conformances Feature → protocol → conformances: derived conformances aka synthesized conformances good first issue Good for newcomers improvement

Comments

@beccadax
Copy link
Contributor

beccadax commented Oct 8, 2019

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

JGiola commented Oct 19, 2019

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
Copy link
Contributor Author

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

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

JGiola commented Oct 20, 2019

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

JGiola commented Oct 20, 2019

@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
Copy link
Contributor Author

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

JGiola commented Oct 20, 2019

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

JGiola commented Nov 15, 2019

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

JGiola commented May 12, 2020

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

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler The Swift compiler in itself derived conformances Feature → protocol → conformances: derived conformances aka synthesized conformances good first issue Good for newcomers improvement
Projects
None yet
Development

No branches or pull requests

3 participants