Synthesize Equatable/Hashable for complex enums, structs #9619

allevato · 2017-05-15T21:28:05Z

This is the implementation of the yet-to-be-merged/reviewed evolution proposal to synthesize Equatable/Hashable for enums with payloads and for structs (apple/swift-evolution#706). I'm offering this in the hopes that we can get the proposal reviewed, any updates if needed can be made to the implementation, and it can make it into Swift 4.

This is my first foray into a big change to the compiler itself, so I'm sure there are a number of subtleties about building these ASTs that may have gone over my head. I'm very open to any feedback anyone has on these changes!

CodaFi · 2017-05-15T23:00:39Z

lib/AST/ASTContext.cpp

+
+  auto intType = getIntDecl()->getDeclaredType();
+  auto inoutIntType = InOutType::get(intType);
+  SmallVector<ValueDecl *, 30> xorFuncs;


Nit: Are there actually 30 of these? Usually SmallVector sizes are powers of 2, so 32 would be more apropos

Thanks for pointing that out—I didn't go back and change it after I cribbed the function from ==. It looks like there are currently ~12, so I'll use 16.

CodaFi · 2017-05-15T23:03:19Z

lib/Sema/DerivedConformanceEquatableHashable.cpp

  auto *cmpExpr = new (C) BinaryExpr(cmpFuncExpr, abTuple, /*implicit*/ true);
  statements.push_back(new (C) ReturnStmt(SourceLoc(), cmpExpr));
-
+  
  BraceStmt *body = BraceStmt::create(C, SourceLoc(), statements, SourceLoc());
  eqDecl->setBody(body);
 }


Please check your editor's whitespace setting and remove the above space changes.

CodaFi · 2017-05-15T23:12:29Z

lib/Sema/DerivedConformanceEquatableHashable.cpp

