Skip to content

Make Number non-exhaustive#564

Merged
juntyr merged 5 commits into
ron-rs:masterfrom
juntyr:non-exhaustive-number
Mar 22, 2025
Merged

Make Number non-exhaustive#564
juntyr merged 5 commits into
ron-rs:masterfrom
juntyr:non-exhaustive-number

Conversation

@juntyr

@juntyr juntyr commented Mar 20, 2025

Copy link
Copy Markdown
Member

Fixes #563

  • I've included my change in CHANGELOG.md

@juntyr

juntyr commented Mar 20, 2025

Copy link
Copy Markdown
Member Author

@epage What are your thoughts on this compromise?

Comment thread src/value/number.rs
Comment on lines +51 to +53
#[cfg(not(doc))]
#[allow(private_interfaces)]
__NonExhaustive(private::Never),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure whats going on but this didn't make the enum exhaustive. I wonder if the compiler recognized that this variant cannot be constructed so therefore it doesn't need to be matched?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uff, maybe this comes from some of the newer uninhabited type matching ...

Since private::Never is private, we can make the type inhabited but not constructible. Would changing this be a breaking change?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, people can exhaustively match on Number. Making it actually non-exhaustive would then be a breaking change.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We’ll definitely need a compile-fail test then to ensure there are no more slip ups - I’m sorry that the first fix didn’t catch this.

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.

Enabling integer128 feature will break code

2 participants