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

synthesized comparable for enums #25696

Merged
merged 19 commits into from
Feb 7, 2020
Merged

synthesized comparable for enums #25696

merged 19 commits into from
Feb 7, 2020

Conversation

tayloraswift
Copy link
Member

@tayloraswift tayloraswift commented Jun 23, 2019

@DougGregor
Copy link
Member

@swift-ci please smoke test

@tayloraswift
Copy link
Member Author

tayloraswift commented Sep 11, 2019

note: i had fixed the merge conflicts to the point where it’s compiling again but it crashes at run time, so the update isn’t really ready yet

@tayloraswift
Copy link
Member Author

tayloraswift commented Sep 19, 2019

@DougGregor hi, i’ve pushed changes that fixed the crasher, added support for enums with associated values, and updated the tests. i think it’s in a working state right now and it was able to pass the entire test suite, including new tests.

@DougGregor
Copy link
Member

@swift-ci please smoke test

@DougGregor
Copy link
Member

@swift-ci please test source compatibility

@DougGregor
Copy link
Member

@Kelvin13 looks like we changed something on master that broke this:

/Users/buildnode/jenkins/workspace/swift-PR-osx-smoke-test/branch-master/swift/lib/Sema/DerivedConformanceComparable.cpp:375:21: error: no member named 'setGenericEnvironment' in 'swift::FuncDecl'
15:28:09     comparableDecl->setGenericEnvironment(genericEnv);
15:28:09     ~~~~~~~~~~~~~~  ^
15:28:09 1 error generated.

@tayloraswift
Copy link
Member Author

tayloraswift commented Sep 20, 2019

can we try this again @DougGregor

@DougGregor
Copy link
Member

@swift-ci please smoke test

@DougGregor
Copy link
Member

@swift-ci please test source compatibility

@theblixguy theblixguy added the swift evolution approved Flag → feature: A feature that was approved through the Swift evolution process label Oct 25, 2019
@harlanhaskins
Copy link
Contributor

@Kelvin13 Can you fix the conflicts? I'll poke CI again.

@harlanhaskins
Copy link
Contributor

Thanks, @Kelvin13!

@swift-ci please smoke test

@harlanhaskins
Copy link
Contributor

@swift-ci please test source compatibility

@harlanhaskins
Copy link
Contributor

@swift-ci please smoke test Linux

@harlanhaskins
Copy link
Contributor

@swift-ci please test source compatibility

@harlanhaskins
Copy link
Contributor

Test failures were unrelated.

@tayloraswift
Copy link
Member Author

new version should be passing the test

@theblixguy
Copy link
Collaborator

@swift-ci please smoke test

@theblixguy
Copy link
Collaborator

@swift-ci please test source compatibility

@theblixguy
Copy link
Collaborator

@swift-ci please test source compatibility debug

@tayloraswift
Copy link
Member Author

this is all green on the checks, can we merge?

@harlanhaskins
Copy link
Contributor

@DougGregor good to merge?

@CodaFi
Copy link
Contributor

CodaFi commented Jan 31, 2020

The sizes can probably just be rationalized in one go in a follow-up.

@tayloraswift
Copy link
Member Author

Alright, the AST synthesis looks fine for now.

Please also update Interpreter/synthesized_extension_conformances.swift and add a test to test/Sema/enum_conformance_synthesis.swift that involves an extension.

done

@harlanhaskins
Copy link
Contributor

@swift-ci please test

@harlanhaskins
Copy link
Contributor

@swift-ci please test source compatibility

@swift-ci
Copy link
Contributor

swift-ci commented Feb 2, 2020

Build failed
Swift Test Linux Platform
Git Sha - d71b3df

@swift-ci
Copy link
Contributor

swift-ci commented Feb 2, 2020

Build failed
Swift Test OS X Platform
Git Sha - d71b3df

@tayloraswift
Copy link
Member Author

the error seems real, but it’s not happening when I run the build on my machine? any idea what’s going on? this is the invocation i’m using:

swift/utils/build-script --release-debuginfo -t

@benrimmington
Copy link
Contributor

the error seems real, but it’s not happening when I run the build on my machine?

Are you testing locally on Linux? It // REQUIRES: objc_interop for some reason.

@tayloraswift
Copy link
Member Author

@benrimmington yes

@CodaFi
Copy link
Contributor

CodaFi commented Feb 3, 2020

Try a release-asserts build.

@benrimmington

This comment has been minimized.

@tayloraswift
Copy link
Member Author

@benrimmington that doesn’t work, it fails to import Foundation

swift/test/Interpreter/synthesized_extension_conformances.swift:8:8: error: no such module 'Foundation'
import Foundation

adding --assert doesn’t seem to do anything different either

@tayloraswift
Copy link
Member Author

It looks like the test failure on macOS can be fixed with:

-struct SInt: Codable, Equatable, Hashable {
+struct SInt: Codable, Equatable, Hashable, Comparable {

You could also try removing the // REQUIRES: objc_interop and see if the tests pass on Linux.

Comparable can’t be synthesized on structs, only enums

@theblixguy
Copy link
Collaborator

theblixguy commented Feb 3, 2020

Comparable can’t be synthesized on structs, only enums

That is true, but that's not what the error is about. The error appears because SInt doesn't conform to Comparable and the reason why is because you added an extension to EGeneric which requires its generic parameter to be Comparable:

extension EGeneric: Comparable where T: Comparable {}

In your test function, you're using EGeneric<SInt> but the compiler is unable to synthesise Comparable conformance for EGeneric because SInt doesn't conform to Comparable.

@benrimmington

This comment has been minimized.

@tayloraswift
Copy link
Member Author

i couldn’t get Foundation to build (part of the compiler needs libssl1.0-dev and another part seems to need libcurl4-openssl-dev and they conflict with each other)

could someone trigger the ci?

@harlanhaskins
Copy link
Contributor

@Kelvin13 Can you go ahead and resolve these new conflicts? I'll kick CI after

@harlanhaskins
Copy link
Contributor

@swift-ci please test

@swift-ci
Copy link
Contributor

swift-ci commented Feb 5, 2020

Build failed
Swift Test OS X Platform
Git Sha - 178b0ff

@swift-ci
Copy link
Contributor

swift-ci commented Feb 5, 2020

Build failed
Swift Test Linux Platform
Git Sha - 178b0ff

@harlanhaskins
Copy link
Contributor

@swift-ci please test source compatibility

@tayloraswift
Copy link
Member Author

@CodaFi ping

Copy link
Contributor

@CodaFi CodaFi left a comment

Choose a reason for hiding this comment

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

:shipit:

@CodaFi
Copy link
Contributor

CodaFi commented Feb 7, 2020

⛵️

@CodaFi CodaFi merged commit 1ed846d into swiftlang:master Feb 7, 2020
@theblixguy
Copy link
Collaborator

@Kelvin13 do you mind adding this to the changelog?

@tayloraswift
Copy link
Member Author

@theblixguy #29782

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
swift evolution approved Flag → feature: A feature that was approved through the Swift evolution process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants