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-9425] OpenClose enum comparison uses String compare #51889
Comments
@eeckstein @jckarter I would think we'd just compare discriminator bits, why is a String comparison involved here? |
This is probably a side effect of `RawRepresentable` providing a default `==` implementation, since that will override the default synthesis. |
That's pretty bad. @slavapestov, @DougGregor can we fix this? |
So, when it's not a String backed enum, everything works fine, i.e. the compiler synthesizes a == function. |
@lorentey recently added a default implementation of == for RawRepresentable types. This is probably a net win for almost everything except for String-backed enums. Please discuss this with Karoy. |
Even for integer-raw-type enums, comparing raw values introduces an unnecessary mapping step. |
Perhaps we only want that implementation for enums that are actually stored as their raw value, ie @objc and imported enums. |
Weird! I merely added Hashable implementations to match the existing == function in #20705 to fix a correctness issue. The == itself is not a recent addition. |
Right, the raw value mapping makes sense for normal RawRepresentable types (including the ones we import), but not for enums defined in Swift. |
Filed rdar://problem/46555205 to make sure we at Apple don't lose track of this one. |
Can the compiler just always synthesize the == for enums and the type checker prefer the synthesized implementation over the generic == implementation? > The == itself is not a recent addition. Yes, Michael said it's not a recent regression. |
I feel like it's tricky because if the user adds their own protocol that has a default |
If we add a definition of `==<T: RawRepresentable>` with a more specific constraint that `T.RawValue` is String/Character, could we get a builtin or something else that lets us access `T`'s (actual) raw representation? If this is only used for associated-value-less-enums/option-sets, then a value of type `T` should always be bit-castable to some `UIntN`. We'd also want to do this for Double/etc. edit: For some reason I was not notified of the prior discussion and it was collapsed by JIRA. I see we're all saying similar things. Seems like we always want RawRepresentable's == to just compare T's (actual) raw representation. |
From the constraint solver output (on main) it looks like there are two solutions for `state == .Opened` :
And the solver ends up picking the former as a better solution which doesn't seem correct since `T: RawRepresentable, T.RawValue: Equatable` is a more refined requirement than `Self: Equatable`. I wonder whether it would be enough to fix ranking to pick the latter overload or would we still need to adjust synthesized implementation for RawRepresentable to compare raw representations? luciano (JIRA User) I think it might be of interest to you to take a look at this one since it has to do with CSRanking. |
Yeah, that seems an interesting one! On my queue =] |
@LucianoPAlmeida great, thanks! Some more info: I attached 3 files for comparison: Compile each file with -O and look at the final LLVM IR or the generated assembly to see how bad it is. I didn’t try, but we probably also have the same problem with the hash value: it should take the enum tag as hash value and not compute the hash from a string |
Thanks @eeckstein, I'll start working on this soon, so those files would be useful =] |
@xedin
Ranking is selecting the first because the second one is a generic function and the rule is to favor non-generic overload choices. I believe this is the correct behavior so the ranking seems fine. But here is the problem, even the ranking selecting first solution here is what we get one SIL // RUN: %target-swift-emit-silgen %s -verify | %FileCheck %s
enum MyState : String {
case closed
case opened
}
@inline(never)
func check_state(_ state : MyState) -> Int {
// CHECK: function_ref @$ss2eeoiySbx_xtSYRzSQ8RawValueRpzlF --> Swift.== infix<A where A: Swift.RawRepresentable, A.RawValue: Swift.Equatable>(A, A) -> Swift.Bool
return state == .opened ? 1 : 0
} And given the definition of the second overload is to compare raw values /// Returns a Boolean value indicating whether the two arguments are equal.
///
/// - Parameters:
/// - lhs: A raw-representable instance.
/// - rhs: A second raw-representable instance.
@inlinable // trivial-implementation
public func == <T: RawRepresentable>(lhs: T, rhs: T) -> Bool
where T.RawValue: Equatable {
return lhs.rawValue == rhs.rawValue
} This leads to generate a string comparation for this case. So I believe something is going wrong at solution application. Line 621 in 22506f9
Where `getWitnessDecl` for `Equatable` conformance in decl `<Self where Self : Equatable> (Self.Type) -> (Self, Self) -> Bool` is finding witness `<T where T : RawRepresentable, T.RawValue : Equatable> (T, T) -> Bool` This is where the effect of `RawRepresentable` providing a default `==` implementation override the default synthesis is happening. So the solution may be to always get the default synthesis witness for enums value I think. |
The solution I'm thinking is that `WitnessChecker::findBestWitness` to not consider `<T where T : RawRepresentable, T.RawValue : Equatable> (T, T) -> Bool` as a match for `<Self where Self : Equatable> (Self.Type) -> (Self, Self) -> Bool` for Equatable conformances of enum types that has synthesized `==` (not all enums have this right?) so the withness would always default to `**derived_enum_equals(: : )`. WDYT? Do you think is a viable solution? |
Yeah, seems that indeed compiler does synthesize `derived_enum_equals` for a raw value enum and the problem is just that it `WitnessChecker::findBestWitness` finds `<T where T : RawRepresentable, T.RawValue : Equatable> (T, T) -> Bool` instead of letting fall to default derived... so I'm struggling with understanding when would be correct just to not try this generic conditional witness because although we know when compiler can derive conformance for each case, check only for this would not be enough here. Should we only don't match specific stdlib declarations?(not sure is a good solution) or the solution may be come up with a rule to default to `derived_enum_equals` given a set of conditions. Just asking because I hit a wall and not sure what would be a good direction here. If anyone has some hint, really appreciate =] |
Attachment: Download
Additional Detail from JIRA
md5: 57d87f1942178a2030684fdb18d4f810
relates to:
Issue Description:
Not sure why, but OpenClose's `check_state` ends up generating a string comparison on master but not 4.2:
4.2
master
The text was updated successfully, but these errors were encountered: