[Proposal] Introduce is_positive/1 and is_negative/1 macros

[Isaac Whitfield]

Hey all!

Sorry if this has been broached previously, but searching didn't show anything (either on the list or on Google).

I recently realised that I write a lot of `> 0` guard clauses, combined with the fact that they're either passing `is_integer/1` or `is_float/1`. I know that it's an easy thing to write yourself but:

def my_function(value) when is_integer(value) and value > 0 do
  # stuff
end

def my_function(value) when is_positive(value) do
  # stuff
end

and based on the frequency I've used it, seems like a reasonable request for stdlib inclusion.

So, I would like to propose the following (and feel free to shoot me down!). Of course each call would just be a macro:

Integer.is_positive/1 - is_integer(val) and val > 0
Integer.is_negative/1 - is_integer(val) and val < 0
Float.is_positive/1   - is_float(val) and val > 0
Float.is_negative/1   - is_float(val) and val < 0
Kernel.is_positive/1  - is_number(val) and val > 0
Kernel.is_negative/1  - is_number(val) and val < 0

And coincidentally, I'm assuming that it's better to perform the type check first, I guess I'd appreciate if someone could confirm that? Also goes without saying that I'd be happy to impl this if we decide to adopt.

Thanks for your consideration!

IW

[Andrea Leopardi]

I lean on a soft no because we’d have to introduce six new functions as you said, plus is_non_negative likely as well. To me, the comparison operator is more clear and if you use it many many times, than nothing wrong with writing down your own macros as you mentioned :).

--
You received this message because you are subscribed to the Google Groups "elixir-lang-core" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elixir-lang-co...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/1d9f2250-9783-4af8-b2bb-9c0379f3b122%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--

Andrea Leopardi

an.le...@gmail.com

[Isaac Whitfield]

Hi Andrea!

