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

From implementations for some WKTs break their contracts #690

Open
panhania opened this issue Nov 23, 2023 · 2 comments
Open

From implementations for some WKTs break their contracts #690

panhania opened this issue Nov 23, 2023 · 2 comments

Comments

@panhania
Copy link

From implementations for Timestamp and Duration well-known types that convert to standard library std::time::SystemTime and std::time::Duration state that they panic when given values out of the allowed range. However, this is against the contract that From imposes on implementations:

Note: This trait must not fail. The From trait is intended for perfect conversions. If the conversion can fail or is not perfect, use TryFrom.

This contract breakage is especially severe in the case of protobuf crate as Protocol Buffers messages usually contain data from "the outside" and should not be trusted. If the application relies on the fact that From conversion cannot fail (as I usually do), it means that a malformed message can easily bring it down.

@stepancheg
Copy link
Owner

Right, TryFrom would be more appropriate.

We cannot remove From, but we can deprecate it.

@panhania
Copy link
Author

panhania commented Dec 1, 2023

We cannot remove From, but we can deprecate it.

If by "deprecate" we mean "add the #[deprecate] attribute", it won't work: rust-lang/rust#39935. Deprecation is possible only in the "note in the docs" sense—but then it is not that much better than information about the panic, although it paves the way for removing it in the next major release.

Alternatively, the implementations could saturate—instead of panicking (because of the overflow) they could return the largest Duration and SystemTime values possible. The issue here is that there is no SystemTime::MAX defined in the standard library (see rust-lang/rust#71224 for related discussion), although there is Duration::MAX.

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

No branches or pull requests

2 participants