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

implement RFCs/294 ; disallow enum <=> enum conversion #16351

Merged
merged 10 commits into from
Apr 3, 2021

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Dec 14, 2020

CI failures caused by this:

only those 2:

  • nimgame2
    /Users/runner/work/Nim/Nim/pkgstemp/nimgame2/nimgame2/texturegraphic.nim(241, 41) Error: type mismatch: got but expected 'RendererFlip = enum'
  • nimquery
    /Users/runner/work/Nim/Nim/pkgstemp/nimquery/nimquery.nim(915, 68) Error: type mismatch: got but expected 'Combinator = enum'

these packages can be fixed upstream, or testament can add logic to pass flags (eg -d:nimLegacyConvEnumEnum or everytime such kind of issue shows up) to tests that need custom flags; this can be done by overriding XDG_CONFIG_HOME for those tests, pointing to a custom config.nims, so that these flags apply recursively through nim invocations via exec. (EDIT: see nim-lang/RFCs#336)

future work

@timotheecour timotheecour changed the title fix https://github.com/nim-lang/RFCs/issues/294 ; disallow enum <=> enum conversion fix RFCs/294 ; disallow enum <=> enum conversion Dec 14, 2020
@timotheecour timotheecour changed the title fix RFCs/294 ; disallow enum <=> enum conversion implement RFCs/294 ; disallow enum <=> enum conversion Dec 14, 2020
@cooldome
Copy link
Member

cooldome commented Dec 14, 2020

cast from one enum to another should work, please test. IMO, cast should work even if enums have different byte size.

Should changelog recommend cast instead of ord?

@timotheecour
Copy link
Member Author

timotheecour commented Dec 14, 2020

cast from one enum to another should work, please test. IMO, cast should work even if enums have different byte size.

done

Should changelog recommend cast instead of ord?

no, cast is overkill in this use case, and won't help catch bugs eg:
cast[Foo](bar) would happily cast any type to any type. ord should have 0 overhead.

changelog.md Outdated Show resolved Hide resolved
@timotheecour
Copy link
Member Author

PTAL

@timotheecour
Copy link
Member Author

@Araq PTAL (fixed bitrot)

@timotheecour
Copy link
Member Author

ping @Araq I had to rebase again to fix bitrot

@Araq Araq merged commit 270964c into nim-lang:devel Apr 3, 2021
@timotheecour timotheecour deleted the pr_fix_294_rfc_enum_conv branch April 3, 2021 07:41
@alaviss
Copy link
Collaborator

alaviss commented May 3, 2021

Can we have a migration error message for this? This will be extremely confusing for anyone not tracking devel and not having their code in important_packages (I have to debug one such codebase recently).

I'm a bit worried how we keep on introducing tiny breaking changes in the language spec between versions.

@timotheecour
Copy link
Member Author

that's what -d:nimLegacyConvEnumEnum + the changelog section Changes affecting backward compatibility is for, those changes are made for a reason.

The changelog clearly mentions this change. In any case, I've just sent out #17928 for this case, but in other cases, it may not always be practical to do this (see that section in the changelog). Users who upgrade should read that section.

@Clyybber
Copy link
Contributor

Clyybber commented May 3, 2021

I'm not sure anymore wether this change is a good idea, the user is already explicit when writing foo.Bar, having to add .ord in there is unneccessary syntax salt.
I was originally positive about this change because two enums might not have any meaningful relation and this would make conversions between them more explicit, but thinking more about it I don't think it fits well with the rest of the language; conversions from distinct T to T or vice versa don't have this extra explicitness either, even though the value of a distinct T might not have any meaningful relation to it's value as T.

@timotheecour
Copy link
Member Author

(let's followup in nim-lang/RFCs#294 (comment) instead to avoid duplicating the discussion)

@timotheecour timotheecour mentioned this pull request Jul 10, 2021
PMunch pushed a commit to PMunch/Nim that referenced this pull request Mar 28, 2022
* fix nim-lang/RFCs#294 ; disallow enum <=> enum conversion
* fix the runnableExamples that was the instigator of this RFC
* legacy -d:nimLegacyConvEnumEnum
* use -d:nimLegacyConvEnumEnum in important_package nimgame2
* add test for enum cast
* improve changelog
* add changelog: Changes affecting backward compatibility
* cleanup changelog
* fix changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

disallow enum to enum conversion (require passing through ord)
6 participants