Thank you for your quick feedback! What you said about adding six functions is true, but they're very straightforward macros so it shouldn't be too painful. I'm not entirely sure I agree about the operators being more clear; people new to Elixir could simply put > 0 in a guard and just assume there's a type check there (even for those not new, it's easy to forget). Seeing is_positive/1 and is_negative/1 in code examples would reinforce the fact that a basic > is not sufficient.

I think this use case is common enough that it warrants being in stdlib. Integer has is_odd/1 and is_even/1 and I would imagine that is_positive/1 and is_negative/1 would be used at least as much (although maybe I'm totally wrong :D). An even more fitting example is the macro for is_nil/1 when that's a single conditional in a guard and really doesn't save much.

Thanks for your feedback!

IW

[Isaac Whitfield]

Pure coincidence but whilst browsing the Timex library, they also had to roll their own - so it can't be just me! https://github.com/bitwalker/timex/blob/c86a96af43cb0052249a4899d988aaa4f665982b/lib/timex/macros.ex

[Andrew Timberlake]

To chime in, I’ve been caught out using value > 0 without checking I was dealing with an integer first as well.

Andrew

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/5cad86c6-4e89-485b-9db9-6a4140c13a63%40googlegroups.com.

[Paul Schoenfelder]

To be clear, those macros are used correctly within Timex, but that pattern is not safe in general, as the arguments are expanded multiple times, if the expanded arguments perform side effects, you'll have a bad time. Since those macros are meant for internal use only in Timex, I wasn't worried about it, but I wouldn't use it as a good example - it's very much macro abuse ;)

Paul

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/5cad86c6-4e89-485b-9db9-6a4140c13a63%40googlegroups.com.

[Tallak Tveide]

I would say no to this one. I think using < is ok. No change necessary.

Elixir isnt statically typed, so at least for me, in general, i dont bother with is_number

[Christopher Keele]

I would say no to this one. I think using < is ok. No change necessary.

Elixir isnt statically typed, so at least for me, in general, i dont bother with is_number

As Issac points out, the comparison operators are not type-safe for this use-case, though—just using < is not ok, as everything is comparable with everything and "foo" < self() is perfectly valid. These guards aren't syntactic sugar, they make the numeric is_xxx check precisely because it is easy to forget that v > 0 =/= is_positive(0) and introduce subtle bugs into your program:

def order_eggs(count) do

  if count > 0 do

    EggIO.order!(count)

  else

    raise ArgumentError, "you must order a positive number of eggs"

  end

end

order_eggs(:asdf) #=>{:ok, %EggIO.Order{count: :asdf, status: :ordered}}

Or even:

def is_positive(value) when bar > 0, do: true

def is_positive(value) when bar <= 0, do: false

def is_positive(_), do: raise ArgumentError, "value is not a number"

foo(:asdf) #=> true

The weak typing of the comparison operators is useful when allowing any term to be a key in an ordered data set. However in guard contexts it's easy to forget, or assume that the comparison operators alone suffice for positive number checks, especially as all the other Kernel operators are strongly-typed:

"foo" ++ "bar" #=> ArgumentError

[] <> [] #=> ArgumentError

I think these guards would make a reasonable addition to core since they could help prevent bugs and minimize a potential source of surprise to newcomers. I know they would have spared me one confused afternoon of debugging, since even though I know better I still sometimes I space out, and even when I'm not thinking of type safety I like to use guards that read nicely, so I would have unwittingly been accidentally protected from this class of bug.

I lean on a soft no because we’d have to introduce six new functions as you said, plus is_non_negative likely as well

I think is_non_negative isn't necessary because it's common to match on the zero case separately, and it's trivial to check it alongside the guard to get the same effect: (eg is_positive(value) or value == 0)

Additionally we could choose between just the Kernel ones, or just the namespaced ones. I feel as if the Kernel ones are more likely to be used and therefore more likely to protect users from type-omissions bugs, so if I had to only pick a subset I'd add them.

To be clear, those macros are used correctly within Timex, but that pattern is not safe in general, as the arguments are expanded multiple times.

That's a good point—any PR should use the recently-merged defguard which was specifically implemented to handle this unquoting situation safely for you.

[Isaac Whitfield]

Hey guys,

Excellent examples by Christopher and the fact they came up as a reply to a misunderstanding on this thread shows exactly why I think they should be added. I also agree that even those of us familiar with Elixir have those moments where you totally forget and just fall back to using the comparison operators alone, so it would be better to reinforce the concept with macros in core. It's not necessary to have is_non_negative/1 because you can do "when not is_negative(val)". If it absolutely comes down to choosing, I'd say the Kernel ones would be the best due to 1) the import by default and 2) covering both numeric types (but as they're compile time, I really don't see why not to include all).

Side note; I'm totally unfamiliar with the flow for suggesting features here... what determines a "yes" vs a "no" (as this thread has both)? Does it just get PR'd and rejected at that point, or?

Thanks everyone for your feedback!!
IW

[Tallak Tveide]

I am still not convinced. Elixir is not a statically typed language. The guard you mention does add some checking, and I wouldnt mind seeing it in Elixir code, but there are so many of these kinds of bugs that still remain ‘unsolved’. The tool to deal with these kinds of bugs are unit tests and dialyzer.

[Norbert Melzer]

I'm not convinced by this idea as well. Why should we have special treatment for lesser/greater than zero, but not for an arbitrary number? What if I want to allow float AND integer, with the proposed guards it is not possible. 

Tallak Tveide <tal...@gmail.com> schrieb am Mi., 22. Nov. 2017 um 07:20 Uhr:

I am still not convinced. Elixir is not a statically typed language. The guard you mention does add some checking, and I wouldnt mind seeing it in Elixir code, but there are so many of these kinds of bugs that still remain ‘unsolved’. The tool to deal with these kinds of bugs are unit tests and dialyzer.

--
You received this message because you are subscribed to the Google Groups "elixir-lang-core" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elixir-lang-co...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/e7227105-994d-4416-b829-17466b11da00%40googlegroups.com.

