Remove None and All default values for flag enums.

[straight-shoota]

Continuation from https://github.com/crystal-lang/crystal/issues/7270#issuecomment-451947228

With most flag enums the semantics of None and All probably make sense. That's why they were added as defaults.

But they're not necessarily always semantically valid. When some of the flags are mutually exclusive, All is not possible. When setting at least one flag is required, None doesn't make sense either.

Because of this they better shouldn't be added to every flag enum.

Autogeneration can simply be removed. Where appropriate, these members can be added manually as None = 0 and All = ? (would it be Int32::MAX?).

Another option could be to configure the generation of these special enum members via the Flags annotation. For example, @[Flags(all: false)] would not autogenerate a None member. We could keep the current semantics by default (plain @[Flags] annotation generates both special members) but allow to opt-out when the autogenerated values are not applicable. This is reasonable when most flag enums use All and None members (which I assume, but it's not based on evidence). This solution adds more complexity to the compiler and another feature to the language. The alternative (remove + manual creation) is easier to follow because there is only one syntax and it's explicit.

Considering the highest desired state where each enum member would be documented with a individual description, autogeneration (in either form) is not useful because it can't generate meaningful documentation anyway.

Thus, I think I would prefer to remove autogeneration completely and require explicit declaration.

N.B. If we keep autogeneration in any form, there should be a fix to ensure autogenerated members are visible in the API docs. The documentation needs to be manually overridable.

[bcardiff]

The point of the autogeneration is

1. to avoid doing all the bitwise-or with values or manually calculate them (All)
2. ensure there is always a None constant.
3. to have a clear semantics to exclude All and None constants as members in Enum.each and Enum#each and in the compiler (num_members in TopLevelVisitor)

In order to keep that we would need to go with the option of @[Flags(all: false)] or settle that All/None will be constants like the others and might be less fun to use because the All and None will be members.

If we allow @[Flags(all: false)] we can also allow @[Flags(all: ALL)] to allow custom names btw.

I'm not voting for anyone yet, just dumping thoughts.

[asterite]

@bcardiff I don't think we need @[Flags(all, none, whatever)]. Instead of having none and all has members we can have class methods, so SomeEnum.none and SomeEnum.all, and that's easy to compute (and defined in Crystal!). That is, if we really want those. For now, I'd like to remove them, with no replacement (if people want them they can define them or define class methods as suggested).

[bew]

@asterite are thinking about something like this?

@[Flags]
enum Foo
  A
  B
  MyAll = all

  def self.all
    # generated with macros.. 
    # but how to detect that
    # MyAll should be skipped? 
    A | B
  end
end

p Foo::MyAll

(see question in comment)

[asterite]

@bew No, I'm thinking about this:

@[Flags]
enum Foo
  A
  B
  All = A | B
end

[bew]

So if I have an enum with 20 fields, how would it look like from a user pov?

@[Flags]
enum Foo
  A
  B
  #...
  S
  T

  # Do I need to write it myself? (I hope not..)
  AtoT = A | B | ... | S | T
end

[asterite]

Yes, like that. How often is that needed? And how many times are you going to write that one All declaration? Just once. So it's not a big deal. It's simpler and the constants are visible, nothing is implicit.

[ysbaddaden]

Also: do you really need ALL that often that it must exist in the first place?

[bew]

To me it's not about how often I need it, because I don't need flags very often. But I'm not representative of the users of this feature.

A simple use case would be to compute a mask of all possible values. Now I agree it's not common to use it as a enum member, but I think we should at least have a builtin method (on Enum) to get a All-like value.

Like:

struct Enum
  def self.all
     # logical OR of all members 
  end
end

[ysbaddaden]

Then it's back to square one: the method name may conflict with the actual ALL.

[bew]

How so? (what 'actual ALL' are you talking about?)

The only 'bad' think I see is that you'd have a .all class method for non flags enum too..

[asterite]

But I'm not representative of the users of this feature.

Who's a representative user of this feature? Please, reply here. If we don't get enough replies then I say we should remove that "feature".

[jhass]

Let's remove them. None is trivially specified manually.

If there's outcry I would vote to move the functionality to an auto-generated class method, something like SomeFlagEnum.all_flags (and then maybe SomFlagEnum.no_flags for parity). There's actually no good reason to have it as a member and this should avoid any conflicts with them.

[straight-shoota]

There seem to be no strict objections against removing the default flags, so let's move forward with this.

[asterite]

Cool! I can work on that, it should be pretty easy to remove them. (but for 0.33.0, of course)

[asterite]

It turns out this can't be implemented in one go. We first need to remove the errors that happen if you try to redefine None and All in one release, and in a subsequent release redefine those while also removing their autogeneration.

[asterite]

I won't be able to work on this, but if someone wants to work on the first part, PRs are very welcome! 🙏

[oprypin]

I am currently not on board with fully removing None and All. Obviously we can't remove them before 2.0 anyway. All actions that we do shouldn't assume that we'll eventually remove these and instead act in the best interest within the current limits.

I only support this PR for the moment:

• 10497

[straight-shoota]

Yeah, currently #10497 is the only piece tentatively agreed upon and up for approval. It's a preliminary step for eventual removal, but also useful on its own.

Any further action is due to discussion. And of course we can't do a breaking change before 2.0. There are however other short term options, for example configuration via a parameter on the Flag annotation.