Crystal: Incorrect Enum#to_s for flag enums with explicit zero member

[jhass]

@[Flags]
enum Foo
  A = 0
  B = 2
  C = 4
end

p Foo::A | Foo::B

Expected "A, B" actual "B".

https://carc.in/#/r/10e6

[fernandes]

you can assign me to this one 👍

[asterite]

I don't think 0 should be allowed in this case. What do you think?

[oprypin]

(0 | 2) & 0 == 0. The output is correct.

[jhass]

I think for cases like https://github.com/crystal-lang/crystal/blob/master/src/openssl/lib_ssl.cr#L24 it's very important to be able to "rename" it.

[oprypin]

There is no need for a "none" member in a flags enum. It means "no members". Crystal already provides it under the name None. Adding nothing to a set keeps it the same. At the same time, let's keep the compatibility with C to not have to drop anything compared to a header definition.

All in all, I think everything is just fine and nothing needs to be done.

---

To clarify a bit more, there is no way to make this output "correct" because there is no way to detect a zero in a bitmask. bitmask | 0 == bitmask. You either always include the "none" in the output by specialcasing it or always exclude it.

---

(Foo::A | Foo::B) == Foo::B just like 0 | 2 == 2

[asterite]

Maybe we can remove the automatically created None and All for enums. Introducing implicit names in an enum is probably bad. Another reason is that would could maybe want them to be called NONE or ALL, or maybe use another name.

By the way SSLError is not a flags enum.

[jhass]

I meant to link to VerifyMode of course :(

[oprypin]

@asterite No, that makes no sense to me. None, at the very least, is mandatory (in C they'd write 0 and what do you do here?), and an implicit All is also useful so that you don't have to explicitly write it out (and take care not to make a mistake) and confuse people, especially by the output that would say E::A, E::B, E::All. Overriding the None with your own None currently works fine, and nobody prevents anyone from adding a NONE = 0 which works exactly the same as None.

---

What might work though is making all/none methods.

[fernandes]

@jhass why do you need @[Flags] on this enum?

[jhass]

From SSL_CTX_set_verify(3):

SSL_VERIFY_FAIL_IF_NO_PEER_CERT
Server mode: if the client did not return a certificate, the TLS/SSL handshake is immediately terminated with a "handshake failure" alert. This flag must
be used together with SSL_VERIFY_PEER.

Client mode: ignored

SSL_VERIFY_CLIENT_ONCE
Server mode: only request a client certificate on the initial TLS/SSL handshake. Do not ask for a client certificate again in case of a renegotiation.
This flag must be used together with SSL_VERIFY_PEER.

Client mode: ignored

[fernandes]

according to documentation None is already being added, so:

@[Flags]
enum VerifyMode : Int32
  PEER                 = 1
  FAIL_IF_NO_PEER_CERT = 2
  CLIENT_ONCE          = 4
end

Wouldn't work?

this snippet works as expected

p VerifyModel::None | VerifyMode::PEER # PEER

[jhass]

Yes, but the OpenSSL docs call it NONE, None would be unexpected IMO. And Ary and me already pointed out earlier that I got my link wrong ;)

[fernandes]

sorry, misunderstood the link comments

@[Flags]
enum VerifyMode : Int32
  NONE                 = 0 
  PEER                 = 1 
  FAIL_IF_NO_PEER_CERT = 2 
  CLIENT_ONCE          = 4 
end

p VerifyMode::NONE                           # None                                                                                                             
p VerifyMode::NONE | VerifyMode::PEER        # PEER
p VerifyMode::CLIENT_ONCE | VerifyMode::PEER # PEER, CLIENT_ONCE
p VerifyMode::All                            # PEER, FAIL_IF_NO_PEER_CERT, CLIENT_ONCE

On the OpenSSL situation the VerifyMode::NONE -> None would cause any unexpected behavior?

Not sure if NONE not appearing in All would cause any problem.

[ozra]

Just my two cents:
I like None and All being implicitly added.
I also like being able to define NONE_WHATSOEVER = 0 if I want.
I don't expect "zero-flag" in to_s ouput if any flags are set (including All).

[jhass]

I'll retract.