units: Prevent casting pub enums as ints#4127
Conversation
|
I didn't go searching for the name you suggested already @Kixunil, happy to change the one here to it if you post it. |
|
🚨 API BREAKING CHANGE DETECTED To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section. |
3b19558 to
39049c3
Compare
|
🚨 API BREAKING CHANGE DETECTED To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section. |
|
The annoying and perhaps confusing part is that LSP (rust-analyzer) still offers it for completions and other assistance. fn x(d: Denomination) {
match d {
|
}
}Fill match arms: fn x(d: Denomination) {
match d {
Denomination::Bitcoin => todo!(),
Denomination::CentiBitcoin => todo!(),
Denomination::MilliBitcoin => todo!(),
Denomination::MicroBitcoin => todo!(),
Denomination::Bit => todo!(),
Denomination::Satoshi => todo!(),
Denomination::PreventCastingVariantAsInt(infallible) => todo!(),
}
}Completion:
|
|
Damn, yes that is ugly. |
units/src/amount/mod.rs
Outdated
| /// satoshi (1 BTC = 100,000,000 satoshi). | ||
| Satoshi, | ||
| #[doc(hidden)] | ||
| PreventCastingVariantAsInt(Infallible), |
There was a problem hiding this comment.
By convention (and perhaps to silence lints) such hidden enum variants typically start with underscore
units/src/amount/mod.rs
Outdated
| Denomination::MicroBitcoin => -2, | ||
| Denomination::Bit => -2, | ||
| Denomination::Satoshi => 0, | ||
| Denomination::PreventCastingVariantAsInt(_) => i8::MIN, |
There was a problem hiding this comment.
No, this has to return match {} to convey the impossibility.
units/src/amount/mod.rs
Outdated
| Denomination::MicroBitcoin => "uBTC", | ||
| Denomination::Bit => "bits", | ||
| Denomination::Satoshi => "satoshi", | ||
| Denomination::PreventCastingVariantAsInt(_) => "no-use", |
|
@jirijakes if you're experiencing it in a downstream crate that's a bug in RA worth reporting. Anyway, I do think this is a hack and ideally the language would have a first class support for this. But it's also possible that underscore will fix it. Either way, it's better to have an extra variant which obviously shouldn't be used (Maybe we should have "DoNotUse" in the name) than having a silent breakage like the last time. |
No, it it was within rust-bitcoin. And really, in downstream, the hidden variant is suppressed. No underscore even needed. Interesting. Is it because of doc hidden? |
|
Great!
Yes, that should be the reason. |
c0a1150 to
5645b6b
Compare
|
🚨 API BREAKING CHANGE DETECTED To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section. |
1 similar comment
|
🚨 API BREAKING CHANGE DETECTED To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section. |
units/src/amount/mod.rs
Outdated
| Satoshi, | ||
| /// Stops users from casting this enum to an integer. | ||
| #[doc(hidden)] | ||
| _Never(Infallible), |
There was a problem hiding this comment.
The previous name was better, just the underscore was something worth adding.
There was a problem hiding this comment.
I thought that because it may show up in rust-analyzer that a really long name was annoying and _Never would be less so?
There was a problem hiding this comment.
Why would it be annoying? But anyway, it only shows up in our crate, not downstream. If you want something terse that conveys the intent name it _DoNotUse (and explicitly tell in comment that this may get removed if one day Rust supports disabling casts natively).
units/src/amount/mod.rs
Outdated
| Denomination::MicroBitcoin => -2, | ||
| Denomination::Bit => -2, | ||
| Denomination::Satoshi => 0, | ||
| Denomination::_Never(_) => unreachable!("cannot create this variant"), |
There was a problem hiding this comment.
This can be statically proven and it should be.
There was a problem hiding this comment.
I couldn't get the match {} thing to build, I should have said that when pushing.
There was a problem hiding this comment.
That's impossible, we've used that idiom multiple times in the crate. What's the error message?
There was a problem hiding this comment.
I believe the syntax you want is
...
Denomination::_Never(x) => match x {}
...
A public enum with simple variants gets an automatic integer variant that can be cast by library consumers. This puts a unnecessary maintenance burden upon us because we cannot then add variants in the middle of others. Add a hidden variant to the single public non-error enum in `units`.
|
🚨 API BREAKING CHANGE DETECTED To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section. |
A public enum with simple variants gets an automatic integer variant that can be cast by library consumers. This puts a unnecessary maintenance burden upon us because we cannot then add variants in the middle of others.
Add a hidden variant to the single public non-error enum in
units.