[Isaac Whitfield]

Hey guys,

I am still not convinced. Elixir is not a statically typed language. The guard you mention does add some checking, and I wouldnt mind seeing it in Elixir code, but there are so many of these kinds of bugs that still remain ‘unsolved’. The tool to deal with these kinds of bugs are unit tests and dialyzer.

Sure, but saying "we'll never fix them all, so why even try?" is not a good argument. Saying that the solution is unit tests is true for every behaviour; moreover people who don't know this semantic wouldn't test for it.

I'm not convinced by this idea as well. Why should we have special treatment for lesser/greater than zero, but not for an arbitrary number?

That's a fair point, I just feel that is_positive/1 and is_negative/1 will be used in enough places that make it the common choice. I wish my GitHub search skills were up to scratch for this :)

What if I want to allow float AND integer, with the proposed guards it is not possible.

The proposed guards do make that possible; the ones imported from Kernel would allow both float and integer.

[Tallak Tveide]

Sure, but saying "we'll never fix them all, so why even try?" is not a good argument. Saying that the solution is unit tests is true for every behaviour; moreover people who don't know this semantic wouldn't test for it.

I'm arguing that I feel this direction is not what I would expect of Elixir, being a dynamically typed language. I would not trade "bloating the Kernel module" (sorry for not finding a nicer wording, I don't mean to be harsh) for this functionality.

[Isaac Whitfield]

I'm curious what you feel about is_odd/1, is_even/1 and is_nil/1?

[Andrea Leopardi]

is_nil/1 is harder to justify for me and I get that it could be implemented as "== nil". As for is_odd/1 and is_even/1, have a look at they implementation. They are implemented in a faster but more obscure way than the simple "rem() == 0" or "rem() == 1".

Andrea Leopardi

an.le...@gmail.com

--

You received this message because you are subscribed to the Google Groups "elixir-lang-core" group.

To unsubscribe from this group and stop receiving emails from it, send an email to elixir-lang-core+unsubscribe@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/09ce8113-eb6f-41cd-aa06-9885c79098e4%40googlegroups.com.

[José Valim]

The issue with is_positive and is_negative is that they do not really solve the problem. What if you want >= 0? Should we also add is_non_negative and is_non_positive? Or what if you want x >= 2, which is what you would use if you were implementing fibonacci?

José Valim

www.plataformatec.com.br

Founder and 

Director of R&D

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/CAM9Rf%2BJmDDSS3pnMkiPOthE7p6JT%3Dsq4Z__frjzQzH5KOpV8yw%40mail.gmail.com.

[Isaac Whitfield]

I don't believe we would need the is_non_ versions because you can just invert the positive case, right? Which also means that >= just becomes "when not is_negative(val)".

My only suggestion for your latter point is that it's a far more specific use case than checking positive/negative values, and I'm just aiming for the general case here. Would you be more in favour of an is_greater_than/2 style, or is that "too close" to the > operator at that point?

For what it's worth, I did some digging and already found a handful of places on GitHub that people rolled their own "is_positive" and most of them are assuming the type. Beyond that I can't check with much accuracy due to GitHub not allowing for ">" in a search. If you check for "when 0" there are also a few matches for some guard statements which also assume type. Obviously, there are false positives where the type can only be one thing, but it does show that it's not uncommon.

Thanks all!

On Wednesday, November 22, 2017 at 7:44:40 AM UTC-8, José Valim wrote:

The issue with is_positive and is_negative is that they do not really solve the problem. What if you want >= 0? Should we also add is_non_negative and is_non_positive? Or what if you want x >= 2, which is what you would use if you were implementing fibonacci?

José Valim

www.plataformatec.com.br

Founder and 

Director of R&D

On Wed, Nov 22, 2017 at 1:38 PM, Andrea Leopardi <an.le...@gmail.com> wrote:

