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

Would removing (or moving) Num::from_str_radix() be ever considered? #192

Closed
aldanor opened this issue Oct 15, 2020 · 6 comments · Fixed by #193
Closed

Would removing (or moving) Num::from_str_radix() be ever considered? #192

aldanor opened this issue Oct 15, 2020 · 6 comments · Fixed by #193

Comments

@aldanor
Copy link

aldanor commented Oct 15, 2020

Related: #178 (comment)

Quoting num-traits (this comment itself is super-old),

// FIXME: The standard library from_str_radix on floats was deprecated, so we're stuck
// with this implementation ourselves until we want to make a breaking change.
// (would have to drop it from `Num` though)

The problem: Num requires implementing from_str_radix and tons of other traits in this crate require Num. So, e.g., to be able to make use Signed, you are blocked by having to implement from_str_radix, whether you want it or not. Implementing it properly for a user-defined number type (e.g. a fixed decimal) is a non-trivial task, especially given that it won't actually be used - in most cases it would be simply a waste of time.

This method is not used anywhere in the crate and it's likely it's not used by anyone anywhere at all, simply because better options exist, and the provided implementation suffers from rounding errors.

If from_str_radix() is dropped from Num:

  • Num becomes implementable for various user-defined number types; as of right now, it's inaccessible because of that method
  • Num no longer has any methods or associated types whatsoever, it becomes a neat marker trait with some supertraits
  • As a consequence, various traits like Signed become unlocked for user-defined number types as well

Related thread on internals re: why std::num::from_str_radix is (was) bad. If FromStrRadix was dropped from standard library, why can't it be dropped from here as well?

Finally, if the current implementation has to be retained somewhere, one could move it into a new FromStrRadix trait.

@cuviper
Copy link
Member

cuviper commented Oct 15, 2020

If FromStrRadix was dropped from standard library, why can't it be dropped from here as well?

That's the messy history of the num crate -- these methods were still #[unstable] in std, so they were deprecated in 1.4 and removed in 1.7. But when copied to num, we don't have such stability mechanisms, so they became de-facto stabilized with everything else here.

I don't intend to make breaking changes to num-traits, since the traits have a lot of cross-crate API interaction for our users. I'm thinking I might finally raise to 1.0 with a compatible semver trick to make that position clear.

On Num::from_str_radix in particular, I think we could relax the radix requirements. It's barely specified now, "Convert from a string and radix <= 36," but the integer implementations also enforce the lower bound, assert!(radix >= 2 && radix <= 36), so they panic otherwise. I suggest we codify that, but also allow that implementations may just return Err for any other unsupported radix. Thus a new type could choose to only support radix = 10 like a normal parse, or they might even return an unconditional Err.

@aldanor
Copy link
Author

aldanor commented Oct 15, 2020

That might be a decent middle ground (somewhat ugly still since we're stuck with a useless trait method, but allowing to avoid some breakage I guess).

If that's the preferred way, then why would int implementations panic and not return Err instead?

Also (just thinking aloud), if returning an unconditional Err is now completely valid, why wouldn't it be a default implementation with some default assoc type as well?

@cuviper
Copy link
Member

cuviper commented Oct 15, 2020

why would int implementations panic and not return Err instead?

We're just forwarding to the inherent methods from the standard library. We could pre-check the radix for an error ourselves, but that still has to fit into a ParseIntError. There's also no direct way to construct that type, so we'd have to fake-parse some other string to get the error we want.

Also (just thinking aloud), if returning an unconditional Err is now completely valid, why wouldn't it be a default implementation with some default assoc type as well?

That would be nice, but that feature is still incomplete and unstable: rust-lang/rust#29661

@tspiteri
Copy link
Contributor

I suggest we codify that, but also allow that implementations may just return Err for any other unsupported radix.

This would allow me to implement Num for fixed-point numbers in the fixed crate. Currently I cannot because I don't want to implement a method that does not fulfil the trait's specification, but updating the documentation to explicitly allow returning Err on unsupported radices will solve it in a backwards-compatible way.

@aldanor
Copy link
Author

aldanor commented Oct 18, 2020

@cuviper IIUC, this doc clarification/update wouldn't be considered a breaking change and thus can be done prior to 1.0 bump?

@cuviper
Copy link
Member

cuviper commented Oct 19, 2020

I think it's not a breaking change, but I suppose that's debatable. I would not suggest that any existing Num implementations should reduce their radix abilities, or at least that's a breaking change consideration for that type. I think it's OK that new Num implementations may be limited, just as new implementations of Add may have unique overflow limitations, for example.

But my earlier point was that 1.0 would not have any breaking changes either, just reflect the unchanging status quo.

@cuviper cuviper linked a pull request Oct 29, 2020 that will close this issue
@bors bors bot closed this as completed in #193 Oct 29, 2020
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 a pull request may close this issue.

3 participants