overloadable enums no longer experimental

[metagn]

currently includes #20126 as this is required for packages to pass was merged

[Araq]

Please rebase.

[metagn]

Rebased, CI failing because gitlab is down

[github-actions]

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from 5ebd124

Hint: mm: orc; threads: on; opt: speed; options: -d:release
164015 lines; 14.385s; 842.496MiB peakmem

[Menduist]

This PR caused a regression, there is apparently an issue with overloadable enums:
https://github.com/status-im/nim-libp2p/runs/8222220557?check_suite_focus=true

Current repro (using nim devel):

git clone git@github.com:status-im/nim-libp2p.git
cd nim-libp2p
nimble install -dy
nim c libp2p/multiaddress.nim

(ofc the issue was not directly caused by this PR, this PR just made it more apparent)

I'll try to make a proper reproduction & open issue in a few days, if no one took a look in the meantime

[metagn]

The error message given there is the same as given here:

{.experimental: "overloadableEnums".}

type A = enum foo
type B = enum foo

let x = {foo} # ordinal type expected

This is supposed to be an error but the message is terrible, it is trying to say "ambiguous type". If the case in that code is the same as this one (I am not familiar with the codebase and I would have to go through every import to know), there is a universal workaround of qualifying, i.e. doing MAKind.Length. Otherwise, there may be an actual issue.

[Menduist]

Thanks for the pointer!

Line which cause the crash:

elif proto.kind in {Length, Path}:

Indeed, there is already a Length symbol somewhere else. Switching to elif proto.kind in {Path, Length} fixes the issue (Length get inferred correctly).

Does this mean we have to live in fear that one of the symbol we import starts to use the same name as us, and forces us to rename our field, or always specify MAKind.Length to be safe? How can this situation is worst now with overloadableEnums than without it?

[metagn]

The only saving grace there without overloadableEnums was that MAKind was defined in the same module and not an import. If it was defined in an import, i.e. if two imports had the same name for a enum field, then either it would give the same ambiguity error, or it would pick the one from the last import, which is probably bad behavior.

You should never have to rename the field, the obvious solution to me is qualifying it (add MAKind.) where type inference doesn't work. If it was case proto.kind it would have been inferred.

By far the biggest issue there IMO, and potentially in other places, is the error message saying basically nothing. Even if it said ordinal type expected but got MAKind | OtherEnum I feel like it would be workable.

[Menduist]

Unfortunately, it doesn't work either with case:

type A = enum foo, bar
type B = enum foo

case bar:
#of foo: echo "ok" # works
of {foo, bar}: echo "ok" # doesn't
else: echo "too bad"

So this seems like an issue, where bar == foo is inferred correctly, but not bar in {foo}. Maybe it cannot be inferred though, in which case, from now on, every time we use a set, we have to specify the first one to be safe: {A.foo, bar}.
Maybe giving priority to the enum defined in the current module is the lesser evil, at least it prevents imported modules to break your compilation out of the blue

(and indeed, better error message)

[narimiran]

This PR caused a regression, there is apparently an issue with overloadable enums: https://github.com/status-im/nim-libp2p/runs/8222220557?check_suite_focus=true

I think this CI fail is also connected to the changes in this PR:
https://github.com/status-im/nim-chronos/actions/runs/3019029571/jobs/4854535897#step:12:92

The compiler is confusing HANDLE defined here with Handle defined here - as you can see, a workaround for the problem has already been commited.