+    } else {
+      // One associated value with no label is represented as a paren type.
+      auto actualType = argumentType->getWithoutParens();
+      if (!typeConformsToProtocol(tc, declContext, actualType, protocol)) {


Nit: This, and the call above it, can be used as the return value of this branch.

How so? Unless I'm misunderstanding you, we only want to early-exit if we find a non-conforming type; we have to see the entire loop through before we can know that we can return true.

CodaFi · 2017-05-15T23:12:54Z

lib/Sema/DerivedConformanceEquatableHashable.cpp

+    theStruct->getStoredProperties(/*skipInaccessible=*/true);
+  for (auto propertyDecl : storedProperties) {
+    auto propertyType = propertyDecl->getType();
+    if (!typeConformsToProtocol(tc, declContext, propertyType, protocol)) {


Same comment.

CodaFi · 2017-05-15T23:15:03Z

lib/Sema/DerivedConformanceEquatableHashable.cpp

-      return deriveEquatable_enum_eq(tc, parentDecl, theEnum);
+    if (auto theEnum = dyn_cast<EnumDecl>(type)) {
+      auto bodySynthesizer = theEnum->hasOnlyCasesWithoutAssociatedValues()
+      ? &deriveBodyEquatable_enum_noAssociatedValues_eq


Might want to run clang-format in this region.

CodaFi · 2017-05-15T23:16:10Z

Overall it looks pretty good. I'll have more in-depth comments later.

CodaFi · 2017-05-15T23:17:02Z

test/Sema/struct_equatable_hashable.swift

+
+// FIXME: This should work.
+func overloadFromOtherFile() -> YetAnotherFromOtherFile { return YetAnotherFromOtherFile(v: 1.2) }
+func overloadFromOtherFile() -> Bool { return false }


What's up here?

Oops, forgot to go back and remove that after pulling the examples over from the enum version of the tests. (I'm guessing the bug that was the reason for the FIXME in the enum version was fixed but the comment mistakenly remained in that file?)

I've removed the FIXME from the other file as well.

CodaFi · 2017-05-15T23:20:06Z

lib/AST/ASTContext.cpp

@@ -908,6 +914,75 @@ FuncDecl *ASTContext::getEqualIntDecl() const {
  return nullptr;
 }

+FuncDecl *ASTContext::getMutatingXorIntDecl() const {


Thinking out loud: This code has been copied and pasted enough times it's probably worth it to factor it out at some point...

I'll tinker with factoring the commonalities out into a function that takes a callback and loop back around.

I've just pushed a new commit that factors out the common code for this and for the intrinsic library function lookups.

CodaFi · 2017-05-15T23:26:07Z

lib/AST/Decl.cpp

+      // DerivedConformance::derive{Equatable,Hashable} perform the full check
+      // and it outputs its own diagnostics if conformance cannot be
+      // synthesized.
+      return enumDecl->hasCases();


If this check has to become semantic, maybe libAST isn't the right place for it.

If you can, look into moving this into the type checker itself, and diagnosing the specific members that are hindering synthesis

Since this is my first time really diving into the various compiler components, can you elaborate a little more on what you mean here w.r.t moving it into the type checker?

Re: diagnostics, I think it would be great addition to be able to emit diagnostics that say "protocol couldn't be synthesized because such and such does not conform to protocol", but this is also something that we'd want across the board for Codable and future derived protocols as well. In the interest of scoping this PR, could that be something added to both later?

It absolutely could. The layering here makes me uncomfortable all the same, which is the first half of the comment.

To resolve this: take a look at all the places this accessor is called, I'll bet you most/all of them are in Sema. If that's the case, then move it down into the type checker itself and quash the FIXME.

Understood! It looks like this is only called from Sema, and all those call sites provide easy access to TypeChecker so this should be fairly straightforward to refactor. I'll work on it.

Just pushed a new commit. I didn't move the method into the type checker per se, but into the DerivedConformance namespace where it seemed more appropriate (and it now takes a TC as an argument, so it can do all the derivability checks at once). I resolved the FIXME for Equatable/Hashable but left Codable as-is for now. PTAL.

allevato · 2017-05-16T20:37:24Z

Thanks for the feedback! I've pushed the low-hanging formatting clean-up and am working on the more involved items.

allevato · 2017-05-20T22:20:00Z

I've updated this PR (and the proposal) to forbid synthesis in extensions, to stay in sync with the recently merged Codable change (#9758).

CodaFi · 2017-05-21T01:02:05Z

@swift-ci please smoke test

allevato · 2017-05-22T15:16:23Z

I'm a bit puzzled at why the swiftpm build is crashing on Linux but not on macOS in that smoke test (especially without a stack trace to look at).

I'll try to get a development environment set up on a Linux instance but it'll take a little while to carve out that time.

CodaFi · 2017-05-30T18:55:08Z

@allevato Could you rebase? I'll try to run this on a Linux box for you.

allevato · 2017-05-30T22:09:47Z

@CodaFi I've rebased and pushed. Thanks for taking a look at this!

allevato · 2017-05-31T17:24:21Z

I did a little more digging and found that the smoke tests on macOS don't build Swift Package Manager, which explains why the problem didn't manifest there. I built it explicitly on my Mac and got the same crash that was in the CI logs, so now I have something to debug with.

allevato · 2017-06-02T04:31:17Z

I've pushed a new commit that fixes the type checker issue, added unit tests to verify the fix, and also verified that Swift Package Manager builds with this change.

Can you please kick off another smoke test?

CodaFi · 2017-06-02T05:35:59Z

@swift-ci please smoke test

slavapestov · 2017-10-10T21:18:56Z

@swift-ci Please test

slavapestov · 2017-10-10T21:19:02Z

@swift-ci Please test source compatibility

slavapestov · 2017-10-10T21:19:32Z

@swift-ci Please test macOS

slavapestov · 2017-10-10T21:20:18Z

Looks like CI is down for maintenance. Should be back soon

swift-ci · 2017-10-10T22:16:47Z

Build failed
Swift Test Linux Platform
Git Sha - ddaa390

allevato · 2017-10-10T22:20:55Z

It looks like that failure might be unrelated, but can someone else verify?

<unknown>:0: error: module map file '/home/buildnode/jenkins/workspace/swift-PR-Linux/branch-master/swift-corelibs-libdispatch/dispatch/module.modulemap' not found
<unknown>:0: error: missing required module 'SwiftShims'
ninja: build stopped: subcommand failed.

DougGregor · 2017-10-10T22:31:13Z

@swift-ci please clean smoke test Linux

DougGregor · 2017-10-10T22:31:25Z

@swift-ci please clean smoke test Linux

DougGregor · 2017-10-10T22:32:38Z

Yeah, it's unrelated. Would like to get actual Linux testing though

DougGregor · 2017-10-10T22:32:41Z

@swift-ci please clean smoke test Linux

DougGregor · 2017-10-10T22:32:53Z

@swift-ci please clean test Linux

therealbnut · 2017-10-11T01:25:28Z

🎉 congratulations @allevato et al. I look forward to seeing this in an upcoming Swift release! (hopefully not too far away 😉)

lattner · 2017-10-11T03:59:21Z

Great work @allevato !

solidcell · 2017-10-14T09:18:58Z

Thanks so much @allevato! This was one of my top most wanted features in Swift.

Is this likely to make it into a specific upcoming version? I'm not sure how release coordination works for Swift.

lattner · 2017-10-15T22:23:00Z

This is most likely to go out in "swift 4.1" in spring 2018

bielikb · 2017-12-13T14:59:33Z

Hi guys, today I've tried snapshot of Swift 4.1. (4.1-DEVELOPMENT-SNAPSHOT-2017-12-12-a) I've tried to define enum with associated values in which the value also conforms to Equatable.

enum EnumWithAssociatedValues : Equatable {
    case first(String)
    case second(String)
}

However the conformance was not added and compiler threw couple of errors. Is it possible I misread the proposal and enums/value types with associated values are not yet supported/not part of this proposal? Thanks for your answer in advance.

xwu · 2017-12-13T18:06:25Z

@bielikb Your example works without any errors for me. Are you set up to use the toolchain you downloaded? (Xcode Playgrounds, for example, do not.)

bielikb · 2017-12-13T22:05:35Z

Ive tried to use both - terminal (set local version using swiftenv) & xcode playgrounds (set toolchain). Ill check it tomorrow. Equatable/Hashable conformance worked for enums/structs with no associated types without any problem.

ematejska · 2017-12-14T21:40:25Z

@bielikb If you want to easily try it and make sure that you're using the correct toolchain do the following:

Launch Xcode and select Xcode->Toolchains->your_downloaded_toolchain
Create a new command line project for MacOS (File->New->macOs->Command Line Tool)
Go to your project and select the main.swift file and paste your code you want to test.

The problem you have is that Playgrounds will not respect the toolchain setting so using those will not work for testing.

bielikb · 2017-12-15T09:55:16Z

@ematejska Thanks for hints, that's something I already know.
Im getting errors also for swift script file.

Could you try to build the Swift script that I attached to this comment, if it works for you? Maybe then it's env related problem, rather then implementation itself. When I run Enums.swift with associated values, I got couple of compiler errors. If I remove associated values from declaration, it builds just fine.
Please note that the attached zip contains also .swift-version file that specifies 4.1 snapshot I'm using.

Feel free to correct me, if I'm doing something wrong.

enum_associated_values.zip

ematejska · 2017-12-15T18:01:23Z

@bielikb I already had the 4.1-Snapshot-2017-12-13(a) toolchain installed so I tried it with that and Xcode 9. Seems to work for me. I saw errors with the default toolchain that was shipped with Xcode 9 or the form "error: type 'EnumWithAssociatedValues' does not conform to protocol 'Equatable'" and then when I switched to the 4.1-Snapshot, I was able to build successfully.

Just to be clear, I didn't run this as a script but pasted your code into my command line project as described in the previous comment.

bielikb · 2017-12-15T19:44:24Z

@ematejska I've tried to run it in XCode 9.1 in sample project and it worked! As you mentioned. I however still get problems to build it as a script - but that might be problem of choosing the correct Swift toolchain - e.g. although I specified 4.1 snapshot as local swift version through swiftenv for dir with my Enums.swift, it's probably picking 4.0.2 toolchain. Thanks for help & getting me out of the "fog". I cannot wait until 4.1 will hit the world, good night boiler plate.

benrimmington · 2017-12-16T09:05:34Z

@bielikb Try including the xctoolchain alias/identifier in your script:

#!/usr/bin/xcrun --toolchain 'swift' swift

or

#!/usr/bin/xcrun --toolchain 'org.swift.4120171212a' swift

bielikb · 2017-12-16T14:50:34Z

@benrimmington I've tried the former definition and it worked! Thanks a lot guys ;)

CodaFi reviewed May 15, 2017

View reviewed changes

allevato force-pushed the synthesize-equatable-hashable branch from eb2735b to 0ff89fd Compare May 20, 2017 22:09

CodaFi added the swift evolution pending discussion Flag → feature: A feature that has a Swift evolution proposal currently in review label May 21, 2017

allevato force-pushed the synthesize-equatable-hashable branch 3 times, most recently from 5dc8f98 to 244b5a7 Compare May 30, 2017 22:09

allevato added 7 commits August 8, 2017 18:20

[AST] Accessors for ^= and _mixInt

eda0e38

[AST] Add identifier for synthesized struct equals

a825a31

[AST] Add DeclContext.getAsStructOrStructExtensionContext

6dfa87d

[AST] Enable derived Equatable/Hashable for structs

4777f9d

[Sema] Synthesize Eq/Hash for enums with payloads, structs

425de49

[test] Update enum Equatable/Hashable tests

5f39659

Formatting and other cleanup.

aa93ee5

slavapestov merged commit 124251c into apple:master Oct 11, 2017

allevato deleted the synthesize-equatable-hashable branch October 11, 2017 01:47

BasThomas mentioned this pull request Oct 17, 2017

[92] Issue #92 - Oct 26, 2017 SwiftWeekly/swiftweekly.github.io#325

Closed

allevato mentioned this pull request Oct 24, 2017

Synthesize ==/hashValue for tuple fields/payloads #12598

Closed

ikesyo mentioned this pull request Oct 29, 2017

SE-0185 is implemented in Swift master apple/swift-evolution#756

Merged

xwu mentioned this pull request Nov 23, 2017

[stdlib/Sema/AST] Adopt a commonly used implementation for combining hashes #13056

Merged

Synthesize Equatable/Hashable for complex enums, structs #9619

Synthesize Equatable/Hashable for complex enums, structs #9619

Conversation

allevato commented May 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodaFi commented May 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allevato commented May 16, 2017

allevato commented May 20, 2017

CodaFi commented May 21, 2017

allevato commented May 22, 2017

CodaFi commented May 30, 2017

allevato commented May 30, 2017

allevato commented May 31, 2017

allevato commented Jun 2, 2017

CodaFi commented Jun 2, 2017

slavapestov commented Oct 10, 2017

slavapestov commented Oct 10, 2017

slavapestov commented Oct 10, 2017

slavapestov commented Oct 10, 2017

swift-ci commented Oct 10, 2017

allevato commented Oct 10, 2017

DougGregor commented Oct 10, 2017

DougGregor commented Oct 10, 2017

DougGregor commented Oct 10, 2017

DougGregor commented Oct 10, 2017

DougGregor commented Oct 10, 2017

therealbnut commented Oct 11, 2017

lattner commented Oct 11, 2017

solidcell commented Oct 14, 2017

lattner commented Oct 15, 2017

bielikb commented Dec 13, 2017 • edited

xwu commented Dec 13, 2017

bielikb commented Dec 13, 2017

ematejska commented Dec 14, 2017 • edited

bielikb commented Dec 15, 2017 • edited

ematejska commented Dec 15, 2017

bielikb commented Dec 15, 2017 • edited

benrimmington commented Dec 16, 2017

bielikb commented Dec 16, 2017

bielikb commented Dec 13, 2017 •

edited

ematejska commented Dec 14, 2017 •

edited

bielikb commented Dec 15, 2017 •

edited

bielikb commented Dec 15, 2017 •

edited