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

closed ambiguous enum defaults to first overload #20457

Merged
merged 4 commits into from
Oct 1, 2022

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Sep 29, 2022

In the case where a closed enum field symchoice cannot collapse to a type, assume it has the type of the first overload in the symchoice. From what I understand this is also the closest overload in scope.

Unfortunately this also applies to enum types in the same scope, as disallowing this would require iterating over the overloads again. But this was not possible without overloadableEnums, and the type system should point out issues.

This allows some code that didn't use overloadableEnums to work. Specifically, this should make code that broke with #20298, namely the regression in the comments #20298 (comment) and later reported on the forum https://forum.nim-lang.org/t/9474#62224 work.

This gives a warning that can be turned into an error with --warningAsError:AmbiguousEnum.

@metagn metagn marked this pull request as draft September 29, 2022 17:13
@Araq
Copy link
Member

Araq commented Sep 29, 2022

That's a great solution as it mimics how proc overloading works, roughly.

@metagn metagn force-pushed the collapse-overloadable-enum branch from 79768a5 to 3c98100 Compare September 29, 2022 17:48
@metagn metagn marked this pull request as ready for review September 29, 2022 18:43
@metagn
Copy link
Collaborator Author

metagn commented Sep 29, 2022

Added warning for when this happens, can remove/turn to a hint if needed.

@Menduist
Copy link
Contributor

Menduist commented Sep 30, 2022

Thanks a lot @metagn for going through with this :)

If this is the same behavior as proc, but proc do it silently, maybe a hint / nothing makes more sense?

@metagn
Copy link
Collaborator Author

metagn commented Sep 30, 2022

This is the difference with procs:

type Foo = enum abc
type Bar = enum abc

assert abc is Bar
# a.nim
type Foo* = enum abc

# b.nim
type Bar* = enum abc

# c.nim
import a, b

assert abc is Bar

I can't think of an efficient way to deal with this, so I added the warning in case it's undesired. I think the lowest thing we can do above saying nothing is turning it into a style check hint, or having it completely off by default. But I have no idea what would be best because of how uncommon this is. For now I will turn it to a hint that is off by default.

@metagn metagn force-pushed the collapse-overloadable-enum branch from deceabd to 7857be8 Compare September 30, 2022 15:00
@Araq Araq merged commit cfff454 into nim-lang:devel Oct 1, 2022
@Araq
Copy link
Member

Araq commented Oct 1, 2022

Needs to be documented in the manual too.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 1, 2022

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

Hint: mm: orc; threads: on; opt: speed; options: -d:release
164381 lines; 16.875s; 665.777MiB peakmem

metagn added a commit to metagn/Nim that referenced this pull request Nov 26, 2022
Araq pushed a commit that referenced this pull request Dec 1, 2022
* better procvar ambiguity errors, clean up after #20457

fixes #6359, fixes #13849

* only trigger on closedsymchoice again

* new approach

* add manual entry for ambiguous enums too

* add indent [skip ci]

* move to proc
survivorm pushed a commit to survivorm/Nim that referenced this pull request Feb 28, 2023
…ang#20932)

* better procvar ambiguity errors, clean up after nim-lang#20457

fixes nim-lang#6359, fixes nim-lang#13849

* only trigger on closedsymchoice again

* new approach

* add manual entry for ambiguous enums too

* add indent [skip ci]

* move to proc
capocasa pushed a commit to capocasa/Nim that referenced this pull request Mar 31, 2023
* closed ambiguous enum defaults to first overload

* add warning

* turn to hint

* work around config
capocasa pushed a commit to capocasa/Nim that referenced this pull request Mar 31, 2023
…ang#20932)

* better procvar ambiguity errors, clean up after nim-lang#20457

fixes nim-lang#6359, fixes nim-lang#13849

* only trigger on closedsymchoice again

* new approach

* add manual entry for ambiguous enums too

* add indent [skip ci]

* move to proc
bung87 pushed a commit to bung87/Nim that referenced this pull request Jul 29, 2023
…ang#20932)

* better procvar ambiguity errors, clean up after nim-lang#20457

fixes nim-lang#6359, fixes nim-lang#13849

* only trigger on closedsymchoice again

* new approach

* add manual entry for ambiguous enums too

* add indent [skip ci]

* move to proc
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.

3 participants