Nim: `+=` (+ friends) shouldn't be allowed for `bool`

Created on 10 Jan 2019  路  9Comments  路  Source: nim-lang/Nim

in most languages, += isn't allowed for bool, but in Nim it is.
I can't think of any use case for that "feature".

proc main()=
  var a=false
  a+=true # ok ; proposal: CT error
  echo a
  a+=true # Error: unhandled exception: over- or underflow [OverflowError]

main()

this can lead to OverflowError and also masks the lack of error probably hides bugs (programmer probably didn't intend a to be bool in that context; which just happened to me)

proposal

disallow integer arithmetic operators on bool, instead force using boolean operators. They can't ever lead to such OverflowError so are safer.

RFC

Most helpful comment

also, @krux02, closing an issue/RFC so quickly before it has a chance of being viewed/commented on by other ppl is not very productive; (unless the issue is objectively wrong/false or fixed by some subsequent PR); I understand we have a lot of open issues and a desire to reduce them, but a gestation period is usually good.

All 9 comments

In Nim bool is not special, it is just enum like any other. And it should remain this way.

The only thing can be done is to have a way for user to specify that enum is not ordinal even if values are consecutive

Well you have a point that it isn't constructive to use += on bool values, but as long as users are not actively using this "feature" and run into problems because the compiler didn't warn or stop them from doing this, I don't see a problem in it.

It's a valid issue and Nim is not C++, Nim is strictly typed.

also, @krux02, closing an issue/RFC so quickly before it has a chance of being viewed/commented on by other ppl is not very productive; (unless the issue is objectively wrong/false or fixed by some subsequent PR); I understand we have a lot of open issues and a desire to reduce them, but a gestation period is usually good.

Well, I was thinking about it, and you are right. This shouldn't be allowed. I found this in system.nim:

proc `+=`*[T: SomeOrdinal|uint|uint64](x: var T, y: T) {.
  magic: "Inc", noSideEffect.}
  ## Increments an ordinal

proc `-=`*[T: SomeOrdinal|uint|uint64](x: var T, y: T) {.
  magic: "Dec", noSideEffect.}
  ## Decrements an ordinal

proc `*=`*[T: SomeOrdinal|uint|uint64](x: var T, y: T) {.
  inline, noSideEffect.} =
  ## Binary `*=` operator for ordinals
  x = x * y

# note SomeOrdinal = int|int8|int16|int32|int64|bool|enum|uint8|uint16|uint32

I think the type restriction is unsuitable for all of the given types. I think SomeNumber would be a much more suitable constraint. I think it is possible to deprecate just overloads of enum and bool for a deprecation period.

also, @krux02, closing an issue/RFC so quickly before it has a chance of being viewed/commented on by other ppl is not very productive

I would like to emphasize this because I have noticed @krux02 doing this before. A majority of our issues are old, focus on closing them instead of closing new issues that we didn't get a chance to have a proper discussion on yet.

@dom96 Well currently we are drowning in issues and the amount of issues that can be discussed per day and per person is very limited. So we should really focus carefully on what issues we should pay attention to and what issues are just not worth it. Closing issues before anybody can see them can be very helpful to prevent that something catches attention and distracts from issues that might actually be worth checking out. Personally I think we need some sort of issue review process that ensures that the issues is formally correct before we investigate further.

But for this issue in particular, I am sorry. This is a valid issue and I am all in for more and better constraints for the language.

So we should really focus carefully on what issues we should pay attention to and what issues are just not worth it. Closing issues before anybody can see them can be very helpful to prevent that something catches attention and distracts from issues that might actually be worth checking out.

Yes, but closing an issue doesn't remove it from our notifications.

If we want to narrow our focus I think it might be time to start thinking about automatically closing issues that haven't had activity for 3+ months (with the exceptions of issues tagged with something like donotclose). We have a lot of issues that we simply don't have resources to fix, and I think it might be okay to forget them, especially if the original person that reported the issue no longer cares about it or Nim for that matter.

@dom96

automatically closing issues that haven't had activity for 3+ months

strongly opposed to that. you can just set a filter to ignore issues > 3+ months when viewing issues (and sort by recent actvity). Some repos do that (with a bot that automatically closes stale issues) and it serves no useful purpose other than reducing the number of open issues. The only thing that matters is fixing issues that matter the most (and what matters depends on ppl/workflows), not the purely accounting number of opened issues.

Eg: say a new useful feature is introduced (eg nimpretty); this causes 10 new bugs to appear. Are we in a worse situation after that (because of 10 new open issues)? of course not.

prioritize and fix bugs that matter to you/others, but don't close old bugs just because they're old / haven't had activity (yet remain valid bugs)

Was this page helpful?
0 / 5 - 0 ratings