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-9425] OpenClose enum comparison uses String compare #51889

Open
milseman mannequin opened this issue Dec 6, 2018 · 22 comments
Open

[SR-9425] OpenClose enum comparison uses String compare #51889

milseman mannequin opened this issue Dec 6, 2018 · 22 comments
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself standard library Area: Standard library umbrella

Comments

@milseman
Copy link
Mannequin

milseman mannequin commented Dec 6, 2018

Previous ID SR-9425
Radar rdar://problem/46555205
Original Reporter @milseman
Type Bug
Status Resolved
Resolution Done

Attachment: Download

Additional Detail from JIRA
Votes 0
Component/s Compiler, Standard Library
Labels Bug
Assignee @LucianoPAlmeida
Priority Medium

md5: 57d87f1942178a2030684fdb18d4f810

relates to:

  • SR-11385 Automated conformance can be hijacked by extension protocol conformances that import matching members

Issue Description:

Not sure why, but OpenClose's `check_state` ends up generating a string comparison on master but not 4.2:

enum MyState : String {
    case Closed = "Closed"
    case Opened = "Opened"
}
@inline(never)
func check_state(_ state : MyState) -> Int {
  return state == .Opened ? 1 : 0
}

4.2

_$S27mega_string_regressions_4_211check_stateySiAA7MyStateOF:
 push       rbp
 mov        rbp, rsp
 push       rbx
 push       rax
 mov        ebx, edi
 movabs     rdi, 0xe600000000000000
 call       imp___stubs__swift_bridgeObjectRelease
 and        ebx, 0x1
 mov        rax, rbx
 add        rsp, 0x8
 pop        rbx
 pop        rbp
 ret

master

_$s23mega_string_regressions11check_stateySiAA7MyStateOF:
   push       rbp
   mov        rbp, rsp
   push       r14
   push       rbx
   movabs     r14, 0xe600000000000000
   test       dil, 0x1
   jne        loc_10000cf38

   movabs     rdx, 0x64656e65704f
   lea        rdi, qword [rdx+0x509fbf4]
   xor        r8d, r8d
   mov        rsi, r14
   mov        rcx, r14
   call       imp___stubs__$ss27_stringCompareWithSmolCheck__9expectingSbs11_StringGutsV_ADs01_G16ComparisonResultOtF
   mov        ebx, eax
   mov        rdi, r14
   call       imp___stubs__swift_bridgeObjectRelease
   movzx      eax, bl
   and        eax, 0x1
   jmp        loc_10000cf45

loc_10000cf38:
   mov        rdi, r14
   call       imp___stubs__swift_bridgeObjectRelease
   mov        eax, 0x1

loc_10000cf45:
   pop        rbx
   pop        r14
   pop        rbp
   ret
@milseman
Copy link
Mannequin Author

milseman mannequin commented Dec 6, 2018

@eeckstein @jckarter I would think we'd just compare discriminator bits, why is a String comparison involved here?

@jckarter
Copy link
Member

jckarter commented Dec 6, 2018

This is probably a side effect of `RawRepresentable` providing a default `==` implementation, since that will override the default synthesis.

@eeckstein
Copy link
Member

That's pretty bad.

@slavapestov, @DougGregor can we fix this?

@eeckstein
Copy link
Member

So, when it's not a String backed enum, everything works fine, i.e. the compiler synthesizes a == function.
Can we do that also for enums with raw values?

@slavapestov
Copy link
Member

@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.

@jckarter
Copy link
Member

jckarter commented Dec 7, 2018

Even for integer-raw-type enums, comparing raw values introduces an unnecessary mapping step.

@slavapestov
Copy link
Member

Perhaps we only want that implementation for enums that are actually stored as their raw value, ie @objc and imported enums.

@lorentey
Copy link
Member

lorentey commented Dec 7, 2018

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.

@belkadan
Copy link
Contributor

belkadan commented Dec 7, 2018

Right, the raw value mapping makes sense for normal RawRepresentable types (including the ones we import), but not for enums defined in Swift.

@belkadan
Copy link
Contributor

belkadan commented Dec 7, 2018

Filed rdar://problem/46555205 to make sure we at Apple don't lose track of this one.

@eeckstein
Copy link
Member

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.

@belkadan
Copy link
Contributor

belkadan commented Dec 7, 2018

I feel like it's tricky because if the user adds their own protocol that has a default ==, they might be surprised not to get it.

@milseman
Copy link
Mannequin Author

milseman mannequin commented Dec 7, 2018

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.

@xedin
Copy link
Member

xedin commented Feb 11, 2021

From the constraint solver output (on main) it looks like there are two solutions for `state == .Opened` :

  • `<Self where Self : Equatable> (Self.Type) -> (Self, Self) -> Bool`

  • `<T where T : RawRepresentable, T.RawValue : Equatable> (T, T) -> Bool`

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.

@LucianoPAlmeida
Copy link
Collaborator

Yeah, that seems an interesting one! On my queue =]

@eeckstein
Copy link
Member

@LucianoPAlmeida great, thanks!

Some more info:
For regular enums, we still do the comparison with a switch_enum in SIL. This looks large in SIL, but can be completely optimized down to a single machine instruction by LLVM.

I attached 3 files for comparison:
regular-enum.swift: this is the goal!
string_enum.swift: this is embarrassingly bad
int_enum.swift: although not so bad as with string, but also bad.

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

@LucianoPAlmeida
Copy link
Collaborator

Thanks @eeckstein, I'll start working on this soon, so those files would be useful =]

@LucianoPAlmeida
Copy link
Collaborator

@xedin
As you mentioned, given

  • `<Self where Self : Equatable> (Self.Type) -> (Self, Self) -> Bool`

  • `<T where T : RawRepresentable, T.RawValue : Equatable> (T, T) -> Bool`

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.
Right now I've trace that down to `ExprRewriter::buildDeclRef(SelectedOverload overload, DeclNameLoc loc, ConstraintLocatorBuilder locator, bool implicit)`, where

if (auto witness = conformance.getConcrete()->getWitnessDecl(decl)) {

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.

@LucianoPAlmeida
Copy link
Collaborator

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?

@LucianoPAlmeida
Copy link
Collaborator

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 =]

@LucianoPAlmeida
Copy link
Collaborator

Fixed on main by #36752
@milseman can you please verify and close =]

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@xwu
Copy link
Collaborator

xwu commented Nov 1, 2023

#36752 was backed out in #40983, so this issue is still relevant...

@xwu xwu reopened this Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself standard library Area: Standard library umbrella
Projects
None yet
Development

No branches or pull requests

8 participants