is_nil/1 is harder to justify for me and I get that it could be implemented as "== nil". As for is_odd/1 and is_even/1, have a look at they implementation. They are implemented in a faster but more obscure way than the simple "rem() == 0" or "rem() == 1".

Andrea Leopardi

an.le...@gmail.com

On Wed, Nov 22, 2017 at 4:22 PM, Isaac Whitfield <i...@whitfin.io> wrote:

I'm curious what you feel about is_odd/1, is_even/1 and is_nil/1?

On Wednesday, November 22, 2017 at 7:20:35 AM UTC-8, Tallak Tveide wrote:

Sure, but saying "we'll never fix them all, so why even try?" is not a good argument. Saying that the solution is unit tests is true for every behaviour; moreover people who don't know this semantic wouldn't test for it.

I'm arguing that I feel this direction is not what I would expect of Elixir, being a dynamically typed language. I would not trade "bloating the Kernel module" (sorry for not finding a nicer wording, I don't mean to be harsh) for this functionality.

--
You received this message because you are subscribed to the Google Groups "elixir-lang-core" group.

To unsubscribe from this group and stop receiving emails from it, send an email to elixir-lang-co...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/09ce8113-eb6f-41cd-aa06-9885c79098e4%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "elixir-lang-core" group.

To unsubscribe from this group and stop receiving emails from it, send an email to elixir-lang-co...@googlegroups.com.

[José Valim]

> I don't believe we would need the is_non_ versions because you can just invert the positive case, right? Which also means that >= just becomes "when not is_negative(val)".

Right. Although I find "is_integer(x) and x >= 0" much clearer than "not Integer.is_negative(x)". The second is even longer.

José Valim

www.plataformatec.com.br

Founder and 

Director of R&D

To unsubscribe from this group and stop receiving emails from it, send an email to elixir-lang-core+unsubscribe@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/0054998a-61b9-483d-8d5f-c5b52701759b%40googlegroups.com.

[Norbert Melzer]

Wouldn't the outcome `not Integer.is_negative(:atom)` be even worse than what we have now?

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/0054998a-61b9-483d-8d5f-c5b52701759b%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "elixir-lang-core" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elixir-lang-co...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/CAGnRm4%2Bb3rNi1PvP-KyJA_yLGQmDAg4bLZ-_FQ6NYodkPeh%3DAQ%40mail.gmail.com.

[Isaac Whitfield]

The second is even longer.

 

If you care about the length, import it and it's shorter again ;)

Wouldn't the outcome `not Integer.is_negative(:atom)` be even worse than what we have now?

What do you mean by worse? If you take the face value of what the macro does "is this a negative integer?", then no, it's not worse.

[José Valim]

I knew I shouldn’t have mentioned about the length. :) the most important is that I find it more complex. Being longer just makes it worse.

--

[Xavier Noria]

Since this is a thread with different people expressing opinions, let me chime in :).

I also lean on not adding this to Elixir. Like Andrea, I find the guard pretty clear and concise. You read it and the meaning is direct (vs odd/even, since computing a reminder or doing bitwise stuff has one level of indirection, there encapsulation may pay off). 

My vote goes to doing nothing :).

--

Sent from Gmail Mobile

[Norbert Melzer]

Isaac Whitfield <i...@whitfin.io> schrieb am Mi., 22. Nov. 2017 um 22:55 Uhr:

Wouldn't the outcome `not Integer.is_negative(:atom)` be even worse than what we have now?

What do you mean by worse? If you take the face value of what the macro does "is this a negative integer?", then no, it's not worse.

Well, it has been proposed as an alternative to `is_non_negative/1`, which it clearly isn't which my example shows.

Personally I prefer to simply stick to our current explicit `is_number(x) and x > @threshold` and friends over a set of macros that only covers an arbitrarily chosen threshold.

If something is introduced at all, it should be `m.gt`, `m.lt`, `m.gte`, and `m.lte`, where `m` is `Integer`, `Float`, and `Kernel` which are integer, float and number guarded.

Bye,

  Norbert