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

Synthesize Equatable/Hashable for complex enums, structs #9619

Merged
merged 27 commits into from Oct 11, 2017

Conversation

@allevato
Copy link
Contributor

allevato commented May 15, 2017

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!

lib/AST/ASTContext.cpp Outdated

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

This comment has been minimized.

Copy link
@CodaFi

CodaFi May 15, 2017

Collaborator

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

This comment has been minimized.

Copy link
@allevato

allevato May 16, 2017

Author Contributor

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.

lib/Sema/DerivedConformanceEquatableHashable.cpp Outdated
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);
}

This comment has been minimized.

Copy link
@CodaFi

CodaFi May 15, 2017

Collaborator

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

This comment has been minimized.

Copy link
@allevato

allevato May 16, 2017

Author Contributor

Done.

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

This comment has been minimized.

Copy link
@CodaFi

CodaFi May 15, 2017

Collaborator

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

This comment has been minimized.

Copy link
@allevato

allevato May 16, 2017

Author Contributor

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.

lib/Sema/DerivedConformanceEquatableHashable.cpp Outdated
theStruct->getStoredProperties(/*skipInaccessible=*/true);
for (auto propertyDecl : storedProperties) {
auto propertyType = propertyDecl->getType();
if (!typeConformsToProtocol(tc, declContext, propertyType, protocol)) {

This comment has been minimized.

Copy link
@CodaFi

CodaFi May 15, 2017

Collaborator

Same here

This comment has been minimized.

Copy link
@allevato

allevato May 16, 2017

Author Contributor

Same comment.

lib/Sema/DerivedConformanceEquatableHashable.cpp Outdated
return deriveEquatable_enum_eq(tc, parentDecl, theEnum);
if (auto theEnum = dyn_cast<EnumDecl>(type)) {
auto bodySynthesizer = theEnum->hasOnlyCasesWithoutAssociatedValues()
? &deriveBodyEquatable_enum_noAssociatedValues_eq

This comment has been minimized.

Copy link
@CodaFi

CodaFi May 15, 2017

Collaborator

Might want to run clang-format in this region.

This comment has been minimized.

Copy link
@allevato

allevato May 16, 2017

Author Contributor

Done.

@CodaFi

This comment has been minimized.

Copy link
Collaborator

CodaFi commented May 15, 2017

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

test/Sema/struct_equatable_hashable.swift Outdated

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

This comment has been minimized.

Copy link
@CodaFi

CodaFi May 15, 2017

Collaborator

What's up here?

This comment has been minimized.

Copy link
@allevato

allevato May 16, 2017

Author Contributor

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.

lib/AST/ASTContext.cpp Outdated
@@ -908,6 +914,75 @@ FuncDecl *ASTContext::getEqualIntDecl() const {
return nullptr;
}

FuncDecl *ASTContext::getMutatingXorIntDecl() const {

This comment has been minimized.

Copy link
@CodaFi

CodaFi May 15, 2017

Collaborator

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

This comment has been minimized.

Copy link
@allevato

allevato May 16, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@allevato

allevato May 16, 2017

Author Contributor

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

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

This comment has been minimized.

Copy link
@CodaFi

CodaFi May 15, 2017

Collaborator

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

This comment has been minimized.

Copy link
@CodaFi

CodaFi May 15, 2017

Collaborator

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

This comment has been minimized.

Copy link
@allevato

allevato May 16, 2017

Author Contributor

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?

This comment has been minimized.

Copy link
@CodaFi

CodaFi May 16, 2017

Collaborator

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.

This comment has been minimized.

Copy link
@allevato

allevato May 17, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@allevato

allevato May 17, 2017

Author Contributor

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

This comment has been minimized.

Copy link
Contributor Author

allevato commented May 16, 2017

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

@allevato allevato force-pushed the allevato:synthesize-equatable-hashable branch May 20, 2017
@allevato

This comment has been minimized.

Copy link
Contributor Author

allevato commented May 20, 2017

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

This comment has been minimized.

Copy link
Collaborator

CodaFi commented May 21, 2017

@swift-ci please smoke test

@allevato

This comment has been minimized.

Copy link
Contributor Author

allevato commented May 22, 2017

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

This comment has been minimized.

Copy link
Collaborator

CodaFi commented May 30, 2017

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

@allevato allevato force-pushed the allevato:synthesize-equatable-hashable branch 3 times, most recently May 30, 2017
@allevato

This comment has been minimized.

Copy link
Contributor Author

allevato commented May 30, 2017

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

@allevato

This comment has been minimized.

Copy link
Contributor Author

allevato commented May 31, 2017

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

This comment has been minimized.

Copy link
Contributor Author

allevato commented Jun 2, 2017

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

This comment has been minimized.

Copy link
Collaborator

CodaFi commented Jun 2, 2017

@swift-ci please smoke test

@slavapestov

This comment has been minimized.

Copy link
Member

slavapestov commented Oct 10, 2017

@swift-ci Please test source compatibility

@slavapestov

This comment has been minimized.

Copy link
Member

slavapestov commented Oct 10, 2017

@swift-ci Please test macOS

@slavapestov

This comment has been minimized.

Copy link
Member

slavapestov commented Oct 10, 2017

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

@swift-ci

This comment has been minimized.

Copy link
Contributor

swift-ci commented Oct 10, 2017

Build failed
Swift Test Linux Platform
Git Sha - ddaa390

@allevato

This comment has been minimized.

Copy link
Contributor Author

allevato commented Oct 10, 2017

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

This comment has been minimized.

Copy link
Member

DougGregor commented Oct 10, 2017

@swift-ci please clean smoke test Linux

1 similar comment
@DougGregor

This comment has been minimized.

Copy link
Member

DougGregor commented Oct 10, 2017

@swift-ci please clean smoke test Linux

@DougGregor

This comment has been minimized.

Copy link
Member

DougGregor commented Oct 10, 2017

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

@DougGregor

This comment has been minimized.

Copy link
Member

DougGregor commented Oct 10, 2017

@swift-ci please clean smoke test Linux

@DougGregor

This comment has been minimized.

Copy link
Member

DougGregor commented Oct 10, 2017

@swift-ci please clean test Linux

@slavapestov slavapestov merged commit 124251c into apple:master Oct 11, 2017
5 checks passed
5 checks passed
Swift Source Compatibility Suite on macOS Platform Build finished.
Details
Swift Test Linux Platform 10481 tests run, 0 skipped, 0 failed.
Details
Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform 52530 tests run, 0 skipped, 0 failed.
Details
Swift Test OS X Platform (smoke test)
Details
@therealbnut

This comment has been minimized.

Copy link
Contributor

therealbnut commented Oct 11, 2017

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

@allevato allevato deleted the allevato:synthesize-equatable-hashable branch Oct 11, 2017
@lattner

This comment has been minimized.

Copy link
Collaborator

lattner commented Oct 11, 2017

Great work @allevato !

@solidcell

This comment has been minimized.

Copy link

solidcell commented Oct 14, 2017

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

This comment has been minimized.

Copy link
Collaborator

lattner commented Oct 15, 2017

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

@bielikb

This comment has been minimized.

Copy link

bielikb commented Dec 13, 2017

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

This comment has been minimized.

Copy link
Collaborator

xwu commented Dec 13, 2017

@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

This comment has been minimized.

Copy link

bielikb commented Dec 13, 2017

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

This comment has been minimized.

Copy link
Collaborator

ematejska commented Dec 14, 2017

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

  1. Launch Xcode and select Xcode->Toolchains->your_downloaded_toolchain
  2. Create a new command line project for MacOS (File->New->macOs->Command Line Tool)
  3. 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

This comment has been minimized.

Copy link

bielikb commented Dec 15, 2017

@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

This comment has been minimized.

Copy link
Collaborator

ematejska commented Dec 15, 2017

@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

This comment has been minimized.

Copy link

bielikb commented Dec 15, 2017

@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

This comment has been minimized.

Copy link
Contributor

benrimmington commented Dec 16, 2017

@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

This comment has been minimized.

Copy link

bielikb commented Dec 16, 2017

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

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

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.