Fix issue 17190: Fix isNumeric conflict.

[jmdavis]

Templatizing std.string.isNumeric made it conflict with
std.traits.isNumeric and does not overload well given that one is a
function, and the other is an eponymous template.

This renames std.string.isNumeric to isNumber and adds an alias named
isNumeric that we will deprecate after it's been out a release. That way
we can move away from having std.traits and std.string conflict so badly
with regards to isNumeric.

This doesn't outright fix the regression, since we're still stuck with the conflict for a while, but I think that it's the cleanest way to deal with the problem.

I'm adding the @andralex label since renaming a symbol like this is essentially adding a new symbol.

[dlang-bot]

Thanks for your pull request, @jmdavis!

Bugzilla references

Auto-close	Bugzilla	Description
✓	17190	[REG2.072] isNumeric!string conflict std.traits std.string

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

• Regression or critical bug fixes should always target the stable branch. Learn more about rebasing to stable or the D release process.

[PetarKirov]

According to Circle CI:

std/string.d(5814:1)[warn]: A unittest should be annotated with at least @safe or @system

[jmdavis]

Okay. That should be fixed now.

[PetarKirov]

LGTM, though leaving the decision to @andralex

[CyberShadow]

This rename does not make me happy. isNumeric is an established name across many programming languages.

I think it would be less disruptive to rename the std.traits template instead, however, I'm not sure if it's even better to rename anything. I think doing so is going to break a lot more code than would be affected by this conflict.

[CyberShadow]

I think it would be less disruptive to rename the std.traits template instead,

Never mind that. I'm seeing a lot of hits for both in https://github.com/search?l=&q=isNumeric++language%3AD&ref=advsearch&type=Code&utf8=%E2%9C%93, though the std.traits template seems more common. Still, there are too many uses of either.

We already have a similar issue with std.file.write and std.stdio.write.

[jmdavis]

std.string.isNumeric appears to only be used in one place in all of Phobos outside of std.string, and that's in std.socket. I have no idea how much either is used in the wild. Personally, I'm sure that I've never used std.string.isNumeric, because any case where I might use it, I'd either just use std.conv.to and let it throw on failure, or I'd be looking for it specifically to be all digits, and I'd just be using something like all!isDigit. I don't recall whether I've ever had a need for std.traits.isNumeric.

But std.traits.isNumeric and std.string.isNumeric do distinctly different things and are used in completely different ways and yet they end up conflicting. In general, I think that it's an incredibly bad idea to have conflicts when one is a function and the other is an eponymous template. Under other circumstances, they wouldn't conflict, because they aren't used the same way, but because one's an eponymous template and the other is a templated function, we end up with a conflict. I think that it's far better that we just rename one of them.

Now, if Andrei wants to decide that this isn't worth worrying about, then he can close the PR and the bug, but I think that in this case, we're better off not having a conflict.

As for the name, I think that using isNumeric for checking the type makes more sense than for checking the value of a string, and whereas isNumber works for the checking the string, I don't think that it makes anywhere near as much sense for checking the type. So, if one of them is going to be renamed to isNumber, I think that it should be the one in std.string. But if someone has a better name, fine. I'd prefer not to bikeshed it though, and I really don't see a problem with this particular renaming choice if we're going to rename one of them. The first question though is whether Andrei agrees with renaming either, since if he doesn't, then the whole issue is moot.

[andralex]

First off, the claim is overdone. This is not a conflict. The same name can and will exist in D in different modules comfortably. This is not C, TeX, etc. where people need to make sure the names in their APIs are unique in the world. We should allow such names to coexist in the standard library.

Second, std.string.isNumeric is a textbook example of bad API design. It does a lot of work to return just a bool with no extra information (was that an integral? floating point? signed or not? and what's the actual number?); so then the caller needs to do extra work once again to perform the conversion. The misguided inspiration comes from interpreted languages where efficiency is no concern and that only have one numeric type.

So if I'd approve anything at all it's not going to be a witch hunt for identical names in different modules, though I do sympathize with the loss of comfort at the call site. I would approve deprecating std.string.isNumeric for complete removal.

[jmdavis]

It does make some sense to check whether something is a valid number before converting it given how expensive exceptions can be, but in that case, an API that returned a Nullable rather than throwing an exception would make more sense than what isNumeric does - especially when isNumeric doesn't even do a good job of avoiding an exception before calling to, because you don't know what type of number it is.

I have no problem with deprecating std.string.isNumeric (I've certainly never liked it), though I'm sure that someone will complain. This whole thing came up because templatizing isNumeric made it conflict where it didn't before, and it didn't overload properly, because std.traits.isNumeric isn't a function. I certainly don't think that we need to avoid all naming conflicts (the module system does provide us ways to deal with it unlike in C), but using the same name for an eponymous template and a function results in greater conflict that often happens with just functions, and I think that we should try to avoid that.

Thinking about this, I suspect that the best thing to do overall would be to add something to std.conv that was similar to to but which return a Nullable rather than throwing an exception (and for all conversions, not just integers), but I expect that that will be a good amount of work. Still, I think that that's probably the best way to solve the issue of needing to check whether a conversion works without having to catch an exception, and if that were solved, then std.string.isNumeric would be redundant.

[wilzbach]

an API that returned a Nullable rather than throwing an exception would make more sense than what isNumeric does - especially when isNumeric doesn't even do a good job of avoiding an exception before calling to, because you don't know what type of number it is.

I like this direction - offering APIs that don't throw, but leave the explicit decision to user code whether he uses get or get(defaultValue) is very popular in other languages.

Thinking about this, I suspect that the best thing to do overall would be to add something to std.conv that was similar to to but which return a Nullable rather than throwing an exception (and for all conversions, not just integers), but I expect that that will be a good amount of work. Still, I think that that's probably the best way to solve the issue of needing to check whether a conversion works without having to catch an exception, and if that were solved, then std.string.isNumeric would be redundant.

FWIW other languages have the concept of e.g. toMaybe which allows them to use one function with three ways of handling an error.

• exception (default behavior.)
• nullable (toMaybe)
• default value (withDefault)

An example from Elm - note that toInt is similar to std.conv.to (i.e. it throws an exception by default):

String.toInt "5" |> Result.toMaybe |> Maybe.withDefault 0

I'm not sure yet what the best API approach in D for this would be, but out of my gut it's probably the reverse: let the main API return a Nullable.

[PetarKirov]

+1 I occasionally miss expected<T, Error> from https://channel9.msdn.com/Shows/Going+Deep/C-and-Beyond-2012-Andrei-Alexandrescu-Systematic-Error-Handling-in-C
Which is also similar to Rust's Result type:
https://doc.rust-lang.org/std/result/

[MetaLang]

@jmdavis Looks like this can be closed then?

[andralex]

So this discussion doesn't go to naught, how about putting this function on the way to obsolescence? Marginalize in the documentation with a recommendation for alternative approaches using parse, then deprecate, than remove.